[PATCH] ArmPlatformPkg: remove EFI_MEMORY_UC attribute from normal memory

[PATCH] ArmPlatformPkg: remove EFI_MEMORY_UC attribute from normal memory

[Ard Biesheuvel]

On ARM systems, mapping normal memory as device memory may have unintended
side effects, given that unaligned accesses or loads and stores with special
semantics (e.g., load/store exclusive) may fault or may not work as expected.
Similarly, DC ZVA instructions are only supported on normal memory, not
device memory.

So remove the EFI_MEMORY_UC attribute that we set by default on system RAM.
If any region requires this attribute, it is up to the driver to set this
attribute, and to ensure that no offending operations are performed on it.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---
 ArmPlatformPkg/ArmJunoPkg/Library/ArmJunoLib/ArmJunoMem.c          | 1 -
 ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c | 1 -
 ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c                    | 1 -
 3 files changed, 3 deletions(-)

diff --git a/ArmPlatformPkg/ArmJunoPkg/Library/ArmJunoLib/ArmJunoMem.c b/ArmPlatformPkg/ArmJunoPkg/Library/ArmJunoLib/ArmJunoMem.c
index 41731c1ebdb8..aa8d7d9c3b0d 100644
--- a/ArmPlatformPkg/ArmJunoPkg/Library/ArmJunoLib/ArmJunoMem.c
+++ b/ArmPlatformPkg/ArmJunoPkg/Library/ArmJunoLib/ArmJunoMem.c
@@ -56,7 +56,6 @@ ArmPlatformGetVirtualMemoryMap (
   ResourceAttributes =
       EFI_RESOURCE_ATTRIBUTE_PRESENT |
       EFI_RESOURCE_ATTRIBUTE_INITIALIZED |
-      EFI_RESOURCE_ATTRIBUTE_UNCACHEABLE |
       EFI_RESOURCE_ATTRIBUTE_WRITE_COMBINEABLE |
       EFI_RESOURCE_ATTRIBUTE_WRITE_THROUGH_CACHEABLE |
       EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE |
diff --git a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
index c6df2e1b3de4..115df246796d 100644
--- a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
+++ b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
@@ -61,7 +61,6 @@ ArmPlatformGetVirtualMemoryMap (
     ResourceAttributes =
         EFI_RESOURCE_ATTRIBUTE_PRESENT |
         EFI_RESOURCE_ATTRIBUTE_INITIALIZED |
-        EFI_RESOURCE_ATTRIBUTE_UNCACHEABLE |
         EFI_RESOURCE_ATTRIBUTE_WRITE_COMBINEABLE |
         EFI_RESOURCE_ATTRIBUTE_WRITE_THROUGH_CACHEABLE |
         EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE |
diff --git a/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c b/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c
index 75e6631d7f30..2feb11f21d5d 100644
--- a/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c
+++ b/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c
@@ -89,7 +89,6 @@ MemoryPeim (
   ResourceAttributes = (
       EFI_RESOURCE_ATTRIBUTE_PRESENT |
       EFI_RESOURCE_ATTRIBUTE_INITIALIZED |
-      EFI_RESOURCE_ATTRIBUTE_UNCACHEABLE |
       EFI_RESOURCE_ATTRIBUTE_WRITE_COMBINEABLE |
       EFI_RESOURCE_ATTRIBUTE_WRITE_THROUGH_CACHEABLE |
       EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE |
-- 
2.7.4

Re: [PATCH] ArmPlatformPkg: remove EFI_MEMORY_UC attribute from normal memory

[Leif Lindholm]

On Thu, Sep 08, 2016 at 09:13:38AM +0100, Ard Biesheuvel wrote:
> On ARM systems, mapping normal memory as device memory may have unintended
> side effects, given that unaligned accesses or loads and stores with special
> semantics (e.g., load/store exclusive) may fault or may not work as expected.
> Similarly, DC ZVA instructions are only supported on normal memory, not
> device memory.

I think this is the right thing to do; Arguably, on the modern ARM
architectures, UNCACHEABLE and WRITE_COMBINEABLE are mutually
exclusive. I'll discuss with Charles whether we should codify this in
the UEFI specification.

> So remove the EFI_MEMORY_UC attribute that we set by default on system RAM.
> If any region requires this attribute, it is up to the driver to set this
> attribute, and to ensure that no offending operations are performed on it.
> 
> Contributed-under: TianoCore Contribution Agreement 1.0
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>

> ---
>  ArmPlatformPkg/ArmJunoPkg/Library/ArmJunoLib/ArmJunoMem.c          | 1 -
>  ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c | 1 -
>  ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c                    | 1 -
>  3 files changed, 3 deletions(-)
> 
> diff --git a/ArmPlatformPkg/ArmJunoPkg/Library/ArmJunoLib/ArmJunoMem.c b/ArmPlatformPkg/ArmJunoPkg/Library/ArmJunoLib/ArmJunoMem.c
> index 41731c1ebdb8..aa8d7d9c3b0d 100644
> --- a/ArmPlatformPkg/ArmJunoPkg/Library/ArmJunoLib/ArmJunoMem.c
> +++ b/ArmPlatformPkg/ArmJunoPkg/Library/ArmJunoLib/ArmJunoMem.c
> @@ -56,7 +56,6 @@ ArmPlatformGetVirtualMemoryMap (
>    ResourceAttributes =
>        EFI_RESOURCE_ATTRIBUTE_PRESENT |
>        EFI_RESOURCE_ATTRIBUTE_INITIALIZED |
> -      EFI_RESOURCE_ATTRIBUTE_UNCACHEABLE |
>        EFI_RESOURCE_ATTRIBUTE_WRITE_COMBINEABLE |
>        EFI_RESOURCE_ATTRIBUTE_WRITE_THROUGH_CACHEABLE |
>        EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE |
> diff --git a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
> index c6df2e1b3de4..115df246796d 100644
> --- a/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
> +++ b/ArmPlatformPkg/ArmVExpressPkg/Library/ArmVExpressLibRTSM/RTSMMem.c
> @@ -61,7 +61,6 @@ ArmPlatformGetVirtualMemoryMap (
>      ResourceAttributes =
>          EFI_RESOURCE_ATTRIBUTE_PRESENT |
>          EFI_RESOURCE_ATTRIBUTE_INITIALIZED |
> -        EFI_RESOURCE_ATTRIBUTE_UNCACHEABLE |
>          EFI_RESOURCE_ATTRIBUTE_WRITE_COMBINEABLE |
>          EFI_RESOURCE_ATTRIBUTE_WRITE_THROUGH_CACHEABLE |
>          EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE |
> diff --git a/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c b/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c
> index 75e6631d7f30..2feb11f21d5d 100644
> --- a/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c
> +++ b/ArmPlatformPkg/MemoryInitPei/MemoryInitPeiLib.c
> @@ -89,7 +89,6 @@ MemoryPeim (
>    ResourceAttributes = (
>        EFI_RESOURCE_ATTRIBUTE_PRESENT |
>        EFI_RESOURCE_ATTRIBUTE_INITIALIZED |
> -      EFI_RESOURCE_ATTRIBUTE_UNCACHEABLE |
>        EFI_RESOURCE_ATTRIBUTE_WRITE_COMBINEABLE |
>        EFI_RESOURCE_ATTRIBUTE_WRITE_THROUGH_CACHEABLE |
>        EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE |
> -- 
> 2.7.4
> 

Re: [PATCH] ArmPlatformPkg: remove EFI_MEMORY_UC attribute from normal memory

[Ard Biesheuvel]

On 8 September 2016 at 10:21, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Thu, Sep 08, 2016 at 09:13:38AM +0100, Ard Biesheuvel wrote:
>> On ARM systems, mapping normal memory as device memory may have unintended
>> side effects, given that unaligned accesses or loads and stores with special
>> semantics (e.g., load/store exclusive) may fault or may not work as expected.
>> Similarly, DC ZVA instructions are only supported on normal memory, not
>> device memory.
>
> I think this is the right thing to do; Arguably, on the modern ARM
> architectures, UNCACHEABLE and WRITE_COMBINEABLE are mutually
> exclusive. I'll discuss with Charles whether we should codify this in
> the UEFI specification.
>
>> So remove the EFI_MEMORY_UC attribute that we set by default on system RAM.
>> If any region requires this attribute, it is up to the driver to set this
>> attribute, and to ensure that no offending operations are performed on it.
>>
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>
> Reviewed-by: Leif Lindholm <leif.lindholm@linaro.org>
>

Pushed, thanks.

I will follow up with similar patches for OpenPlatformPkg

Re: [PATCH] ArmPlatformPkg: remove EFI_MEMORY_UC attribute from normal memory

[Cohen, Eugene]

Ard,

> So remove the EFI_MEMORY_UC attribute that we set by default on
> system RAM.
> If any region requires this attribute, it is up to the driver to set this
> attribute, and to ensure that no offending operations are performed
> on it.
>

For DMA common-buffer operations on systems without snooping DMA capabilities, UC or WC mapping of system memory regions is required.

>        EFI_RESOURCE_ATTRIBUTE_PRESENT |
>        EFI_RESOURCE_ATTRIBUTE_INITIALIZED |
> -      EFI_RESOURCE_ATTRIBUTE_UNCACHEABLE |
>        EFI_RESOURCE_ATTRIBUTE_WRITE_COMBINEABLE |
>        EFI_RESOURCE_ATTRIBUTE_WRITE_THROUGH_CACHEABLE |
>        EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE |

The EFI_RESOURCE_ATTRIBUTE_UNCACHEABLE bit your removing is removing the capability for uncacheable memory such that even if a driver wanted to make a DMA buffer uncacheable GCD will no longer allow this because the resource does not support this capability.

Is it your intent to indicate that system memory is no longer capable of being uncacheable?  If so how would you plan to accomodate the DMA use case for GCD SetMemorySpaceAttributes?

Thanks,

Eugene

Re: [PATCH] ArmPlatformPkg: remove EFI_MEMORY_UC attribute from normal memory

[Cohen, Eugene]

> I think this is the right thing to do; Arguably, on the modern ARM
> architectures, UNCACHEABLE and WRITE_COMBINEABLE are mutually
> exclusive. I'll discuss with Charles whether we should codify this in
> the UEFI specification.

Given the corresponding X86 semantics it makes sense for UNCACHEABLE to map to Strongly Ordered and WRITE_COMBINEABLE to map to "Normal" Uncacheable.   It's useful to expose this separately in case a DMA common buffer has semantics that require the strongly ordered behavior.

Since this is providing a list of capabilities I'm not sure what the statement about mutual exclusivity refers to.

Eugene

Re: [PATCH] ArmPlatformPkg: remove EFI_MEMORY_UC attribute from normal memory

[Ard Biesheuvel]

On 8 September 2016 at 18:33, Cohen, Eugene <eugene@hp.com> wrote:
> Ard,
>
>> So remove the EFI_MEMORY_UC attribute that we set by default on
>> system RAM.
>> If any region requires this attribute, it is up to the driver to set this
>> attribute, and to ensure that no offending operations are performed
>> on it.
>>
>
> For DMA common-buffer operations on systems without snooping DMA capabilities, UC or WC mapping of system memory regions is required.
>
>>        EFI_RESOURCE_ATTRIBUTE_PRESENT |
>>        EFI_RESOURCE_ATTRIBUTE_INITIALIZED |
>> -      EFI_RESOURCE_ATTRIBUTE_UNCACHEABLE |
>>        EFI_RESOURCE_ATTRIBUTE_WRITE_COMBINEABLE |
>>        EFI_RESOURCE_ATTRIBUTE_WRITE_THROUGH_CACHEABLE |
>>        EFI_RESOURCE_ATTRIBUTE_WRITE_BACK_CACHEABLE |
>
> The EFI_RESOURCE_ATTRIBUTE_UNCACHEABLE bit your removing is removing the capability for uncacheable memory such that even if a driver wanted to make a DMA buffer uncacheable GCD will no longer allow this because the resource does not support this capability.
>
> Is it your intent to indicate that system memory is no longer capable of being uncacheable?  If so how would you plan to accomodate the DMA use case for GCD SetMemorySpaceAttributes?
>

In general, memory should be mapped as memory, and if strongly ordered
mappings of RAM are required in some cases, it is up to the module
itself to set the memory space capabilities, and to ensure that
routines that expect normal memory are not used on them. The typical
example is the proposed ARM implementation of BaseMemoryLibOptDxe,
which uses unaligned accesses and DC ZVA operations to speed up
operations.

I checked ArmDmaLib, and it uses WC mappings for its uncached
allocations, so those users should be fine. Do you have any examples
of drivers that require device mappings of RAM for DMA? I think it
would be preferable in this case to carve out a chunk of memory at the
platform level instead of annotating all of RAM with the UC bit, given
that it turns up in runtime services code/data regions as well, giving
the OS license to map it using attributes that may be problematic.

Re: [PATCH] ArmPlatformPkg: remove EFI_MEMORY_UC attribute from normal memory

[Leif Lindholm]

On Thu, Sep 08, 2016 at 05:37:13PM +0000, Cohen, Eugene wrote:
> > I think this is the right thing to do; Arguably, on the modern ARM
> > architectures, UNCACHEABLE and WRITE_COMBINEABLE are mutually
> > exclusive. I'll discuss with Charles whether we should codify this in
> > the UEFI specification.
> 
> Given the corresponding X86 semantics it makes sense for UNCACHEABLE
> to map to Strongly Ordered and WRITE_COMBINEABLE to map to "Normal"
> Uncacheable.   It's useful to expose this separately in case a DMA
> common buffer has semantics that require the strongly ordered
> behavior.
> 
> Since this is providing a list of capabilities I'm not sure what the
> statement about mutual exclusivity refers to.

Do note the weasly "arguably" I stuck in there.

My point is basically the same as yours, with the clarification that
for purposes of treating something like general-purpose memory,
flagging a location as possible to map as either UNCACHEABLE (ARM:
Strongly Ordered) or WRITE_COMBINEABLE (ARM: Normal uncached)
generally does not make sense.

Regards,

Leif