On Fri, 24 Apr 2020 at 14:08, André Przywara <andre.przywara@xxxxxxx> wrote: > > On 24/04/2020 07:45, Ard Biesheuvel wrote: > > Hi, > > (adding Leif for EDK-2 discussion) > > > On Thu, 23 Apr 2020 at 23:32, André Przywara <andre.przywara@xxxxxxx> wrote: > >> > >> On 23/04/2020 21:43, Ard Biesheuvel wrote: > > [ ... kvmtool series to add CFI flash emulation allowing EDK-2 to store > variables. Starting with this version (v4) the flash memory region is > presented as a read-only memslot to KVM, to allow direct guest accesses > as opposed to trap-and-emulate even read accesses to the array.] > > >> > >> > >> Just curious: the images Sami gave me this morning did not show any > >> issues anymore (no no-syndrome fault, no alignment issues), even without > >> the mapping [1]. And even though I saw the 800k read traps, I didn't > >> notice any real performance difference (on a Juno). The PXE timeout was > >> definitely much more noticeable. > >> > >> So did you see any performance impact with this series? > >> > > > > You normally don't PXE boot. There is an issue with the iSCSI driver > > as well, which causes a boot delay for some reason, so I disabled that > > in my build. > > > > I definitely *feels* faster :-) But in any case, exposing the array > > mode as a r/o memslot is definitely the right way to deal with this. > > Even if Sami did find a workaround that masks the error, it is no > > guarantee that all accesses go through that library. > > So I was wondering about this, maybe you can confirm or debunk this: > - Any memory given to the compiler (through a pointer) is assumed to be > "normal" memory: the compiler can re-arrange accesses, split them up or > collate them. Also unaligned accesses should be allowed - although I > guess most compilers would avoid them. > - This normally forbids to give a pointer to memory mapped as "device > memory" to the compiler, since this would violate all of the assumptions > above. > - If the device mapped as "device memory" is actually memory (SRAM, > ROM/flash, framebuffer), then most of the assumptions are met, except > the alignment requirement, which is bound to the mapping type, not the > actual device (ARMv8 ARM: Unaligned accesses to device memory always > trap, regardless of SCTLR.A) > - To accommodate the latter, GCC knows the option -malign-strict, to > avoid unaligned accesses. TF-A and U-Boot use this option, to run > without the MMU enabled. > > Now if EDK-2 lets the compiler deal with the flash memory region > directly, I think this would still be prone to alignment faults. In fact > an earlier build I got from Sami faulted on exactly that, when I ran it, > even with the r/o memslot mapping in place. > > So should EDK-2 add -malign-strict to be safe? It already uses this in various places where it matters. > or > Should EDK-2 add an additional or alternate mapping using a non-device > memory type (with all the mismatched attributes consequences)? The memory mapped NOR flash in UEFI is really a special case, since we need the OS to map it for us at runtime, and we cannot tell it to switch between normal-NC and device attributes depending on which mode the firmware is using it in. Note that this is not any different on bare metal. > or > Should EDK-2 only touch the flash region using MMIO accessors, and > forbid the compiler direct access to that region? > It should only touch those regions using abstractions it defines itself, and which can be backed in different ways. This is already the case in EDK2: it has its own CopyMem, ZeroMem, etc string library, and bans the use the standard C ones. On top of that, it bans struct assignment, initializers for automatic variables and are things that result in such calls to be emitted implicitly. So in practice, this issue is under control, unless you use a version of those abstractions that willingly uses unaligned accesses (we have optimized versions based on the cortex-strings library). So my suspicion is that this may have caused the crash: on bare metal, we have to switch to the non-optimized string library for the variable driver for this reason. The real solution is to fix EDK2, and make the variable stack work with NOR flash that is non-memory mapped. This is something that has come up before, and the other day, Sami and I were just discussing logging this as a wishlist item for the firmware team. > So does EDK-2 get away with this because the compiler typically avoids > unaligned accesses? > There are certainly some places in the current code base where it is the compiler that is emitting reads from the NOR flash region, but there aren't that many. Moving the variable data itself in and out will typically use the abstractions, since it is moving anonymous chunks of data. However, there are places where, e.g., fields in the FS metadata are being read by the code, and there it just casts an address pointing into the NOR flash region to the appropriate struct type, and dereferences the fields.
