Re: [PATCH v5 4/4] MdePkg/BaseMemoryLibOptDxe ARM|AARCH64: disallow use in SEC & PEI phases

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

On 6 April 2017 at 10:35, Leif Lindholm <leif.lindholm@linaro.org> wrote:
> On Wed, Apr 05, 2017 at 10:55:49PM +0100, Ard Biesheuvel wrote:
>> >>> I think this is a problem because nowhere in the UEFI specs do I see such
>> >>> restrictions on those memory operations.
>> >>
>> >> Using device attributes for memory is something we should ban for
>> >> AArch64 in the spec.
>
> Yes, completely agree. And doing so is generally the result of
> misinderstanding the memory model (i.e., it probably won't provide the
> guarantee that was sought).
> Charles/Dong? Something to add to list?
>

As an additional note, the UEFI spec mandates that unaligned accesses
are enabled for AArch64, which clearly expresses the intent that
routines operating on memory should be able to do so without going out
of its way to avoid unaligned accesses.

> Can we insert a test preventing device memory type to be set for
> regions with _WB attribute? Or is that already only possible through
> manual trickery?
>

We should simply remove the _UC attribute from all memory. I have
already done so for many of the platforms I more or less maintain (and
for virt, we removed _WT and _WC as well, because KVM only supports
_WB)

Note that this does not prevent the NOR and RTC drivers from creating
_UC regions for their own MMIO registers, it just prevents them from
being remapped _UC via the DXE services.

>> >>> For a specific problematic example, the LcdGraphicsOutputBlt.c uses it
>> >>> for
>> >>> BltVideoFill() and the target of that is likely not regular cached video
>> >>> memory.
>> >>
>> >> Those drivers should be using EFI_MEMORY_WC not EFI_MEMORY_UC for the
>> >> VRAM mapping. Note that EFI_MEMORY_UC is nGnRnE which is unnecessarily
>> >> restrictive.
>> >>
>> >> I agree there is a general issue here which we should address by
>> >> tightening the spec. I don't see a lot of value in avoiding DC ZVA and
>> >> unaligned accesses altogether, I'd rather fix the code instead.
>> >
>> > While I agree with the general sentiment, I find the result brittle. If it
>> > were used as a DEBUG build way to locate sub-optmimal code I would be more
>> > on board. But shipping it like this, puts it into situations where the user
>> > inadvertently changes something (say making the background black and
>> > therefore triggering the DC) or some obscure option ROM (we will get there
>> > right??!!) triggers it in a place where it can't be debugged.
>> >
>> > Particularly since we are talking boot, where the few percent perf
>> > improvement on this operation is likely completely undetectable. The one
>> > place where I can think it might even be measurable is in routines to clear
>> > system memory, and those routines could be a special case anyway.
>>
>> I guess this depends on the use case. For server, it may not matter,
>> but the case is different for mobile, and the Broadcom engineers that
>> did some benchmarks on this code were very pleased with the result
>> (and the speedup was significant, although I don't know which routines
>> are the hotspots)
>>
>> As for option ROMs: those will link to their own BaseMemoryLib
>> implementation (assuming that they are EDK2 based) so the only way
>> they would have access to these routines is via the CopyMem() and
>> SetMem() boot services. Note that that does not dismiss the concern at
>> all, it is just a clarification.
>>
>> Leif, any thoughts?
>
> I would prefer if we could resolve this without waiting for a new spec
> version.
>
> My gut feeling is that the (end-user, I care a _lot_ less
> about development platforms) devices that _could_ be affected by this
> won't be releasing updated firmwares completely rebased onto a newer
> edk2 HEAD. Rather they're likely to be cherry-picking individual
> bugfixes and improvements.
>
> But certainly having some input from abovementioned Broadcom team,
> Evan & co, and others is important before we make a call.
>
> /
>     Leif