Re: [PATCH] ArmVirtPkg: restrict mapping attributes of normal memory to EFI_MEMORY_WB

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

On 8 September 2016 at 08:57, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 8 September 2016 at 08:54, Laszlo Ersek <lersek@redhat.com> wrote:
>> On 09/08/16 09:50, Ard Biesheuvel wrote:
>>> In general, on an ARM system, 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.
>>>
>>> Under KVM, the situation is even worse, since the host may not expect the
>>> guest to perform uncached accesses, and so writes to such an uncached
>>> region may get lost completely.
>>>
>>> Since the only safe mapping type under KVM is EFI_MEMORY_WB, remove all
>>> other memory type attributes.
>>>
>>> Contributed-under: TianoCore Contribution Agreement 1.0
>>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>>> ---
>>>  ArmVirtPkg/HighMemDxe/HighMemDxe.c                                   | 3 +--
>>>  ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c | 3 ---
>>>  2 files changed, 1 insertion(+), 5 deletions(-)
>>>
>>> diff --git a/ArmVirtPkg/HighMemDxe/HighMemDxe.c b/ArmVirtPkg/HighMemDxe/HighMemDxe.c
>>> index 7fd7e8e9a539..4d56e6236b54 100644
>>> --- a/ArmVirtPkg/HighMemDxe/HighMemDxe.c
>>> +++ b/ArmVirtPkg/HighMemDxe/HighMemDxe.c
>>> @@ -78,8 +78,7 @@ InitializeHighMemDxe (
>>>            Status = gDS->AddMemorySpace (
>>>                            EfiGcdMemoryTypeSystemMemory,
>>>                            CurBase, CurSize,
>>> -                          EFI_MEMORY_WB | EFI_MEMORY_WC |
>>> -                          EFI_MEMORY_WT | EFI_MEMORY_UC);
>>> +                          EFI_MEMORY_WB);
>>>
>>>            if (EFI_ERROR (Status)) {
>>>              DEBUG ((EFI_D_ERROR,
>>> diff --git a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
>>> index 251e5314e61d..6f3e54b7afcb 100644
>>> --- a/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
>>> +++ b/ArmVirtPkg/Library/ArmVirtMemoryInitPeiLib/ArmVirtMemoryInitPeiLib.c
>>> @@ -68,9 +68,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 |
>>>        EFI_RESOURCE_ATTRIBUTE_TESTED
>>>    );
>>>
>>
>> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>>
>> Did you encounter any specific problem with these?
>>
>
> No, but I am looking into using the new optimized BaseMemoryLibOptDxe,
> and the AARCH64 version uses DC ZVA instructions for ZeroMem() [i.e.,
> zero a cacheline, which is much faster than writing zeroes, obviously]
> That does not work on uncached memory, and so while I was removing
> /that/ attribute from ArmVirtPkg, I realized that WC and WT are not
> safe either.
>
> For bare metal platforms, we will keep WC and WT, but for ArmVirtQemu,
> they don't make sense. Not sure about Xen though ...

Pushed, thanks.