Re: [PATCH kvmtool v2] Add emulation for CFI compatible flash memory

[Andre Przywara <andre.przywara@arm.com>]

On Fri, 7 Feb 2020 17:34:20 +0000
Alexandru Elisei <alexandru.elisei@arm.com> wrote:

Hi Alex,

many thanks for having a look!

> I'm going to do my best to review your patch :) I'll do it in chunks, because it's
> pretty large, and definitely not trivial.

OK, replying here, and having it mostly fixed already.
Will wait for further replies before a re-post, unless you want to benefit from the split MMIO function, which should make reviewing the state machine easier. Just let me know.

> On 2/7/20 12:19 PM, Andre Przywara wrote:
> > From: Raphael Gault <raphael.gault@arm.com>
> >
> > 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 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 <raphael.gault@arm.com>
> > [Andre: rewriting and fixing]
> > Signed-off-by: Andre Przywra <andre.przywara@arm.com>
> > ---
> > Hi,
> >
> > an update addressing Will's comments. I added coarse grained locking
> > to the MMIO handler, to prevent concurrent vCPU accesses from messing up
> > the internal CFI flash state machine.
> > I also folded the actual flash array read access into the MMIO handler
> > and fixed the other small issues.
> >
> > Cheers,
> > Andre
> >
> >  Makefile                          |   6 +
> >  arm/include/arm-common/kvm-arch.h |   3 +
> >  builtin-run.c                     |   2 +
> >  hw/cfi_flash.c                    | 546 ++++++++++++++++++++++++++++++
> >  include/kvm/kvm-config.h          |   1 +
> >  include/kvm/util.h                |   5 +
> >  6 files changed, 563 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..2bb085f4 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	0x10000
> >  #define ARM_GIC_CPUI_SIZE	0x20000
> >  
> > +#define ARM_FLASH_MMIO_BASE	0x2000000		/* 32 MB */
> > +#define KVM_FLASH_MMIO_BASE	ARM_FLASH_MMIO_BASE  
> 
> Each time I try to read the memory layout for ARM I get a headache. According to
> my calculations, this falls right inside ARM_MMIO_AREA, right? Any particular
> reason for choosing this address? Why not carve its own dedicate area, so we won't
> run the highly unlikely risk that it will be overwritten, since it's in the MMIO
> allocation area?

The EDK2 build I used has the base address fixed at 32MB. So I just used this address here. Sami is working on making this flexible as we speak, but it's not easy due to some EDK-2 design issues.
As an interim measure I would try to describe this using the existing MMIO layout macros, to at least avoid overlaps with virtio-mmio.
I actually might move that address to the beginning for now, as 32MB is currently in the middle of the MMIO area.
QEMU has that hardcoded (both in QEMU and EDK-2) as well, btw.
 
> > +
> >  #define ARM_IOPORT_SIZE		(ARM_MMIO_AREA - ARM_IOPORT_AREA)
> >  #define ARM_VIRTIO_MMIO_SIZE	(ARM_AXI_AREA - (ARM_MMIO_AREA + ARM_GIC_SIZE))  
> 
> That's not correct anymore, because flash memory is in the ARM_MMIO_AREA.

True, I will try to find the right place for this. Somewhat problematic is the differing size, but we could just impose an upper limit on this.

