Hi,
On 2/14/20 1:47 PM, Andre Przywara wrote:
> On Fri, 7 Feb 2020 17:34:20 +0000
> Alexandru Elisei <alexandru.eli...@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.
I'll finish my review on this version of the patch, no need to post a v3.
>
>> On 2/7/20 12:19 PM, Andre Przywara wrote:
>>> From: Raphael Gault <raphael.ga...@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.ga...@arm.com>
>>> [Andre: rewriting and fixing]
>>> Signed-off-by: Andre Przywra <andre.przyw...@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.
From this and the above, it kinda sounds like we a flexible memory layout for
kvmtool, doesn't it? ;-)
>
>>> #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 ...
I was commenting on the consistency. I don't have a preference for a particular
spelling.
>
>>> + 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.
Good idea.
Thanks,
Alex
>
>>> +
>>> +#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", ®_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);
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
