Re: [PATCH kvmtool] Add emulation for CFI compatible flash memory
On Thu, 6 Feb 2020 14:42:11 +
Andre Przywara wrote:
Hi,
> On Wed, 5 Feb 2020 17:11:57 +
> Will Deacon wrote:
>
> Hi Will,
>
> many thanks for having a look!
>
> > On Wed, Jan 08, 2020 at 06:32:12PM +, Andre Przywara wrote:
> > > From: Raphael Gault
[ ... ]
> > > +/* We only support synchronous page mode read accesses. */
> > > +static void read_flash(struct cfi_flash_device *sfdev,
> > > +u64 addr, u8 *buffer, int len)
> > > +{
> > > + memcpy(buffer, sfdev->flash_memory + addr, len);
> > > +}
> >
> > Hmm, you open-code the memcpy when writing the flash so it's a bit weird
>
> Not sure what you refer to exactly?
> The only open-code access I see is in the MMIO handler when doing the CFI QRY
> *read*, which is a very special MMIO style read access. Every other write
> access (word_program(), buffer_confirm()) is already using memcpy.
> What am I missing here?
Robin pointed out that you probably mean that there is a wrapper around the
memcpy on read_flash, but not on the write operations?
The reason for that is that read and write are two very different operations on
any flash memory: the read side is following proper memory semantics: no side
effects, could be cached, etc. I was briefly tempted to actually map it r/o
into the guest, but the problem is that this semantics only holds when we are
in read mode. In any other mode and whenever we write, the CFI flash is
actually an MMIO mapped device, where each access triggers something and the
access width matters. So that would require frequently changing the memslot,
because we need to trap reads when not in read mode.
Anyway, adding a write wrapper doesn't make sense here, but I can of course
easily replace the read_flash() call with the respective memcpy() line.
Cheers,
Andre
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
Re: [PATCH kvmtool] Add emulation for CFI compatible flash memory
On Wed, 5 Feb 2020 17:11:57 +
Will Deacon wrote:
Hi Will,
many thanks for having a look!
> On Wed, Jan 08, 2020 at 06:32:12PM +, 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).
> >
> > 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 towards booting with UEFI inside ARM guests,
> > the other is to provide writable PCI BARs, which is also WIP.
> >
> > Signed-off-by: Raphael Gault
> > [Andre: rewriting and fixing]
> > Signed-off-by: Andre Przywra
> > ---
> > Makefile | 6 +
> > arm/include/arm-common/kvm-arch.h | 3 +
> > builtin-run.c | 2 +
> > hw/cfi_flash.c| 547 ++
> > include/kvm/kvm-config.h | 1 +
> > 5 files changed, 559 insertions(+)
> > 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..cbc9e7aa 100644
> > --- a/arm/include/arm-common/kvm-arch.h
> > +++ b/arm/include/arm-common/kvm-arch.h
> > @@ -21,6 +21,9 @@
> > #define ARM_GIC_DIST_SIZE 0x1
> > #define ARM_GIC_CPUI_SIZE 0x2
> >
> > +#define ARM_FLASH_MMIO_BASE(32ULL << 20)
>
> Can you just use the hex constant (0x200) here please?
>
> > +#define KVM_FLASH_MMIO_BASEARM_FLASH_MMIO_BASE
> > +
> > #define ARM_IOPORT_SIZE(ARM_MMIO_AREA - ARM_IOPORT_AREA)
> > #define ARM_VIRTIO_MMIO_SIZE (ARM_AXI_AREA - (ARM_MMIO_AREA +
> > ARM_GIC_SIZE))
> > #define ARM_PCI_CFG_SIZE (1ULL << 24)
> > 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", &(cfg)->firmware_filename, "firmware",\
> > "Firmware image to boot in virtual machine"), \
> > + OPT_STRING('F', "flash", &(cfg)->flash_filename, "flash",\
> > + "Flash image to present to virtual machine"), \
> > \
> > OPT_GROUP("Networking options:"), \
> > OPT_CALLBACK_DEFAULT('n', "network", NULL, "network params",\
> > diff --git a/hw/cfi_flash.c b/hw/cfi_flash.c
> > new file mode 100644
> > index ..33cfeefe
> > --- /dev/null
> > +++ b/hw/cfi_flash.c
> > @@ -0,0 +1,547 @@
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#include "kvm/kvm.h"
> > +#include "kvm/kvm-arch.h"
> > +#include "kvm/devices.h"
> > +#include "kvm/fdt.h"
> > +#include "kvm/util.h"
> > +
> > +/* The EDK2 driver hardcodes two 16-bit chips on a 32-bit bus. */
> > +#define CFI_NR_FLASH_CHIPS 2
> > +//#define CFI_NR_FLASH_CHIPS 1
>
> Delete this commented define?
>
> > +/* We always emulate a 32 bit bus width. */
> > +#define CFI_BUS_WIDTH 4
> > +
> > +/* The *effective* size of an erase block (over all chips) */
> > +#define FLASH_BLOCK_SIZE SZ_64K
> > +
> > +#define
Re: [PATCH kvmtool] Add emulation for CFI compatible flash memory
On Wed, Jan 08, 2020 at 06:32:12PM +, 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).
>
> 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 towards booting with UEFI inside ARM guests,
> the other is to provide writable PCI BARs, which is also WIP.
>
> Signed-off-by: Raphael Gault
> [Andre: rewriting and fixing]
> Signed-off-by: Andre Przywra
> ---
> Makefile | 6 +
> arm/include/arm-common/kvm-arch.h | 3 +
> builtin-run.c | 2 +
> hw/cfi_flash.c| 547 ++
> include/kvm/kvm-config.h | 1 +
> 5 files changed, 559 insertions(+)
> 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..cbc9e7aa 100644
> --- a/arm/include/arm-common/kvm-arch.h
> +++ b/arm/include/arm-common/kvm-arch.h
> @@ -21,6 +21,9 @@
> #define ARM_GIC_DIST_SIZE0x1
> #define ARM_GIC_CPUI_SIZE0x2
>
> +#define ARM_FLASH_MMIO_BASE (32ULL << 20)
Can you just use the hex constant (0x200) here please?
> +#define KVM_FLASH_MMIO_BASE ARM_FLASH_MMIO_BASE
> +
> #define ARM_IOPORT_SIZE (ARM_MMIO_AREA - ARM_IOPORT_AREA)
> #define ARM_VIRTIO_MMIO_SIZE (ARM_AXI_AREA - (ARM_MMIO_AREA + ARM_GIC_SIZE))
> #define ARM_PCI_CFG_SIZE (1ULL << 24)
> 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", &(cfg)->firmware_filename, "firmware",\
> "Firmware image to boot in virtual machine"), \
> + OPT_STRING('F', "flash", &(cfg)->flash_filename, "flash",\
> + "Flash image to present to virtual machine"), \
> \
> OPT_GROUP("Networking options:"), \
> OPT_CALLBACK_DEFAULT('n', "network", NULL, "network params",\
> diff --git a/hw/cfi_flash.c b/hw/cfi_flash.c
> new file mode 100644
> index ..33cfeefe
> --- /dev/null
> +++ b/hw/cfi_flash.c
> @@ -0,0 +1,547 @@
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "kvm/kvm.h"
> +#include "kvm/kvm-arch.h"
> +#include "kvm/devices.h"
> +#include "kvm/fdt.h"
> +#include "kvm/util.h"
> +
> +/* The EDK2 driver hardcodes two 16-bit chips on a 32-bit bus. */
> +#define CFI_NR_FLASH_CHIPS 2
> +//#define CFI_NR_FLASH_CHIPS 1
Delete this commented define?
> +/* We always emulate a 32 bit bus width. */
> +#define CFI_BUS_WIDTH4
> +
> +/* The *effective* size of an erase block (over all chips) */
> +#define FLASH_BLOCK_SIZE SZ_64K
> +
> +#define PROGRAM_BUFF_SIZE_BITS 7
> +#define PROGRAM_BUFF_SIZE(1U << PROGRAM_BUFF_SIZE_BITS)
> +
> +/* CFI commands */
> +#define CFI_CMD_LOCK_BLOCK 0x01
> +#define CFI_CMD_ALTERNATE_WORD_PROGRAM_SETUP 0x10
> +#define CFI_CMD_BLOCK_ERASE_SETUP0x20
> +#define