> >  #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 00000000..d7c0e7e8
> > --- /dev/null
> > +++ b/hw/cfi_flash.c
> > @@ -0,0 +1,546 @@
> > +#include <stdbool.h>
> > +#include <stdlib.h>
> > +#include <string.h>
> > +#include <linux/bitops.h>
> > +#include <linux/err.h>
> > +#include <linux/sizes.h>
> > +#include <linux/types.h>
> > +
> > +#include "kvm/kvm.h"
> > +#include "kvm/kvm-arch.h"
> > +#include "kvm/devices.h"
> > +#include "kvm/fdt.h"
> > +#include "kvm/mutex.h"
> > +#include "kvm/util.h"
> > +
> > +/* The EDK2 driver hardcodes two 16-bit chips on a 32-bit bus. */
> > +#define CFI_NR_FLASH_CHIPS			2
> > +
> > +/* 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 PROGRAM_BUFF_SIZE_BITS			7
> > +#define PROGRAM_BUFF_SIZE			(1U << PROGRAM_BUFF_SIZE_BITS)  
> 
> Just making sure this is not an off-by-one error. The buffer size is 2^7 = 128
> words, which makes it 512 bytes, right?

Looks like it ;-)
The reason this is presented in this rather awkward way here is that we need the number of bits to be presented in the CFI query structure later on.
I will add a comment pointing out this is in units of "words" - after double checking that it really is ;-)
 
> > +
> > +/* CFI commands */
> > +#define CFI_CMD_LOCK_BLOCK			0x01
> > +#define CFI_CMD_ALTERNATE_WORD_PROGRAM_SETUP	0x10
> > +#define CFI_CMD_BLOCK_ERASE_SETUP		0x20
> > +#define CFI_CMD_WORD_PROGRAM_SETUP		0x40
> > +#define CFI_CMD_CLEAR_STATUS_REGISTER		0x50
> > +#define CFI_CMD_LOCK_BLOCK_SETUP		0x60
> > +#define CFI_CMD_READ_STATUS_REGISTER		0x70
> > +#define CFI_CMD_READ_JEDEC			0x90
> > +#define CFI_CMD_READ_CFI_QUERY			0x98
> > +#define CFI_CMD_BUFFERED_PROGRAM_CONFIRM	0xd0
> > +#define CFI_CMD_BLOCK_ERASE_CONFIRM		0xd0
> > +#define CFI_CMD_UNLOCK_BLOCK			0xd0
> > +#define CFI_CMD_BUFFERED_PROGRAM_SETUP		0xe8
> > +#define CFI_CMD_READ_ARRAY			0xff
> > +
> > +/*
> > + * CFI query table contents, as far as it is constant.
> > + */
> > +#define CFI_GEOM_OFFSET				0x27
> > +static u8 cfi_query_table[] = {
> > +		/* offset 0x10: CFI query identification string */
> > +	'Q', 'R', 'Y',		/* ID string */
> > +	0x01, 0x00,		/* primary command set: Intel/Sharp extended */
> > +	0x31, 0x00,		/* address of primary extended query table */
> > +	0x00, 0x00,		/* alternative command set: unused */
> > +	0x00, 0x00,		/* address of alternative extended query table*/
> > +		/* offset 0x1b: system interface information */
> > +	0x45,			/* minimum Vcc voltage: 4.5V */
> > +	0x55,			/* maximum Vcc voltage: 5.5V */
> > +	0x00,			/* minimum Vpp voltage: 0.0V (unused) */
> > +	0x00,			/* maximum Vpp voltage: 0.0V *(unused) */
> > +	0x01,			/* timeout for single word program: 2 us */
> > +	0x01,			/* timeout for multi-byte program: 2 us */
> > +	0x01,			/* timeout for block erase: 2 ms */
> > +	0x00,			/* timeout for full chip erase: not supported */
> > +	0x00,			/* max timeout for single word program: 1x */
> > +	0x00,			/* max timeout for mulit-byte program: 1x */
> > +	0x00,			/* max timeout for block erase: 1x */
> > +	0x00,			/* max timeout for chip erase: not supported */
> > +		/* offset 0x27: flash geometry information */
> > +	0x00,			/* size in power-of-2 bytes, filled later */
> > +	0x06, 0x00,		/* interface description: 32 and 16 bits */
> > +	PROGRAM_BUFF_SIZE_BITS + 1 - CFI_NR_FLASH_CHIPS, 0x00,
> > +				/* number of multi-byte writes */
> > +	0x01,			/* one erase block region */
> > +	0x00, 0x00, 0x00, 0x00, /* number and size of erase blocks, filled */
> > +		/* offset 0x31: Intel primary algorithm extended query table */
> > +	'P', 'R', 'I',
> > +	'1', '0',		/* version 1.0 */
> > +	0xa0, 0x00, 0x00, 0x00, /* optional features: instant lock & pm-read */
> > +	0x00,			/* no functions after suspend */
> > +	0x01, 0x00,		/* only lock bit supported */
> > +	0x50,			/* best Vcc value: 5.0V */
> > +	0x00,			/* best Vpp value: 0.0V (unused) */
> > +	0x01,			/* number of protection register fields */
> > +	0x00, 0x00, 0x00, 0x00,	/* protection field 1 description */
> > +};
> > +
> > +
> > +/*
> > + * Those states represent a subset of the CFI flash state machine.
> > + */
> > +enum cfi_flash_state {
> > +	READY,
> > +	LOCK_SETUP,
> > +	WP_SETUP,
> > +	BP_SETUP,
> > +	BP_LOAD,
> > +	ERASE_SETUP,
> > +};
> > +
> > +/*
> > + * The device can be in several **Read** modes.
> > + * We don't implement the asynchronous burst mode.
> > + */
> > +enum cfi_read_mode {
> > +	READ_ARRAY,
> > +	READ_STATUS,
> > +	READ_DEVICE_ID,
> > +	READ_QUERY,
> > +};
> > +
> > +struct cfi_flash_device {
> > +	struct device_header	dev_hdr;
> > +	/* Protects the CFI state machine variables in this data structure. */
> > +	struct mutex		mutex;
> > +	u64			base_addr;
> > +	u32			size;
> > +
> > +	void			*flash_memory;
> > +	u8			program_buffer[PROGRAM_BUFF_SIZE * 4];  
> 
> You're multiplying by 4 because PROGRAM_BUFF_SIZE is the size of the buffer in
> words, right?

Yeah, I can use "sizeof(u32)" if that is better.

> > +	unsigned long		*lock_bm;
> > +	u64			last_address;
> > +	unsigned int		buff_written;
> > +	unsigned int		program_length;
> > +
> > +	enum cfi_flash_state	state;
> > +	enum cfi_read_mode	read_mode;
> > +	u16			rcr;
> > +	u8			sr;
> > +};
> > +
> > +static int nr_erase_blocks(struct cfi_flash_device *sfdev)
> > +{
> > +	return sfdev->size / FLASH_BLOCK_SIZE;
> > +}
> > +
> > +/*
> > + * CFI queries always deal with one byte of information, possibly mirrored
> > + * to other bytes on the bus. This is dealt with in the callers.
> > + * The address provided is the one for 8-bit addressing, and would need to
> > + * be adjusted for wider accesses.
> > + */
> > +static u8 read_cfi(struct cfi_flash_device *sfdev, u64 addr)
> > +{
> > +	if (addr < 0x10)		/* CFI information starts at 0x10 */
> > +		return 0;
> > +
> > +	if (addr - 0x10 > sizeof(cfi_query_table)) {
> > +		pr_debug("CFI query read access beyond the end of table");
> > +		return 0;
> > +	}
> > +
> > +	/* Fixup dynamic information in the geometry part of the table. */
> > +	switch (addr) {
> > +	case CFI_GEOM_OFFSET:		/* device size in bytes, power of two */
> > +		return pow2_size(sfdev->size / CFI_NR_FLASH_CHIPS);
> > +	case CFI_GEOM_OFFSET + 6:	/* number of erase blocks, minus one */
> > +		return (nr_erase_blocks(sfdev) - 1) & 0xff;
> > +	case CFI_GEOM_OFFSET + 7:
> > +		return (nr_erase_blocks(sfdev) - 1) >> 8;
> > +	case CFI_GEOM_OFFSET + 8:	/* erase block size, in units of 256 */
> > +		return ((FLASH_BLOCK_SIZE / 256 ) / CFI_NR_FLASH_CHIPS) & 0xff;
> > +	case CFI_GEOM_OFFSET + 9:
> > +		return ((FLASH_BLOCK_SIZE / 256 ) / CFI_NR_FLASH_CHIPS) >> 8;
> > +	}
> > +
> > +	return cfi_query_table[addr - 0x10];
> > +}
> > +
> > +static bool block_is_locked(struct cfi_flash_device *sfdev, u64 addr)
> > +{
> > +	int block_nr = addr / FLASH_BLOCK_SIZE;
> > +
> > +	return test_bit(block_nr, sfdev->lock_bm);
> > +}
> > +
> > +#define DEV_ID_MASK 0x7ff
> > +static u16 read_dev_id(struct cfi_flash_device *sfdev, u64 addr)
> > +{
> > +	switch ((addr & DEV_ID_MASK) / CFI_BUS_WIDTH) {
> > +	case 0x0:				/* vendor ID */
> > +		return 0x0000;
> > +	case 0x1:				/* device ID */
> > +		return 0xffff;
> > +	case 0x2:
> > +		return block_is_locked(sfdev, addr & ~DEV_ID_MASK);
> > +	case 0x5:
> > +		return sfdev->rcr;
> > +	default:			/* Ignore the other entries. */
> > +		return 0;
> > +	}
> > +}
> > +
> > +static void lock_block(struct cfi_flash_device *sfdev, u64 addr, bool lock)
> > +{
> > +	int block_nr = addr / FLASH_BLOCK_SIZE;
> > +
> > +	if (lock)
> > +		set_bit(block_nr, sfdev->lock_bm);
> > +	else
> > +		clear_bit(block_nr, sfdev->lock_bm);
> > +}
> > +
> > +static void word_program(struct cfi_flash_device *sfdev,
> > +			 u64 addr, void *data, int len)
> > +{
> > +	if (block_is_locked(sfdev, addr)) {
> > +		sfdev->sr |= 0x12;
> > +		return;
> > +	}
> > +
> > +	memcpy(sfdev->flash_memory + addr, data, len);
> > +}
> > +
> > +/* Reset the program buffer state to prepare for follow-up writes. */
> > +static void buffer_setup(struct cfi_flash_device *sfdev)
> > +{
> > +	memset(sfdev->program_buffer, 0, sizeof(sfdev->program_buffer));
> > +	sfdev->last_address = ~0ULL;
> > +	sfdev->buff_written = 0;
> > +}
> > +
> > +static bool buffer_program(struct cfi_flash_device *sfdev,
> > +			   u64 addr, void *buffer, int len)
> > +{
> > +	unsigned int buf_addr;
> > +
> > +	if (sfdev->buff_written >= sfdev->program_length)
> > +		return false;
> > +
> > +	/*
> > +	 * The first word written into the buffer after the setup command
> > +	 * happens to be the base address for the buffer.
> > +	 * All subsequent writes need to be within this address and this
> > +	 * address plus the buffer size, so keep this value around.
> > +	 */
> > +	if (sfdev->last_address == ~0ULL)
> > +		sfdev->last_address = addr;
> > +
> > +	if (addr < sfdev->last_address)
> > +		return false;
> > +	buf_addr = addr - sfdev->last_address;
> > +	if (buf_addr >= PROGRAM_BUFF_SIZE)
> > +		return false;
> > +
> > +	memcpy(sfdev->program_buffer + buf_addr, buffer, len);
> > +	sfdev->buff_written++;
> > +
> > +	return true;
> > +}
> > +
> > +static void buffer_confirm(struct cfi_flash_device *sfdev)
> > +{
> > +	if (block_is_locked(sfdev, sfdev->last_address)) {
> > +		sfdev->sr |= 0x12;
> > +		return;
> > +	}
> > +	memcpy(sfdev->flash_memory + sfdev->last_address,
> > +	       sfdev->program_buffer,
> > +	       sfdev->buff_written * sizeof(u32));
> > +}
> > +
> > +static void block_erase_confirm(struct cfi_flash_device *sfdev, u64 addr)
> > +{
> > +	if (block_is_locked(sfdev, addr)) {
> > +		sfdev->sr |= 0x12;
> > +		return;
> > +	}
> > +
> > +	memset(sfdev->flash_memory + addr, 0xFF, FLASH_BLOCK_SIZE);
> > +}
> > +
> > +static void cfi_flash_mmio(struct kvm_cpu *vcpu,
> > +			   u64 addr, u8 *data, u32 len, u8 is_write,
> > +			   void *context)
> > +{
> > +	struct cfi_flash_device *sfdev = context;
> > +	u64 faddr = addr - sfdev->base_addr;
> > +	u32 value;
> > +
> > +	if (!is_write) {
> > +		u16 cfi_value = 0;
> > +
> > +		mutex_lock(&sfdev->mutex);
> > +
> > +		switch (sfdev->read_mode) {
> > +		case READ_ARRAY:
> > +			/* just copy the requested bytes from the array */
> > +			memcpy(data, sfdev->flash_memory + faddr, len);
> > +			goto out_unlock;
> > +		case READ_STATUS:
> > +			cfi_value = sfdev->sr;
> > +			break;
> > +		case READ_DEVICE_ID:
> > +			cfi_value = read_dev_id(sfdev, faddr);
> > +			break;
> > +		case READ_QUERY:
> > +			cfi_value = read_cfi(sfdev, faddr / CFI_BUS_WIDTH);
> > +			break;
> > +		}
> > +		switch (len) {
> > +		case 1:
> > +			*data = cfi_value;
> > +			break;
> > +		case 8: memset(data + 4, 0, 4);
> > +			/* fall-through */
> > +		case 4:
> > +			if (CFI_NR_FLASH_CHIPS == 2)
> > +				memcpy(data + 2, &cfi_value, 2);
> > +			else
> > +				memset(data + 2, 0, 2);
> > +			/* fall-through */
> > +		case 2:
> > +			memcpy(data, &cfi_value, 2);
> > +			break;
> > +		default:
> > +			pr_debug("CFI flash: illegal access length %d for read mode %d",
> > +				 len, sfdev->read_mode);
> > +			break;
> > +		}
> > +
> > +		goto out_unlock;
> > +	}
> > +
> > +	if (len > 4) {
> > +		pr_info("CFI flash: MMIO %d-bit write access not supported",
> > +			 len * 8);
> > +		return;
> > +	}
> > +
> > +	memcpy(&value, data, len);
> > +
> > +	mutex_lock(&sfdev->mutex);
> > +
> > +	switch (sfdev->state) {
> > +	case READY:			/* handled below */
> > +		break;
> > +
> > +	case LOCK_SETUP:
> > +		switch (value & 0xff) {
> > +		case CFI_CMD_LOCK_BLOCK:
> > +			lock_block(sfdev, faddr, true);
> > +			sfdev->read_mode = READ_STATUS;
> > +			break;
> > +		case CFI_CMD_UNLOCK_BLOCK:
> > +			lock_block(sfdev, faddr, false);
> > +			sfdev->read_mode = READ_STATUS;
> > +			break;
> > +		default:
> > +			sfdev->sr |= 0x30;
> > +			break;
> > +		}
> > +		sfdev->state = READY;
> > +		goto out_unlock;
> > +
> > +	case WP_SETUP:
> > +		word_program(sfdev, faddr, data, len);
> > +		sfdev->read_mode = READ_STATUS;
> > +		sfdev->state = READY;
> > +		goto out_unlock;
> > +
> > +	case BP_LOAD:
> > +		if (buffer_program(sfdev, faddr, data, len))
> > +			goto out_unlock;
> > +
> > +		if ((value & 0xFF) == CFI_CMD_BUFFERED_PROGRAM_CONFIRM) {
> > +			buffer_confirm(sfdev);
> > +			sfdev->read_mode = READ_STATUS;
> > +		} else {
> > +			pr_debug("CFI flash: BP_LOAD: expected CONFIRM(0xd0), got 0x%x @ 0x%llx",
> > +				 value, faddr);
> > +			sfdev->sr |= 0x10;
> > +		}
> > +		sfdev->state = READY;
> > +		goto out_unlock;
> > +
> > +	case BP_SETUP:
> > +		sfdev->program_length = (value & 0xffff) + 1;
> > +		if (sfdev->program_length > PROGRAM_BUFF_SIZE / 4)
> > +			sfdev->program_length = PROGRAM_BUFF_SIZE / 4;
> > +		sfdev->state = BP_LOAD;
> > +		sfdev->read_mode = READ_STATUS;
> > +		goto out_unlock;
> > +
> > +	case ERASE_SETUP:
> > +		if ((value & 0xff) == CFI_CMD_BLOCK_ERASE_CONFIRM)
> > +			block_erase_confirm(sfdev, faddr);
> > +		else
> > +			sfdev->sr |= 0x30;
> > +
> > +		sfdev->state = READY;
> > +		sfdev->read_mode = READ_STATUS;
> > +		goto out_unlock;
> > +	}
> > +
> > +	/* write commands in READY state */
> > +	switch (value & 0xFF) {
> > +	case CFI_CMD_READ_JEDEC:
> > +		sfdev->read_mode = READ_DEVICE_ID;
> > +		break;
> > +	case CFI_CMD_READ_STATUS_REGISTER:
> > +		sfdev->read_mode = READ_STATUS;
> > +		break;
> > +	case CFI_CMD_READ_CFI_QUERY:
> > +		sfdev->read_mode = READ_QUERY;
> > +		break;
> > +	case CFI_CMD_CLEAR_STATUS_REGISTER:
> > +		sfdev->sr = 0x80;
> > +		break;
> > +	case CFI_CMD_WORD_PROGRAM_SETUP:
> > +	case CFI_CMD_ALTERNATE_WORD_PROGRAM_SETUP:
> > +		sfdev->state = WP_SETUP;
> > +		sfdev->read_mode = READ_STATUS;
> > +		break;
> > +	case CFI_CMD_LOCK_BLOCK_SETUP:
> > +		sfdev->state = LOCK_SETUP;
> > +		break;
> > +	case CFI_CMD_BLOCK_ERASE_SETUP:
> > +		sfdev->state = ERASE_SETUP;
> > +		sfdev->read_mode = READ_STATUS;
> > +		break;
> > +	case CFI_CMD_BUFFERED_PROGRAM_SETUP:
> > +		buffer_setup(sfdev);
> > +		sfdev->state = BP_SETUP;
> > +		sfdev->read_mode = READ_STATUS;
> > +		break;
> > +	case CFI_CMD_BUFFERED_PROGRAM_CONFIRM:
> > +		pr_debug("CFI flash: unexpected confirm command 0xD0");
> > +		break;
> > +	default:
> > +		pr_debug("CFI flash: unknown command 0x%x", value);
> > +		/* fall through */  
> 
> Above (in the read case), you wrote it "fall-through".

GCC has a list of allowed spellings, and both versions are in it ;-)
But sure will fix this ...
 
> > +	case CFI_CMD_READ_ARRAY:
> > +		sfdev->read_mode = READ_ARRAY;
> > +		break;
> > +	}
> > +
> > +out_unlock:
> > +	mutex_unlock(&sfdev->mutex);
> > +}  
> 
> The function is huge and complicated. How about splitting it into a read and write
> function, at the very least?

