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 _______________________________________________ edk2-devel mailing list edk2-devel@lists.01.org https://lists.01.org/mailman/listinfo/edk2-devel