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

[Leif Lindholm <leif.lindholm@linaro.org>]

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?

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?

> >>> 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