Good point. Looks like "write command in READY state" should be separate as well, since it's only doing state transitions.

> > +
> > +#ifdef CONFIG_HAS_LIBFDT
> > +static void generate_cfi_flash_fdt_node(void *fdt,
> > +					struct device_header *dev_hdr,
> > +					void (*generate_irq_prop)(void *fdt,
> > +								  u8 irq,
> > +								enum irq_type))
> > +{
> > +	struct cfi_flash_device *sfdev;
> > +	u64 reg_prop[2];
> > +
> > +	sfdev = container_of(dev_hdr, struct cfi_flash_device, dev_hdr);
> > +	reg_prop[0] = cpu_to_fdt64(sfdev->base_addr);
> > +	reg_prop[1] = cpu_to_fdt64(sfdev->size);
> > +
> > +	_FDT(fdt_begin_node(fdt, "flash"));
> > +	_FDT(fdt_property_cell(fdt, "bank-width", CFI_BUS_WIDTH));
> > +	_FDT(fdt_property_cell(fdt, "#address-cells", 0x1));
> > +	_FDT(fdt_property_cell(fdt, "#size-cells", 0x1));
> > +	_FDT(fdt_property_string(fdt, "compatible", "cfi-flash"));
> > +	_FDT(fdt_property_string(fdt, "label", "System-firmware"));
> > +	_FDT(fdt_property(fdt, "reg", &reg_prop, sizeof(reg_prop)));
> > +	_FDT(fdt_end_node(fdt));
> > +}
> > +#else
> > +#define generate_cfi_flash_fdt_node NULL
> > +#endif
> > +
> > +static struct cfi_flash_device *create_flash_device_file(struct kvm *kvm,
> > +							 const char *filename)
> > +{
> > +	struct cfi_flash_device *sfdev;
> > +	struct stat statbuf;  
> 
> Here you're using "buf" as shorthand for "buffer", but at the top of the file
> (PROGRAM_BUFF_*) you use "buff".

I guess because one was written by me, the other by Raphael ;-)
Will consolidate this.

