Re: [PATCH kvmtool v3] Add emulation for CFI compatible flash memory
On 07/04/2020 16:15, Alexandru Elisei wrote: > Hi, > > I've tested this patch by running badblocks and fio on a flash device inside a > guest, everything worked as expected. > > I've also looked at the flowcharts for device operation from Intel Application > Note 646, pages 12-21, and they seem implemented correctly. > > A few minor issues below. ^^^ Slight understatement ;-) > > On 2/21/20 4:55 PM, Andre Przywara wrote: >> From: Raphael Gault >> >> 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, we add a CFI flash >> emulation to kvmtool. >> This is backed by a file, specified via the --flash or -F command line >> option. Any flash writes done by the guest will immediately be reflected >> into this file (kvmtool mmap's the file). >> The flash will be limited to the nearest power-of-2 size, so only the >> first 2 MB of a 3 MB file will be used. >> >> This implements a CFI flash using the "Intel/Sharp extended command >> set", as specified in: >> - JEDEC JESD68.01 >> - JEDEC JEP137B >> - Intel Application Note 646 >> Some gaps in those specs have been filled by looking at real devices and >> other implementations (QEMU, Linux kernel driver). >> >> At the moment this relies on DT to advertise the base address of the >> flash memory (mapped into the MMIO address space) and is only enabled >> for ARM/ARM64. The emulation itself is architecture agnostic, though. >> >> This is one missing piece toward a working UEFI boot with kvmtool on >> ARM guests, the other is to provide writable PCI BARs, which is WIP. >> >> Signed-off-by: Raphael Gault >> [Andre: rewriting and fixing] >> Signed-off-by: Andre Przywra >> --- >> Hi, >> >> an update fixing Alexandru's review comments (many thanks for those!) >> The biggest change code-wise is the split of the MMIO handler into three >> different functions. Another significant change is the rounding *down* of >> the present flash file size to the nearest power-of-two, to match flash >> hardware chips and Linux' expectations. >> >> Cheers, >> Andre >> >> 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. >> >> Makefile | 6 + >> arm/include/arm-common/kvm-arch.h | 8 +- >> builtin-run.c | 2 + >> hw/cfi_flash.c| 576 ++ >> include/kvm/kvm-config.h | 1 + >> include/kvm/util.h| 8 + >> 6 files changed, 599 insertions(+), 2 deletions(-) >> create mode 100644 hw/cfi_flash.c >> >> diff --git a/Makefile b/Makefile >> index 3862112c..7ed6fb5e 100644 >> --- a/Makefile >> +++ b/Makefile >> @@ -170,6 +170,7 @@ ifeq ($(ARCH), arm) >> CFLAGS += -march=armv7-a >> >> ARCH_WANT_LIBFDT := y >> +ARCH_HAS_FLASH_MEM := y >> endif >> >> # ARM64 >> @@ -182,6 +183,7 @@ ifeq ($(ARCH), arm64) >> ARCH_INCLUDE+= -Iarm/aarch64/include >> >> ARCH_WANT_LIBFDT := y >> +ARCH_HAS_FLASH_MEM := y >> endif >> >> ifeq ($(ARCH),mips) >> @@ -261,6 +263,10 @@ ifeq (y,$(ARCH_HAS_FRAMEBUFFER)) >> endif >> endif >> >> +ifeq (y,$(ARCH_HAS_FLASH_MEM)) >> +OBJS+= hw/cfi_flash.o >> +endif >> + >> ifeq ($(call try-build,$(SOURCE_ZLIB),$(CFLAGS),$(LDFLAGS) -lz),y) >> CFLAGS_DYNOPT += -DCONFIG_HAS_ZLIB >> LIBS_DYNOPT += -lz >> diff --git a/arm/include/arm-common/kvm-arch.h >> b/arm/include/arm-common/kvm-arch.h >> index b9d486d5..d84e50cd 100644 >> --- a/arm/include/arm-common/kvm-arch.h >> +++ b/arm/include/arm-common/kvm-arch.h >> @@ -8,7 +8,8 @@ >> #include "arm-common/gic.h" >> >> #define ARM_IOPORT_AREA _AC(0x, UL) >> -#define ARM_MMIO_AREA _AC(0x0001, UL) >> +#define ARM_FLASH_AREA _AC(0x0200, UL) >> +#define ARM_MMIO_AREA _AC(0x0300, UL) >> #define ARM_AXI_AREA_AC(0x4000, UL) >> #define ARM_MEMORY_AREA _AC(0x8000, UL) >> >> @@ -21,7 +22,10 @@ >> #define
Re: [PATCH kvmtool v3] Add emulation for CFI compatible flash memory
On Wed, 15 Apr 2020 at 17:43, Ard Biesheuvel wrote: > > On Tue, 7 Apr 2020 at 17:15, Alexandru Elisei > wrote: > > > > Hi, > > > > I've tested this patch by running badblocks and fio on a flash device > > inside a > > guest, everything worked as expected. > > > > I've also looked at the flowcharts for device operation from Intel > > Application > > Note 646, pages 12-21, and they seem implemented correctly. > > > > A few minor issues below. > > > > On 2/21/20 4:55 PM, Andre Przywara wrote: > > > From: Raphael Gault > > > > > > 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, we add a CFI flash > > > emulation to kvmtool. > > > This is backed by a file, specified via the --flash or -F command line > > > option. Any flash writes done by the guest will immediately be reflected > > > into this file (kvmtool mmap's the file). > > > The flash will be limited to the nearest power-of-2 size, so only the > > > first 2 MB of a 3 MB file will be used. > > > > > > This implements a CFI flash using the "Intel/Sharp extended command > > > set", as specified in: > > > - JEDEC JESD68.01 > > > - JEDEC JEP137B > > > - Intel Application Note 646 > > > Some gaps in those specs have been filled by looking at real devices and > > > other implementations (QEMU, Linux kernel driver). > > > > > > At the moment this relies on DT to advertise the base address of the > > > flash memory (mapped into the MMIO address space) and is only enabled > > > for ARM/ARM64. The emulation itself is architecture agnostic, though. > > > > > > This is one missing piece toward a working UEFI boot with kvmtool on > > > ARM guests, the other is to provide writable PCI BARs, which is WIP. > > > > > I have given this a spin with UEFI built for kvmtool, and it appears > to be working correctly. However, I noticed that it is intolerably > slow, which seems to be caused by the fact that both array mode and > command mode (or whatever it is called in the CFI spec) are fully > emulated, whereas in the QEMU implementation (for instance), the > region is actually exposed to the guest using a read-only KVM memslot > in array mode, and so the read accesses are made natively. > > It is also causing problems in the UEFI implementation, as we can no > longer use unaligned accesses to read from the region, which is > something the code currently relies on (and which works fine on actual > hardware as long as you use normal non-cacheable mappings) > Actually, the issue is not alignment. The issue is with instructions with multiple outputs, which means you cannot do an ordinary memcpy() from the NOR region using ldp instructions, aligned or not. ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH kvmtool v3] Add emulation for CFI compatible flash memory
On Tue, 7 Apr 2020 at 17:15, Alexandru Elisei wrote: > > Hi, > > I've tested this patch by running badblocks and fio on a flash device inside a > guest, everything worked as expected. > > I've also looked at the flowcharts for device operation from Intel Application > Note 646, pages 12-21, and they seem implemented correctly. > > A few minor issues below. > > On 2/21/20 4:55 PM, Andre Przywara wrote: > > From: Raphael Gault > > > > 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, we add a CFI flash > > emulation to kvmtool. > > This is backed by a file, specified via the --flash or -F command line > > option. Any flash writes done by the guest will immediately be reflected > > into this file (kvmtool mmap's the file). > > The flash will be limited to the nearest power-of-2 size, so only the > > first 2 MB of a 3 MB file will be used. > > > > This implements a CFI flash using the "Intel/Sharp extended command > > set", as specified in: > > - JEDEC JESD68.01 > > - JEDEC JEP137B > > - Intel Application Note 646 > > Some gaps in those specs have been filled by looking at real devices and > > other implementations (QEMU, Linux kernel driver). > > > > At the moment this relies on DT to advertise the base address of the > > flash memory (mapped into the MMIO address space) and is only enabled > > for ARM/ARM64. The emulation itself is architecture agnostic, though. > > > > This is one missing piece toward a working UEFI boot with kvmtool on > > ARM guests, the other is to provide writable PCI BARs, which is WIP. > > I have given this a spin with UEFI built for kvmtool, and it appears to be working correctly. However, I noticed that it is intolerably slow, which seems to be caused by the fact that both array mode and command mode (or whatever it is called in the CFI spec) are fully emulated, whereas in the QEMU implementation (for instance), the region is actually exposed to the guest using a read-only KVM memslot in array mode, and so the read accesses are made natively. It is also causing problems in the UEFI implementation, as we can no longer use unaligned accesses to read from the region, which is something the code currently relies on (and which works fine on actual hardware as long as you use normal non-cacheable mappings) Are there any plans to implement this as well? I am aware that this is a big ask, but for the general utility of this feature, I think it is rather important. -- Ard. > > Signed-off-by: Raphael Gault > > [Andre: rewriting and fixing] > > Signed-off-by: Andre Przywra > > --- > > Hi, > > > > an update fixing Alexandru's review comments (many thanks for those!) > > The biggest change code-wise is the split of the MMIO handler into three > > different functions. Another significant change is the rounding *down* of > > the present flash file size to the nearest power-of-two, to match flash > > hardware chips and Linux' expectations. > > > > Cheers, > > Andre > > > > 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. > > > > Makefile | 6 + > > arm/include/arm-common/kvm-arch.h | 8 +- > > builtin-run.c | 2 + > > hw/cfi_flash.c| 576 ++ > > include/kvm/kvm-config.h | 1 + > > include/kvm/util.h| 8 + > > 6 files changed, 599 insertions(+), 2 deletions(-) > > create mode 100644 hw/cfi_flash.c > > > > diff --git a/Makefile b/Makefile > > index 3862112c..7ed6fb5e 100644 > > --- a/Makefile > > +++ b/Makefile > > @@ -170,6 +170,7 @@ ifeq ($(ARCH), arm) > > CFLAGS += -march=armv7-a > > > > ARCH_WANT_LIBFDT := y > > + ARCH_HAS_FLASH_MEM := y > > endif > > > > # ARM64 > > @@ -182,6 +183,7 @@ ifeq ($(ARCH), arm64) > > ARCH_INCLUDE+= -Iarm/aarch64/include > > > > ARCH_WANT_LIBFDT := y > > + ARCH_HAS_FLASH_MEM := y > > endif > > > > ifeq ($(ARCH),mips) > > @@ -261,6 +263,10 @@ ifeq (y,$(ARCH_HAS_FRAMEBUFFER)) > >
Re: [PATCH kvmtool v3] Add emulation for CFI compatible flash memory
On Wed, 15 Apr 2020 at 18:11, André Przywara wrote:
>
> On 15/04/2020 16:55, Ard Biesheuvel wrote:
> > On Wed, 15 Apr 2020 at 17:43, Ard Biesheuvel wrote:
> >>
> >> On Tue, 7 Apr 2020 at 17:15, Alexandru Elisei
> >> wrote:
> >>>
> >>> Hi,
> >>>
> >>> I've tested this patch by running badblocks and fio on a flash device
> >>> inside a
> >>> guest, everything worked as expected.
> >>>
> >>> I've also looked at the flowcharts for device operation from Intel
> >>> Application
> >>> Note 646, pages 12-21, and they seem implemented correctly.
> >>>
> >>> A few minor issues below.
> >>>
> >>> On 2/21/20 4:55 PM, Andre Przywara wrote:
> From: Raphael Gault
>
> 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, we add a CFI flash
> emulation to kvmtool.
> This is backed by a file, specified via the --flash or -F command line
> option. Any flash writes done by the guest will immediately be reflected
> into this file (kvmtool mmap's the file).
> The flash will be limited to the nearest power-of-2 size, so only the
> first 2 MB of a 3 MB file will be used.
>
> This implements a CFI flash using the "Intel/Sharp extended command
> set", as specified in:
> - JEDEC JESD68.01
> - JEDEC JEP137B
> - Intel Application Note 646
> Some gaps in those specs have been filled by looking at real devices and
> other implementations (QEMU, Linux kernel driver).
>
> At the moment this relies on DT to advertise the base address of the
> flash memory (mapped into the MMIO address space) and is only enabled
> for ARM/ARM64. The emulation itself is architecture agnostic, though.
>
> This is one missing piece toward a working UEFI boot with kvmtool on
> ARM guests, the other is to provide writable PCI BARs, which is WIP.
>
> >>
> >> I have given this a spin with UEFI built for kvmtool, and it appears
> >> to be working correctly. However, I noticed that it is intolerably
> >> slow, which seems to be caused by the fact that both array mode and
> >> command mode (or whatever it is called in the CFI spec) are fully
> >> emulated, whereas in the QEMU implementation (for instance), the
> >> region is actually exposed to the guest using a read-only KVM memslot
> >> in array mode, and so the read accesses are made natively.
> >>
> >> It is also causing problems in the UEFI implementation, as we can no
> >> longer use unaligned accesses to read from the region, which is
> >> something the code currently relies on (and which works fine on actual
> >> hardware as long as you use normal non-cacheable mappings)
> >>
> >
> > Actually, the issue is not alignment. The issue is with instructions
> > with multiple outputs, which means you cannot do an ordinary memcpy()
> > from the NOR region using ldp instructions, aligned or not.
>
> Yes, we traced that down to an "ldrb with post-inc", in the memcpy code.
> My suggestion was to provide a version of memcpy_{from,to}_io(), as
> Linux does, which only uses MMIO accessors to avoid "fancy" instructions.
>
That is possible, and the impact on the code is manageable, given the
modular nature of EDK2.
> Back at this point I was challenging the idea of accessing a flash
> device with a normal memory mapping, because of it failing when being in
> some query mode. Do you know of any best practices for flash mappings?
> Are two mappings common?
>
In the QEMU port of EDK2, we use normal non-cacheable for the first
flash device, which contains the executable image, and is not
updatable by the guest. The second flash bank is used for the variable
store, and is actually mapped as a device all the time.
Another thing I just realized is that you cannot fetch instructions
from an emulated flash device either, so to execute from NOR flash,
you will need a true memory mapping as well.
So in summary, I think the mode switch is needed to be generally
useful, even if the current approach is sufficient for (slow)
read/write using special memory accessors.
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH kvmtool v3] Add emulation for CFI compatible flash memory
On Wed, 15 Apr 2020 at 18:36, André Przywara wrote:
>
> On 15/04/2020 17:20, Ard Biesheuvel wrote:
> > On Wed, 15 Apr 2020 at 18:11, André Przywara wrote:
> >>
> >> On 15/04/2020 16:55, Ard Biesheuvel wrote:
> >>> On Wed, 15 Apr 2020 at 17:43, Ard Biesheuvel wrote:
>
> On Tue, 7 Apr 2020 at 17:15, Alexandru Elisei
> wrote:
> >
> > Hi,
> >
> > I've tested this patch by running badblocks and fio on a flash device
> > inside a
> > guest, everything worked as expected.
> >
> > I've also looked at the flowcharts for device operation from Intel
> > Application
> > Note 646, pages 12-21, and they seem implemented correctly.
> >
> > A few minor issues below.
> >
> > On 2/21/20 4:55 PM, Andre Przywara wrote:
> >> From: Raphael Gault
> >>
> >> 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, we add a CFI flash
> >> emulation to kvmtool.
> >> This is backed by a file, specified via the --flash or -F command line
> >> option. Any flash writes done by the guest will immediately be
> >> reflected
> >> into this file (kvmtool mmap's the file).
> >> The flash will be limited to the nearest power-of-2 size, so only the
> >> first 2 MB of a 3 MB file will be used.
> >>
> >> This implements a CFI flash using the "Intel/Sharp extended command
> >> set", as specified in:
> >> - JEDEC JESD68.01
> >> - JEDEC JEP137B
> >> - Intel Application Note 646
> >> Some gaps in those specs have been filled by looking at real devices
> >> and
> >> other implementations (QEMU, Linux kernel driver).
> >>
> >> At the moment this relies on DT to advertise the base address of the
> >> flash memory (mapped into the MMIO address space) and is only enabled
> >> for ARM/ARM64. The emulation itself is architecture agnostic, though.
> >>
> >> This is one missing piece toward a working UEFI boot with kvmtool on
> >> ARM guests, the other is to provide writable PCI BARs, which is WIP.
> >>
>
> I have given this a spin with UEFI built for kvmtool, and it appears
> to be working correctly. However, I noticed that it is intolerably
> slow, which seems to be caused by the fact that both array mode and
> command mode (or whatever it is called in the CFI spec) are fully
> emulated, whereas in the QEMU implementation (for instance), the
> region is actually exposed to the guest using a read-only KVM memslot
> in array mode, and so the read accesses are made natively.
>
> It is also causing problems in the UEFI implementation, as we can no
> longer use unaligned accesses to read from the region, which is
> something the code currently relies on (and which works fine on actual
> hardware as long as you use normal non-cacheable mappings)
>
> >>>
> >>> Actually, the issue is not alignment. The issue is with instructions
> >>> with multiple outputs, which means you cannot do an ordinary memcpy()
> >>> from the NOR region using ldp instructions, aligned or not.
> >>
> >> Yes, we traced that down to an "ldrb with post-inc", in the memcpy code.
> >> My suggestion was to provide a version of memcpy_{from,to}_io(), as
> >> Linux does, which only uses MMIO accessors to avoid "fancy" instructions.
> >>
> >
> > That is possible, and the impact on the code is manageable, given the
> > modular nature of EDK2.
> >
> >> Back at this point I was challenging the idea of accessing a flash
> >> device with a normal memory mapping, because of it failing when being in
> >> some query mode. Do you know of any best practices for flash mappings?
> >> Are two mappings common?
> >>
> >
> > In the QEMU port of EDK2, we use normal non-cacheable for the first
> > flash device, which contains the executable image, and is not
> > updatable by the guest. The second flash bank is used for the variable
> > store, and is actually mapped as a device all the time.
> >
> > Another thing I just realized is that you cannot fetch instructions
> > from an emulated flash device either, so to execute from NOR flash,
> > you will need a true memory mapping as well.
>
> Wait, did you put the whole of EDK-2 image in the flash?
No, my point is that you cannot actually do that, since I don't think
you can fetch instructions using MMIO emulation.
> My assumption
> (and testing) was to use
>
> $ lkvm run -f KVMTOOL_EFI.fd --flash just_the_variables.img
>
> Hence my ignorance about performance, because it would just be a few
> bytes written/read. -f loads the firmware image into guest RAM.
>
No, the performance impact is due to the numerous variable accesses
done by UEFI during boot.
> > So in summary, I think the mode switch is needed to be generally
> > useful, even if the current approach is
Re: [PATCH kvmtool v3] Add emulation for CFI compatible flash memory
On 15/04/2020 17:20, Ard Biesheuvel wrote:
> On Wed, 15 Apr 2020 at 18:11, André Przywara wrote:
>>
>> On 15/04/2020 16:55, Ard Biesheuvel wrote:
>>> On Wed, 15 Apr 2020 at 17:43, Ard Biesheuvel wrote:
On Tue, 7 Apr 2020 at 17:15, Alexandru Elisei
wrote:
>
> Hi,
>
> I've tested this patch by running badblocks and fio on a flash device
> inside a
> guest, everything worked as expected.
>
> I've also looked at the flowcharts for device operation from Intel
> Application
> Note 646, pages 12-21, and they seem implemented correctly.
>
> A few minor issues below.
>
> On 2/21/20 4:55 PM, Andre Przywara wrote:
>> From: Raphael Gault
>>
>> 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, we add a CFI flash
>> emulation to kvmtool.
>> This is backed by a file, specified via the --flash or -F command line
>> option. Any flash writes done by the guest will immediately be reflected
>> into this file (kvmtool mmap's the file).
>> The flash will be limited to the nearest power-of-2 size, so only the
>> first 2 MB of a 3 MB file will be used.
>>
>> This implements a CFI flash using the "Intel/Sharp extended command
>> set", as specified in:
>> - JEDEC JESD68.01
>> - JEDEC JEP137B
>> - Intel Application Note 646
>> Some gaps in those specs have been filled by looking at real devices and
>> other implementations (QEMU, Linux kernel driver).
>>
>> At the moment this relies on DT to advertise the base address of the
>> flash memory (mapped into the MMIO address space) and is only enabled
>> for ARM/ARM64. The emulation itself is architecture agnostic, though.
>>
>> This is one missing piece toward a working UEFI boot with kvmtool on
>> ARM guests, the other is to provide writable PCI BARs, which is WIP.
>>
I have given this a spin with UEFI built for kvmtool, and it appears
to be working correctly. However, I noticed that it is intolerably
slow, which seems to be caused by the fact that both array mode and
command mode (or whatever it is called in the CFI spec) are fully
emulated, whereas in the QEMU implementation (for instance), the
region is actually exposed to the guest using a read-only KVM memslot
in array mode, and so the read accesses are made natively.
It is also causing problems in the UEFI implementation, as we can no
longer use unaligned accesses to read from the region, which is
something the code currently relies on (and which works fine on actual
hardware as long as you use normal non-cacheable mappings)
>>>
>>> Actually, the issue is not alignment. The issue is with instructions
>>> with multiple outputs, which means you cannot do an ordinary memcpy()
>>> from the NOR region using ldp instructions, aligned or not.
>>
>> Yes, we traced that down to an "ldrb with post-inc", in the memcpy code.
>> My suggestion was to provide a version of memcpy_{from,to}_io(), as
>> Linux does, which only uses MMIO accessors to avoid "fancy" instructions.
>>
>
> That is possible, and the impact on the code is manageable, given the
> modular nature of EDK2.
>
>> Back at this point I was challenging the idea of accessing a flash
>> device with a normal memory mapping, because of it failing when being in
>> some query mode. Do you know of any best practices for flash mappings?
>> Are two mappings common?
>>
>
> In the QEMU port of EDK2, we use normal non-cacheable for the first
> flash device, which contains the executable image, and is not
> updatable by the guest. The second flash bank is used for the variable
> store, and is actually mapped as a device all the time.
>
> Another thing I just realized is that you cannot fetch instructions
> from an emulated flash device either, so to execute from NOR flash,
> you will need a true memory mapping as well.
Wait, did you put the whole of EDK-2 image in the flash? My assumption
(and testing) was to use
$ lkvm run -f KVMTOOL_EFI.fd --flash just_the_variables.img
Hence my ignorance about performance, because it would just be a few
bytes written/read. -f loads the firmware image into guest RAM.
> So in summary, I think the mode switch is needed to be generally
> useful, even if the current approach is sufficient for (slow)
> read/write using special memory accessors.
Well,in hindsight I regret pursuing this whole flash emulation approach
in kvmtool in the first place. Just some magic "this memory region is
persistent" (mmapping a file and presenting as a memslot) would be
*much* easier on the kvmtool side. It just seems that there wasn't any
good DT binding or existing device class for this (to my surprise), or
at least not one without issues. And then EDK-2 had this CFI flash
support already, so
Re: [PATCH kvmtool v3] Add emulation for CFI compatible flash memory
On 15/04/2020 16:55, Ard Biesheuvel wrote:
> On Wed, 15 Apr 2020 at 17:43, Ard Biesheuvel wrote:
>>
>> On Tue, 7 Apr 2020 at 17:15, Alexandru Elisei
>> wrote:
>>>
>>> Hi,
>>>
>>> I've tested this patch by running badblocks and fio on a flash device
>>> inside a
>>> guest, everything worked as expected.
>>>
>>> I've also looked at the flowcharts for device operation from Intel
>>> Application
>>> Note 646, pages 12-21, and they seem implemented correctly.
>>>
>>> A few minor issues below.
>>>
>>> On 2/21/20 4:55 PM, Andre Przywara wrote:
From: Raphael Gault
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, we add a CFI flash
emulation to kvmtool.
This is backed by a file, specified via the --flash or -F command line
option. Any flash writes done by the guest will immediately be reflected
into this file (kvmtool mmap's the file).
The flash will be limited to the nearest power-of-2 size, so only the
first 2 MB of a 3 MB file will be used.
This implements a CFI flash using the "Intel/Sharp extended command
set", as specified in:
- JEDEC JESD68.01
- JEDEC JEP137B
- Intel Application Note 646
Some gaps in those specs have been filled by looking at real devices and
other implementations (QEMU, Linux kernel driver).
At the moment this relies on DT to advertise the base address of the
flash memory (mapped into the MMIO address space) and is only enabled
for ARM/ARM64. The emulation itself is architecture agnostic, though.
This is one missing piece toward a working UEFI boot with kvmtool on
ARM guests, the other is to provide writable PCI BARs, which is WIP.
>>
>> I have given this a spin with UEFI built for kvmtool, and it appears
>> to be working correctly. However, I noticed that it is intolerably
>> slow, which seems to be caused by the fact that both array mode and
>> command mode (or whatever it is called in the CFI spec) are fully
>> emulated, whereas in the QEMU implementation (for instance), the
>> region is actually exposed to the guest using a read-only KVM memslot
>> in array mode, and so the read accesses are made natively.
>>
>> It is also causing problems in the UEFI implementation, as we can no
>> longer use unaligned accesses to read from the region, which is
>> something the code currently relies on (and which works fine on actual
>> hardware as long as you use normal non-cacheable mappings)
>>
>
> Actually, the issue is not alignment. The issue is with instructions
> with multiple outputs, which means you cannot do an ordinary memcpy()
> from the NOR region using ldp instructions, aligned or not.
Yes, we traced that down to an "ldrb with post-inc", in the memcpy code.
My suggestion was to provide a version of memcpy_{from,to}_io(), as
Linux does, which only uses MMIO accessors to avoid "fancy" instructions.
Back at this point I was challenging the idea of accessing a flash
device with a normal memory mapping, because of it failing when being in
some query mode. Do you know of any best practices for flash mappings?
Are two mappings common?
Cheers,
Andre
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH kvmtool v3] Add emulation for CFI compatible flash memory
On 15/04/2020 16:43, Ard Biesheuvel wrote: Hi Ard, > On Tue, 7 Apr 2020 at 17:15, Alexandru Elisei > wrote: >> >> Hi, >> >> I've tested this patch by running badblocks and fio on a flash device inside >> a >> guest, everything worked as expected. >> >> I've also looked at the flowcharts for device operation from Intel >> Application >> Note 646, pages 12-21, and they seem implemented correctly. >> >> A few minor issues below. >> >> On 2/21/20 4:55 PM, Andre Przywara wrote: >>> From: Raphael Gault >>> >>> 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, we add a CFI flash >>> emulation to kvmtool. >>> This is backed by a file, specified via the --flash or -F command line >>> option. Any flash writes done by the guest will immediately be reflected >>> into this file (kvmtool mmap's the file). >>> The flash will be limited to the nearest power-of-2 size, so only the >>> first 2 MB of a 3 MB file will be used. >>> >>> This implements a CFI flash using the "Intel/Sharp extended command >>> set", as specified in: >>> - JEDEC JESD68.01 >>> - JEDEC JEP137B >>> - Intel Application Note 646 >>> Some gaps in those specs have been filled by looking at real devices and >>> other implementations (QEMU, Linux kernel driver). >>> >>> At the moment this relies on DT to advertise the base address of the >>> flash memory (mapped into the MMIO address space) and is only enabled >>> for ARM/ARM64. The emulation itself is architecture agnostic, though. >>> >>> This is one missing piece toward a working UEFI boot with kvmtool on >>> ARM guests, the other is to provide writable PCI BARs, which is WIP. >>> > > I have given this a spin with UEFI built for kvmtool, and it appears > to be working correctly. However, I noticed that it is intolerably > slow, which seems to be caused by the fact that both array mode and > command mode (or whatever it is called in the CFI spec) are fully > emulated, whereas in the QEMU implementation (for instance), the > region is actually exposed to the guest using a read-only KVM memslot > in array mode, and so the read accesses are made natively. Yes, I have been thinking about this, but didn't implement it this way for the following reasons: 1) The use case here is pure UEFI firmware load, which should not be too much affected by performance. At least this what I was thinking so far. Your "intolerably slow" make me wonder if this assumption is not true. Can you elaborate on that? Do you have any numbers? Is that due to the trapping or something else? 2) As you mentioned, we need to switch between trapping and mapping, upon the guest switching between array mode and command mode. So that just adds complexity. Given 1) it didn't seem worth to do. > It is also causing problems in the UEFI implementation, as we can no > longer use unaligned accesses to read from the region, which is > something the code currently relies on (and which works fine on actual > hardware as long as you use normal non-cacheable mappings) So this is something I was discussing with Sami about already: It seems to me that a parallel memory mapped flash chip is actually a device, just with the twist of behaving with normal (ROM) memory semantics under certain circumstances. So for write accesses and read access in any of the query modes we would definitely need to use device memory and MMIO accessors, otherwise the compiler could mess up the carefully crafted access semantics. So does EDK-2 use separate mappings for both cases? Does it make sure to not access the array when being in command mode? I couldn't find any trace of two mappings in the Linux driver, IIRC. > Are there any plans to implement this as well? I am aware that this is > a big ask, but for the general utility of this feature, I think it is > rather important. I wasn't aware that this has a significant impact, so avoided the added complexity. I doesn't sound like a big deal, though, so I might have a look at it. Cheers, Andre ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH kvmtool v3] Add emulation for CFI compatible flash memory
Hi, I've tested this patch by running badblocks and fio on a flash device inside a guest, everything worked as expected. I've also looked at the flowcharts for device operation from Intel Application Note 646, pages 12-21, and they seem implemented correctly. A few minor issues below. On 2/21/20 4:55 PM, Andre Przywara wrote: > From: Raphael Gault > > 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, we add a CFI flash > emulation to kvmtool. > This is backed by a file, specified via the --flash or -F command line > option. Any flash writes done by the guest will immediately be reflected > into this file (kvmtool mmap's the file). > The flash will be limited to the nearest power-of-2 size, so only the > first 2 MB of a 3 MB file will be used. > > This implements a CFI flash using the "Intel/Sharp extended command > set", as specified in: > - JEDEC JESD68.01 > - JEDEC JEP137B > - Intel Application Note 646 > Some gaps in those specs have been filled by looking at real devices and > other implementations (QEMU, Linux kernel driver). > > At the moment this relies on DT to advertise the base address of the > flash memory (mapped into the MMIO address space) and is only enabled > for ARM/ARM64. The emulation itself is architecture agnostic, though. > > This is one missing piece toward a working UEFI boot with kvmtool on > ARM guests, the other is to provide writable PCI BARs, which is WIP. > > Signed-off-by: Raphael Gault > [Andre: rewriting and fixing] > Signed-off-by: Andre Przywra > --- > Hi, > > an update fixing Alexandru's review comments (many thanks for those!) > The biggest change code-wise is the split of the MMIO handler into three > different functions. Another significant change is the rounding *down* of > the present flash file size to the nearest power-of-two, to match flash > hardware chips and Linux' expectations. > > Cheers, > Andre > > 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. > > Makefile | 6 + > arm/include/arm-common/kvm-arch.h | 8 +- > builtin-run.c | 2 + > hw/cfi_flash.c| 576 ++ > include/kvm/kvm-config.h | 1 + > include/kvm/util.h| 8 + > 6 files changed, 599 insertions(+), 2 deletions(-) > create mode 100644 hw/cfi_flash.c > > diff --git a/Makefile b/Makefile > index 3862112c..7ed6fb5e 100644 > --- a/Makefile > +++ b/Makefile > @@ -170,6 +170,7 @@ ifeq ($(ARCH), arm) > CFLAGS += -march=armv7-a > > ARCH_WANT_LIBFDT := y > + ARCH_HAS_FLASH_MEM := y > endif > > # ARM64 > @@ -182,6 +183,7 @@ ifeq ($(ARCH), arm64) > ARCH_INCLUDE+= -Iarm/aarch64/include > > ARCH_WANT_LIBFDT := y > + ARCH_HAS_FLASH_MEM := y > endif > > ifeq ($(ARCH),mips) > @@ -261,6 +263,10 @@ ifeq (y,$(ARCH_HAS_FRAMEBUFFER)) > endif > endif > > +ifeq (y,$(ARCH_HAS_FLASH_MEM)) > + OBJS+= hw/cfi_flash.o > +endif > + > ifeq ($(call try-build,$(SOURCE_ZLIB),$(CFLAGS),$(LDFLAGS) -lz),y) > CFLAGS_DYNOPT += -DCONFIG_HAS_ZLIB > LIBS_DYNOPT += -lz > diff --git a/arm/include/arm-common/kvm-arch.h > b/arm/include/arm-common/kvm-arch.h > index b9d486d5..d84e50cd 100644 > --- a/arm/include/arm-common/kvm-arch.h > +++ b/arm/include/arm-common/kvm-arch.h > @@ -8,7 +8,8 @@ > #include "arm-common/gic.h" > > #define ARM_IOPORT_AREA _AC(0x, UL) > -#define ARM_MMIO_AREA_AC(0x0001, UL) > +#define ARM_FLASH_AREA _AC(0x0200, UL) > +#define ARM_MMIO_AREA_AC(0x0300, UL) > #define ARM_AXI_AREA _AC(0x4000, UL) > #define ARM_MEMORY_AREA _AC(0x8000, UL) > > @@ -21,7 +22,10 @@ > #define ARM_GIC_DIST_SIZE0x1 > #define ARM_GIC_CPUI_SIZE0x2 > > -#define ARM_IOPORT_SIZE (ARM_MMIO_AREA - ARM_IOPORT_AREA) > +#define KVM_FLASH_MMIO_BASE ARM_FLASH_AREA > +#define
Re: [PATCH kvmtool v3] Add emulation for CFI compatible flash memory
Hi Will, On 3/18/20 9:58 PM, Will Deacon wrote: > On Fri, Feb 21, 2020 at 04:55:32PM +, Andre Przywara wrote: >> From: Raphael Gault >> >> 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, we add a CFI flash >> emulation to kvmtool. >> This is backed by a file, specified via the --flash or -F command line >> option. Any flash writes done by the guest will immediately be reflected >> into this file (kvmtool mmap's the file). >> The flash will be limited to the nearest power-of-2 size, so only the >> first 2 MB of a 3 MB file will be used. >> >> This implements a CFI flash using the "Intel/Sharp extended command >> set", as specified in: >> - JEDEC JESD68.01 >> - JEDEC JEP137B >> - Intel Application Note 646 >> Some gaps in those specs have been filled by looking at real devices and >> other implementations (QEMU, Linux kernel driver). >> >> At the moment this relies on DT to advertise the base address of the >> flash memory (mapped into the MMIO address space) and is only enabled >> for ARM/ARM64. The emulation itself is architecture agnostic, though. >> >> This is one missing piece toward a working UEFI boot with kvmtool on >> ARM guests, the other is to provide writable PCI BARs, which is WIP. >> >> Signed-off-by: Raphael Gault >> [Andre: rewriting and fixing] >> Signed-off-by: Andre Przywra >> --- >> Hi, >> >> an update fixing Alexandru's review comments (many thanks for those!) >> The biggest change code-wise is the split of the MMIO handler into three >> different functions. Another significant change is the rounding *down* of >> the present flash file size to the nearest power-of-two, to match flash >> hardware chips and Linux' expectations. > Alexandru -- are you happy with this now? I really appreciate taking the time to look at it, but at the moment I'm busy testing v3 of the reassignable BARs and PCIE support [1]. I'll try to send the patches as soon as possible, then I'll review this patch. [1] https://www.spinics.net/lists/kvm/msg204878.html Thanks, Alex > > Will ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH kvmtool v3] Add emulation for CFI compatible flash memory
On Fri, Feb 21, 2020 at 04:55:32PM +, Andre Przywara wrote: > From: Raphael Gault > > 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, we add a CFI flash > emulation to kvmtool. > This is backed by a file, specified via the --flash or -F command line > option. Any flash writes done by the guest will immediately be reflected > into this file (kvmtool mmap's the file). > The flash will be limited to the nearest power-of-2 size, so only the > first 2 MB of a 3 MB file will be used. > > This implements a CFI flash using the "Intel/Sharp extended command > set", as specified in: > - JEDEC JESD68.01 > - JEDEC JEP137B > - Intel Application Note 646 > Some gaps in those specs have been filled by looking at real devices and > other implementations (QEMU, Linux kernel driver). > > At the moment this relies on DT to advertise the base address of the > flash memory (mapped into the MMIO address space) and is only enabled > for ARM/ARM64. The emulation itself is architecture agnostic, though. > > This is one missing piece toward a working UEFI boot with kvmtool on > ARM guests, the other is to provide writable PCI BARs, which is WIP. > > Signed-off-by: Raphael Gault > [Andre: rewriting and fixing] > Signed-off-by: Andre Przywra > --- > Hi, > > an update fixing Alexandru's review comments (many thanks for those!) > The biggest change code-wise is the split of the MMIO handler into three > different functions. Another significant change is the rounding *down* of > the present flash file size to the nearest power-of-two, to match flash > hardware chips and Linux' expectations. Alexandru -- are you happy with this now? Will ___ kvmarm mailing list kvmarm@lists.cs.columbia.edu https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
[PATCH kvmtool v3] Add emulation for CFI compatible flash memory
From: Raphael Gault
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, we add a CFI flash
emulation to kvmtool.
This is backed by a file, specified via the --flash or -F command line
option. Any flash writes done by the guest will immediately be reflected
into this file (kvmtool mmap's the file).
The flash will be limited to the nearest power-of-2 size, so only the
first 2 MB of a 3 MB file will be used.
This implements a CFI flash using the "Intel/Sharp extended command
set", as specified in:
- JEDEC JESD68.01
- JEDEC JEP137B
- Intel Application Note 646
Some gaps in those specs have been filled by looking at real devices and
other implementations (QEMU, Linux kernel driver).
At the moment this relies on DT to advertise the base address of the
flash memory (mapped into the MMIO address space) and is only enabled
for ARM/ARM64. The emulation itself is architecture agnostic, though.
This is one missing piece toward a working UEFI boot with kvmtool on
ARM guests, the other is to provide writable PCI BARs, which is WIP.
Signed-off-by: Raphael Gault
[Andre: rewriting and fixing]
Signed-off-by: Andre Przywra
---
Hi,
an update fixing Alexandru's review comments (many thanks for those!)
The biggest change code-wise is the split of the MMIO handler into three
different functions. Another significant change is the rounding *down* of
the present flash file size to the nearest power-of-two, to match flash
hardware chips and Linux' expectations.
Cheers,
Andre
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.
Makefile | 6 +
arm/include/arm-common/kvm-arch.h | 8 +-
builtin-run.c | 2 +
hw/cfi_flash.c| 576 ++
include/kvm/kvm-config.h | 1 +
include/kvm/util.h| 8 +
6 files changed, 599 insertions(+), 2 deletions(-)
create mode 100644 hw/cfi_flash.c
diff --git a/Makefile b/Makefile
index 3862112c..7ed6fb5e 100644
--- a/Makefile
+++ b/Makefile
@@ -170,6 +170,7 @@ ifeq ($(ARCH), arm)
CFLAGS += -march=armv7-a
ARCH_WANT_LIBFDT := y
+ ARCH_HAS_FLASH_MEM := y
endif
# ARM64
@@ -182,6 +183,7 @@ ifeq ($(ARCH), arm64)
ARCH_INCLUDE+= -Iarm/aarch64/include
ARCH_WANT_LIBFDT := y
+ ARCH_HAS_FLASH_MEM := y
endif
ifeq ($(ARCH),mips)
@@ -261,6 +263,10 @@ ifeq (y,$(ARCH_HAS_FRAMEBUFFER))
endif
endif
+ifeq (y,$(ARCH_HAS_FLASH_MEM))
+ OBJS+= hw/cfi_flash.o
+endif
+
ifeq ($(call try-build,$(SOURCE_ZLIB),$(CFLAGS),$(LDFLAGS) -lz),y)
CFLAGS_DYNOPT += -DCONFIG_HAS_ZLIB
LIBS_DYNOPT += -lz
diff --git a/arm/include/arm-common/kvm-arch.h
b/arm/include/arm-common/kvm-arch.h
index b9d486d5..d84e50cd 100644
--- a/arm/include/arm-common/kvm-arch.h
+++ b/arm/include/arm-common/kvm-arch.h
@@ -8,7 +8,8 @@
#include "arm-common/gic.h"
#define ARM_IOPORT_AREA_AC(0x, UL)
-#define ARM_MMIO_AREA _AC(0x0001, UL)
+#define ARM_FLASH_AREA _AC(0x0200, UL)
+#define ARM_MMIO_AREA _AC(0x0300, UL)
#define ARM_AXI_AREA _AC(0x4000, UL)
#define ARM_MEMORY_AREA_AC(0x8000, UL)
@@ -21,7 +22,10 @@
#define ARM_GIC_DIST_SIZE 0x1
#define ARM_GIC_CPUI_SIZE 0x2
-#define ARM_IOPORT_SIZE(ARM_MMIO_AREA - ARM_IOPORT_AREA)
+#define KVM_FLASH_MMIO_BASEARM_FLASH_AREA
+#define KVM_FLASH_MAX_SIZE (ARM_MMIO_AREA - ARM_FLASH_AREA)
+
+#define ARM_IOPORT_SIZE(1U << 16)
#define ARM_VIRTIO_MMIO_SIZE (ARM_AXI_AREA - (ARM_MMIO_AREA + ARM_GIC_SIZE))
#define ARM_PCI_CFG_SIZE (1ULL << 24)
#define ARM_PCI_MMIO_SIZE (ARM_MEMORY_AREA - \
diff --git a/builtin-run.c b/builtin-run.c
index f8dc6c72..df8c6741 100644
--- a/builtin-run.c
+++ b/builtin-run.c
@@ -138,6 +138,8 @@ void kvm_run_set_wrapper_sandbox(void)
"Kernel command line arguments"), \
OPT_STRING('f', "firmware",
