| Message ID | 20200423173844.24220-1-andre.przywara@arm.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Add CFI flash emulation | expand |
On Thu, 23 Apr 2020 at 19:39, Andre Przywara <andre.przywara@arm.com> wrote: > > Hi, > > an update for the CFI flash emulation, addressing Alex' comments and > adding direct mapping support. > The actual code changes to the flash emulation are minimal, mostly this > is about renaming and cleanups. > This versions now adds some patches. 1/5 is a required fix, the last > three patches add mapping support as an extension. See below. > > In addition to a branch with this series[1], I also put a git branch with > all the changes compared to v3[2] as separate patches on the server, please > have a look if you want to verify against a previous review. > > =============== > The EDK II UEFI firmware implementation requires some storage for the EFI > variables, which is typically some flash storage. > Since this is already supported on the EDK II side, and looks like a > generic standard, this series adds a CFI flash emulation to kvmtool. > > Patch 2/5 is the actual emulation code, patch 1/5 is a bug-fix for > registering MMIO devices, which is needed for this device. > Patches 3-5 add support for mapping the flash memory into guest, should > it be in read-array mode. For this to work, patch 3/5 is cherry-picked > from Alex' PCIe reassignable BAR series, to support removing a memslot > mapping. Patch 4/5 adds support for read-only mappings, while patch 5/5 > adds or removes the mapping based on the current state. > I am happy to squash 5/5 into 2/5, if we agree that patch 3/5 should be > merged either separately or the PCIe series is actually merged before > this one. > > This is one missing piece towards a working UEFI boot with kvmtool on > ARM guests, the other is to provide writable PCI BARs, which is WIP. > This series alone already enables UEFI boot, but only with virtio-mmio. > Excellent! Thanks for taking the time to implement the r/o memslot for the flash, it really makes the UEFI firmware much more usable. I will test this as soon as I get a chance, probably tomorrow. > > [1] http://www.linux-arm.org/git?p=kvmtool.git;a=log;h=refs/heads/cfi-flash/v4 > [2] http://www.linux-arm.org/git?p=kvmtool.git;a=log;h=refs/heads/cfi-flash/v3 > git://linux-arm.org/kvmtool.git (branches cfi-flash/v3 and cfi-flash/v4) > > Changelog v3 .. v4: > - Rename file to cfi-flash.c (dash instead of underscore). > - Unify macro names for states, modes and commands. > - Enforce one or two chips only. > - Comment on pow2_size() function. > - Use more consistent identifier spellings. > - Assign symbols to status register values. > - Drop RCR register emulation. > - Use numerical offsets instead of names for query offsets to match spec. > - Cleanup error path and reword info message in create_flash_device_file(). > - Add fix to allow non-virtio MMIO device emulations. > - Support tearing down and adding read-only memslots. > - Add read-only memslot mapping when in read mode. > > Changelog v2 .. v3: > - Breaking MMIO handling into three separate functions. > - Assing the flash base address in the memory map, but stay at 32 MB for now. > The MMIO area has been moved up to 48 MB, to never overlap with the > flash. > - Impose a limit of 16 MB for the flash size, mostly to fit into the > (for now) fixed memory map. > - Trim flash size down to nearest power-of-2, to match hardware. > - Announce forced flash size trimming. > - Rework the CFI query table slightly, to add the addresses as array > indicies. > - Fix error handling when creating the flash device. > - Fix pow2_size implementation for 0 and 1 as input values. > - Fix write buffer size handling. > - Improve some comments. > > Changelog v1 .. v2: > - Add locking for MMIO handling. > - Fold flash read into handler. > - Move pow2_size() into generic header. > - Spell out flash base address.
On Thu, 23 Apr 2020 at 19:55, Ard Biesheuvel <ardb@kernel.org> wrote: > > On Thu, 23 Apr 2020 at 19:39, Andre Przywara <andre.przywara@arm.com> wrote: > > > > Hi, > > > > an update for the CFI flash emulation, addressing Alex' comments and > > adding direct mapping support. > > The actual code changes to the flash emulation are minimal, mostly this > > is about renaming and cleanups. > > This versions now adds some patches. 1/5 is a required fix, the last > > three patches add mapping support as an extension. See below. > > > > In addition to a branch with this series[1], I also put a git branch with > > all the changes compared to v3[2] as separate patches on the server, please > > have a look if you want to verify against a previous review. > > > > =============== > > The EDK II UEFI firmware implementation requires some storage for the EFI > > variables, which is typically some flash storage. > > Since this is already supported on the EDK II side, and looks like a > > generic standard, this series adds a CFI flash emulation to kvmtool. > > > > Patch 2/5 is the actual emulation code, patch 1/5 is a bug-fix for > > registering MMIO devices, which is needed for this device. > > Patches 3-5 add support for mapping the flash memory into guest, should > > it be in read-array mode. For this to work, patch 3/5 is cherry-picked > > from Alex' PCIe reassignable BAR series, to support removing a memslot > > mapping. Patch 4/5 adds support for read-only mappings, while patch 5/5 > > adds or removes the mapping based on the current state. > > I am happy to squash 5/5 into 2/5, if we agree that patch 3/5 should be > > merged either separately or the PCIe series is actually merged before > > this one. > > > > This is one missing piece towards a working UEFI boot with kvmtool on > > ARM guests, the other is to provide writable PCI BARs, which is WIP. > > This series alone already enables UEFI boot, but only with virtio-mmio. > > > > Excellent! Thanks for taking the time to implement the r/o memslot for > the flash, it really makes the UEFI firmware much more usable. > > I will test this as soon as I get a chance, probably tomorrow. > I tested this on a SynQuacer box as a host, using EFI firmware [0] built from patches provided by Sami. I booted the Debian buster installer, completed the installation, and could boot into the system. The only glitch was the fact that the reboot didn't work, but I suppose we are not preserving the memory the contains the firmware image, so there is nothing to reboot into. But just restarting kvmtool with the flash image containing the EFI variables got me straight into GRUB and the installed OS. Tested-by: Ard Biesheuvel <ardb@kernel.org> Thanks again for getting this sorted. [0] https://people.linaro.org/~ard.biesheuvel/KVMTOOL_EFI.fd
On 23/04/2020 21:43, Ard Biesheuvel wrote: Hi Ard, > On Thu, 23 Apr 2020 at 19:55, Ard Biesheuvel <ardb@kernel.org> wrote: >> >> On Thu, 23 Apr 2020 at 19:39, Andre Przywara <andre.przywara@arm.com> wrote: >>> >>> Hi, >>> >>> an update for the CFI flash emulation, addressing Alex' comments and >>> adding direct mapping support. >>> The actual code changes to the flash emulation are minimal, mostly this >>> is about renaming and cleanups. >>> This versions now adds some patches. 1/5 is a required fix, the last >>> three patches add mapping support as an extension. See below. >>> >>> In addition to a branch with this series[1], I also put a git branch with >>> all the changes compared to v3[2] as separate patches on the server, please >>> have a look if you want to verify against a previous review. >>> >>> =============== >>> The EDK II UEFI firmware implementation requires some storage for the EFI >>> variables, which is typically some flash storage. >>> Since this is already supported on the EDK II side, and looks like a >>> generic standard, this series adds a CFI flash emulation to kvmtool. >>> >>> Patch 2/5 is the actual emulation code, patch 1/5 is a bug-fix for >>> registering MMIO devices, which is needed for this device. >>> Patches 3-5 add support for mapping the flash memory into guest, should >>> it be in read-array mode. For this to work, patch 3/5 is cherry-picked >>> from Alex' PCIe reassignable BAR series, to support removing a memslot >>> mapping. Patch 4/5 adds support for read-only mappings, while patch 5/5 >>> adds or removes the mapping based on the current state. >>> I am happy to squash 5/5 into 2/5, if we agree that patch 3/5 should be >>> merged either separately or the PCIe series is actually merged before >>> this one. >>> >>> This is one missing piece towards a working UEFI boot with kvmtool on >>> ARM guests, the other is to provide writable PCI BARs, which is WIP. >>> This series alone already enables UEFI boot, but only with virtio-mmio. >>> >> >> Excellent! Thanks for taking the time to implement the r/o memslot for >> the flash, it really makes the UEFI firmware much more usable. >> >> I will test this as soon as I get a chance, probably tomorrow. >> > > I tested this on a SynQuacer box as a host, using EFI firmware [0] > built from patches provided by Sami. > > I booted the Debian buster installer, completed the installation, and > could boot into the system. The only glitch was the fact that the > reboot didn't work, but I suppose we are not preserving the memory the > contains the firmware image, so there is nothing to reboot into. It's even worth, kvmtool does actually not support reset at all: https://git.kernel.org/pub/scm/linux/kernel/git/will/kvmtool.git/tree/kvm-cpu.c#n220 And yeah, the UEFI firmware is loaded at the beginning of RAM, so most of it is long gone by then. kvmtool could reload the image and reset the VCPUs, but every device emulation would need to be reset, for which there is no code yet. > But > just restarting kvmtool with the flash image containing the EFI > variables got me straight into GRUB and the installed OS. So, yeah, this is the way to do it ;-) > Tested-by: Ard Biesheuvel <ardb@kernel.org> Many thanks for that! > Thanks again for getting this sorted. It was actually easier than I thought (see the last patch). 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? > [0] https://people.linaro.org/~ard.biesheuvel/KVMTOOL_EFI.fd Ah, nice, will this stay there for a while? I can't provide binaries, but wanted others to be able to easily test this. Cheers, Andre. [1] http://www.linux-arm.org/git?p=kvmtool.git;a=commitdiff;h=2f2cf67f9514894d88e9ca799bb9dacd1f7557d4
On Thu, 23 Apr 2020 at 23:32, André Przywara <andre.przywara@arm.com> wrote: > > On 23/04/2020 21:43, Ard Biesheuvel wrote: > > Hi Ard, > > > On Thu, 23 Apr 2020 at 19:55, Ard Biesheuvel <ardb@kernel.org> wrote: > >> > >> On Thu, 23 Apr 2020 at 19:39, Andre Przywara <andre.przywara@arm.com> wrote: > >>> > >>> Hi, > >>> > >>> an update for the CFI flash emulation, addressing Alex' comments and > >>> adding direct mapping support. > >>> The actual code changes to the flash emulation are minimal, mostly this > >>> is about renaming and cleanups. > >>> This versions now adds some patches. 1/5 is a required fix, the last > >>> three patches add mapping support as an extension. See below. > >>> > >>> In addition to a branch with this series[1], I also put a git branch with > >>> all the changes compared to v3[2] as separate patches on the server, please > >>> have a look if you want to verify against a previous review. > >>> > >>> =============== > >>> The EDK II UEFI firmware implementation requires some storage for the EFI > >>> variables, which is typically some flash storage. > >>> Since this is already supported on the EDK II side, and looks like a > >>> generic standard, this series adds a CFI flash emulation to kvmtool. > >>> > >>> Patch 2/5 is the actual emulation code, patch 1/5 is a bug-fix for > >>> registering MMIO devices, which is needed for this device. > >>> Patches 3-5 add support for mapping the flash memory into guest, should > >>> it be in read-array mode. For this to work, patch 3/5 is cherry-picked > >>> from Alex' PCIe reassignable BAR series, to support removing a memslot > >>> mapping. Patch 4/5 adds support for read-only mappings, while patch 5/5 > >>> adds or removes the mapping based on the current state. > >>> I am happy to squash 5/5 into 2/5, if we agree that patch 3/5 should be > >>> merged either separately or the PCIe series is actually merged before > >>> this one. > >>> > >>> This is one missing piece towards a working UEFI boot with kvmtool on > >>> ARM guests, the other is to provide writable PCI BARs, which is WIP. > >>> This series alone already enables UEFI boot, but only with virtio-mmio. > >>> > >> > >> Excellent! Thanks for taking the time to implement the r/o memslot for > >> the flash, it really makes the UEFI firmware much more usable. > >> > >> I will test this as soon as I get a chance, probably tomorrow. > >> > > > > I tested this on a SynQuacer box as a host, using EFI firmware [0] > > built from patches provided by Sami. > > > > I booted the Debian buster installer, completed the installation, and > > could boot into the system. The only glitch was the fact that the > > reboot didn't work, but I suppose we are not preserving the memory the > > contains the firmware image, so there is nothing to reboot into. > > It's even worth, kvmtool does actually not support reset at all: > https://git.kernel.org/pub/scm/linux/kernel/git/will/kvmtool.git/tree/kvm-cpu.c#n220 > > And yeah, the UEFI firmware is loaded at the beginning of RAM, so most > of it is long gone by then. > kvmtool could reload the image and reset the VCPUs, but every device > emulation would need to be reset, for which there is no code yet. > Fair enough. For my use case, it doesn't really matter anyway. > > But > > just restarting kvmtool with the flash image containing the EFI > > variables got me straight into GRUB and the installed OS. > > So, yeah, this is the way to do it ;-) > > > Tested-by: Ard Biesheuvel <ardb@kernel.org> > > Many thanks for that! > > > Thanks again for getting this sorted. > > It was actually easier than I thought (see the last patch). > > 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. > > [0] https://people.linaro.org/~ard.biesheuvel/KVMTOOL_EFI.fd > > Ah, nice, will this stay there for a while? I can't provide binaries, > but wanted others to be able to easily test this. > Sure, I will leave it up until Linaro decides to take down my account.
On Thu, Apr 23, 2020 at 06:38:39PM +0100, Andre Przywara wrote: > an update for the CFI flash emulation, addressing Alex' comments and > adding direct mapping support. > The actual code changes to the flash emulation are minimal, mostly this > is about renaming and cleanups. > This versions now adds some patches. 1/5 is a required fix, the last > three patches add mapping support as an extension. See below. Cheers, this mostly looks good to me. I've left a couple of minor comments, and I'll give Alexandru a chance to have another look, but hopefully we can merge it soon. Will
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@arm.com> 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? or Should EDK-2 add an additional or alternate mapping using a non-device memory type (with all the mismatched attributes consequences)? or Should EDK-2 only touch the flash region using MMIO accessors, and forbid the compiler direct access to that region? So does EDK-2 get away with this because the compiler typically avoids unaligned accesses? Cheers, Andre >>> [0] https://people.linaro.org/~ard.biesheuvel/KVMTOOL_EFI.fd >> >> Ah, nice, will this stay there for a while? I can't provide binaries, >> but wanted others to be able to easily test this. >> > > Sure, I will leave it up until Linaro decides to take down my account. >
On Fri, 24 Apr 2020 at 14:08, André Przywara <andre.przywara@arm.com> 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@arm.com> 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.
On Fri, Apr 24, 2020 at 09:40:51AM +0100, Will Deacon wrote: > On Thu, Apr 23, 2020 at 06:38:39PM +0100, Andre Przywara wrote: > > an update for the CFI flash emulation, addressing Alex' comments and > > adding direct mapping support. > > The actual code changes to the flash emulation are minimal, mostly this > > is about renaming and cleanups. > > This versions now adds some patches. 1/5 is a required fix, the last > > three patches add mapping support as an extension. See below. > > Cheers, this mostly looks good to me. I've left a couple of minor comments, > and I'll give Alexandru a chance to have another look, but hopefully we can > merge it soon. Ok, I pushed this out along with the follow-up patch. Thanks! Will
On Fri, 24 Apr 2020 at 19:03, Will Deacon <will@kernel.org> wrote: > > On Fri, Apr 24, 2020 at 09:40:51AM +0100, Will Deacon wrote: > > On Thu, Apr 23, 2020 at 06:38:39PM +0100, Andre Przywara wrote: > > > an update for the CFI flash emulation, addressing Alex' comments and > > > adding direct mapping support. > > > The actual code changes to the flash emulation are minimal, mostly this > > > is about renaming and cleanups. > > > This versions now adds some patches. 1/5 is a required fix, the last > > > three patches add mapping support as an extension. See below. > > > > Cheers, this mostly looks good to me. I've left a couple of minor comments, > > and I'll give Alexandru a chance to have another look, but hopefully we can > > merge it soon. > > Ok, I pushed this out along with the follow-up patch. > Cheers for that, this is useful stuff. For the record, I did a quick benchmark on RPi4 booting Debian in a VM, and I get the following delays (with GRUB and EFI timeouts both set to 0) 17:04:58.487065 17:04:58.563700 UEFI firmware (version built at 22:13:20 on Apr 23 2020) 17:04:58.853653 Welcome to GRUB! 17:04:58.924606 Booting `Debian GNU/Linux' 17:04:58.927835 Loading Linux 5.5.0-2-arm64 ... 17:04:59.063490 Loading initial ramdisk ... 17:05:01.303560 /dev/vda2: recovering journal 17:05:01.408861 /dev/vda2: clean, 37882/500960 files, 457154/2001920 blocks 17:05:09.646023 Debian GNU/Linux bullseye/sid rpi4vm64 ttyS0 So it takes less than 400 ms from starting kvmtool to entering GRUB when the boot path is set normally. Any other delays you are observing may be due to the fact that no boot path has been configured yet, which is why it attempts PXE boot or other things. Also, note that you can pass the --rng option to kvmtool to get the EFI_RNG_PROTOCOL to be exposed to the EFI stub, for KASLR and for seeding the kernel's RNG.