> 
> > +	unsigned int value;
> > +	int ret;
> > +	int fd;
> > +
> > +	fd = open(filename, O_RDWR);
> > +	if (fd < 0)
> > +		return ERR_PTR(-errno);
> > +	if (fstat(fd, &statbuf) < 0) {
> > +		close(fd);
> > +		return ERR_PTR(-errno);
> > +	}
> > +
> > +	sfdev = malloc(sizeof(struct cfi_flash_device));
> > +	if (!sfdev) {
> > +		close(fd);
> > +		return ERR_PTR(-ENOMEM);
> > +	}
> > +
> > +	sfdev->size = (statbuf.st_size + 4095) & ~0xfffUL;
> > +	sfdev->flash_memory = mmap(NULL, statbuf.st_size,
> > +				   PROT_READ | PROT_WRITE, MAP_SHARED,
> > +				   fd, 0);
> > +	if (sfdev->flash_memory == MAP_FAILED) {
> > +		close(fd);
> > +		free(sfdev);
> > +		return ERR_PTR(-errno);
> > +	}
> > +	sfdev->base_addr = KVM_FLASH_MMIO_BASE;
> > +	sfdev->state = READY;
> > +	sfdev->read_mode = READ_ARRAY;
> > +	sfdev->sr = 0x80;
> > +	sfdev->rcr = 0xbfcf;
> > +
> > +	value = roundup(nr_erase_blocks(sfdev), BITS_PER_LONG) / 8;
> > +	sfdev->lock_bm = malloc(value);
> > +	memset(sfdev->lock_bm, 0, value);
> > +
> > +	sfdev->dev_hdr.bus_type = DEVICE_BUS_MMIO;
> > +	sfdev->dev_hdr.data = generate_cfi_flash_fdt_node;
> > +	mutex_init(&sfdev->mutex);
> > +	ret = device__register(&sfdev->dev_hdr);
> > +	if (ret) {
> > +		free(sfdev->flash_memory);  
> 
> That's strange, I wrote a quick test for this and free'ing a a file-backed mmap'ed
> memory resulted in a segmentation fault. Did you mean munmap?

Ouch, indeed. Looks like a leftover from the original code, which was using malloc().

> Also, if mmap'ing
> the flash fails, you close the file descriptor, which you don't do here. To be
> honest, I think the best approach would be to add all the cleaning up  after the
> return statement and a series of labels to jump to depending where you got an
> error (similar to virtio__pci_init).

Yeah, it looks much better now that way.

> 
> > +		free(sfdev);
> > +		return ERR_PTR(ret);
> > +	}
> > +
> > +	ret = kvm__register_mmio(kvm,
> > +				 sfdev->base_addr, sfdev->size,
> > +				 false, cfi_flash_mmio, sfdev);
> > +	if (ret) {
> > +		device__unregister(&sfdev->dev_hdr);
> > +		free(sfdev->flash_memory);
> > +		free(sfdev);
> > +		return ERR_PTR(ret);
> > +	}
> > +
> > +	return sfdev;
> > +}
> > +
> > +static int flash__init(struct kvm *kvm)
> > +{
> > +	struct cfi_flash_device *sfdev;
> > +
> > +	if (!kvm->cfg.flash_filename)
> > +		return 0;
> > +
> > +	sfdev = create_flash_device_file(kvm, kvm->cfg.flash_filename);
> > +	if (IS_ERR(sfdev))
> > +		return PTR_ERR(sfdev);
> > +
> > +	return 0;
> > +}
> > +dev_init(flash__init);
> > diff --git a/include/kvm/kvm-config.h b/include/kvm/kvm-config.h
> > index a052b0bc..f4a8b831 100644
> > --- a/include/kvm/kvm-config.h
> > +++ b/include/kvm/kvm-config.h
> > @@ -35,6 +35,7 @@ struct kvm_config {
> >  	const char *vmlinux_filename;
> >  	const char *initrd_filename;
> >  	const char *firmware_filename;
> > +	const char *flash_filename;
> >  	const char *console;
> >  	const char *dev;
> >  	const char *network;
> > diff --git a/include/kvm/util.h b/include/kvm/util.h
> > index 4ca7aa93..5c37f0b7 100644
> > --- a/include/kvm/util.h
> > +++ b/include/kvm/util.h
> > @@ -104,6 +104,11 @@ static inline unsigned long roundup_pow_of_two(unsigned long x)
> >  	return x ? 1UL << fls_long(x - 1) : 0;
> >  }
> >  
> > +static inline int pow2_size(unsigned long x)
> > +{
> > +	return (sizeof(x) * 8) - __builtin_clzl(x - 1);
> > +}  
> 
> For the life of me I can't understand what this function is supposed to do. Also,
> from the gcc online docs:

The idea is to determine the "number of address bits needed to cover x bytes of memory", which is something that is well known on actual hardware. I will add a comment.
 
> "Returns the number of leading 0-bits in x, starting at the most significant bit
> position. If xis 0, the result is undefined."
> 
> you might want to add a special case for x == 1.

Good point, although in our case the input value is always at least 2048. But 0 isn't covered as well and also I moved this to generic code, so will fix it.

Cheers,
Andre

> 
> Thanks,
> Alex
> > +
> >  struct kvm;
> >  void *mmap_hugetlbfs(struct kvm *kvm, const char *htlbfs_path, u64 size);
> >  void *mmap_anon_or_hugetlbfs(struct kvm *kvm, const char *hugetlbfs_path, u64 size);  


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel