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

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

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.