diff mbox series

[v1,5/6] riscv/virt: Add the PFlash CFI01 device

Message ID 0a5c141a26fada6d93d06e996a2f24e1b269ec50.1568931866.git.alistair.francis@wdc.com
State New
Headers show
Series RISC-V: Add more machine memory | expand

Commit Message

Alistair Francis Sept. 19, 2019, 10:25 p.m. UTC
Add the CFI01 PFlash to the RISC-V virt board. This is the same PFlash
from the ARM Virt board and the implementation is based on the ARM Virt
board. This allows users to specify flash files from the command line.

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 hw/riscv/Kconfig        |  1 +
 hw/riscv/virt.c         | 81 +++++++++++++++++++++++++++++++++++++++++
 include/hw/riscv/virt.h |  3 ++
 3 files changed, 85 insertions(+)

Comments

Bin Meng Sept. 20, 2019, 5:15 a.m. UTC | #1
On Fri, Sep 20, 2019 at 6:36 AM Alistair Francis
<alistair.francis@wdc.com> wrote:
>
> Add the CFI01 PFlash to the RISC-V virt board. This is the same PFlash
> from the ARM Virt board and the implementation is based on the ARM Virt
> board. This allows users to specify flash files from the command line.
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  hw/riscv/Kconfig        |  1 +
>  hw/riscv/virt.c         | 81 +++++++++++++++++++++++++++++++++++++++++
>  include/hw/riscv/virt.h |  3 ++
>  3 files changed, 85 insertions(+)
>
> diff --git a/hw/riscv/Kconfig b/hw/riscv/Kconfig
> index fb19b2df3a..b12660b9f8 100644
> --- a/hw/riscv/Kconfig
> +++ b/hw/riscv/Kconfig
> @@ -36,4 +36,5 @@ config RISCV_VIRT
>      select SERIAL
>      select VIRTIO_MMIO
>      select PCI_EXPRESS_GENERIC_BRIDGE
> +    select PFLASH_CFI01
>      select SIFIVE
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index d36f5625ec..ca002ecea7 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -26,6 +26,7 @@
>  #include "hw/boards.h"
>  #include "hw/loader.h"
>  #include "hw/sysbus.h"
> +#include "hw/qdev-properties.h"
>  #include "hw/char/serial.h"
>  #include "target/riscv/cpu.h"
>  #include "hw/riscv/riscv_hart.h"
> @@ -61,12 +62,72 @@ static const struct MemmapEntry {
>      [VIRT_PLIC] =        {  0xc000000,     0x4000000 },
>      [VIRT_UART0] =       { 0x10000000,         0x100 },
>      [VIRT_VIRTIO] =      { 0x10001000,        0x1000 },
> +    [VIRT_FLASH] =       { 0x20000000,     0x2000000 },
>      [VIRT_DRAM] =        { 0x80000000,           0x0 },
>      [VIRT_PCIE_MMIO] =   { 0x40000000,    0x40000000 },
>      [VIRT_PCIE_PIO] =    { 0x03000000,    0x00010000 },
>      [VIRT_PCIE_ECAM] =   { 0x30000000,    0x10000000 },
>  };
>
> +#define VIRT_FLASH_SECTOR_SIZE (256 * KiB)
> +
> +static PFlashCFI01 *virt_flash_create1(RISCVVirtState *s,
> +                                       const char *name,
> +                                       const char *alias_prop_name)
> +{
> +    /*
> +     * Create a single flash device.  We use the same parameters as
> +     * the flash devices on the ARM virt board.
> +     */
> +    DeviceState *dev = qdev_create(NULL, TYPE_PFLASH_CFI01);
> +
> +    qdev_prop_set_uint64(dev, "sector-length", VIRT_FLASH_SECTOR_SIZE);
> +    qdev_prop_set_uint8(dev, "width", 4);
> +    qdev_prop_set_uint8(dev, "device-width", 2);
> +    qdev_prop_set_bit(dev, "big-endian", false);
> +    qdev_prop_set_uint16(dev, "id0", 0x89);
> +    qdev_prop_set_uint16(dev, "id1", 0x18);
> +    qdev_prop_set_uint16(dev, "id2", 0x00);
> +    qdev_prop_set_uint16(dev, "id3", 0x00);
> +    qdev_prop_set_string(dev, "name", name);

alias_prop_name is unused? ARM virt has 2 more calls in the same function here.

> +
> +    return PFLASH_CFI01(dev);
> +}
> +
> +static void virt_flash_create(RISCVVirtState *s)
> +{
> +    s->flash[0] = virt_flash_create1(s, "virt.flash0", "pflash0");
> +    s->flash[1] = virt_flash_create1(s, "virt.flash1", "pflash1");

I don't think we should mirror what is used on ARM virt board to
create 2 flash for sifive_u. For ARM virt, there are 2 flashes because
they need distinguish secure and non-secure. For sifive_u, only one is
enough.

> +}
> +
> +static void virt_flash_map1(PFlashCFI01 *flash,
> +                            hwaddr base, hwaddr size,
> +                            MemoryRegion *sysmem)
> +{
> +    DeviceState *dev = DEVICE(flash);
> +
> +    assert(size % VIRT_FLASH_SECTOR_SIZE == 0);
> +    assert(size / VIRT_FLASH_SECTOR_SIZE <= UINT32_MAX);
> +    qdev_prop_set_uint32(dev, "num-blocks", size / VIRT_FLASH_SECTOR_SIZE);
> +    qdev_init_nofail(dev);
> +
> +    memory_region_add_subregion(sysmem, base,
> +                                sysbus_mmio_get_region(SYS_BUS_DEVICE(dev),
> +                                                       0));
> +}
> +
> +static void virt_flash_map(RISCVVirtState *s,
> +                           MemoryRegion *sysmem)
> +{
> +    hwaddr flashsize = virt_memmap[VIRT_FLASH].size / 2;
> +    hwaddr flashbase = virt_memmap[VIRT_FLASH].base;
> +
> +    virt_flash_map1(s->flash[0], flashbase, flashsize,
> +                    sysmem);
> +    virt_flash_map1(s->flash[1], flashbase + flashsize, flashsize,
> +                    sysmem);
> +}
> +
>  static void create_pcie_irq_map(void *fdt, char *nodename,
>                                  uint32_t plic_phandle)
>  {
> @@ -121,6 +182,8 @@ static void create_fdt(RISCVVirtState *s, const struct MemmapEntry *memmap,
>      char *nodename;
>      uint32_t plic_phandle, phandle = 1;
>      int i;
> +    hwaddr flashsize = virt_memmap[VIRT_FLASH].size / 2;
> +    hwaddr flashbase = virt_memmap[VIRT_FLASH].base;
>
>      fdt = s->fdt = create_device_tree(&s->fdt_size);
>      if (!fdt) {
> @@ -316,6 +379,15 @@ static void create_fdt(RISCVVirtState *s, const struct MemmapEntry *memmap,
>          qemu_fdt_setprop_string(fdt, "/chosen", "bootargs", cmdline);
>      }
>      g_free(nodename);
> +
> +    nodename = g_strdup_printf("/flash@%" PRIx64, flashbase);
> +    qemu_fdt_add_subnode(s->fdt, nodename);
> +    qemu_fdt_setprop_string(s->fdt, nodename, "compatible", "cfi-flash");
> +    qemu_fdt_setprop_sized_cells(s->fdt, nodename, "reg",
> +                                 2, flashbase, 2, flashsize,
> +                                 2, flashbase + flashsize, 2, flashsize);
> +    qemu_fdt_setprop_cell(s->fdt, nodename, "bank-width", 4);
> +    g_free(nodename);
>  }
>
>
> @@ -496,6 +568,15 @@ static void riscv_virt_board_init(MachineState *machine)
>          0, qdev_get_gpio_in(DEVICE(s->plic), UART0_IRQ), 399193,
>          serial_hd(0), DEVICE_LITTLE_ENDIAN);
>
> +    virt_flash_create(s);
> +
> +    /* Map legacy -drive if=pflash to machine properties */
> +    for (i = 0; i < ARRAY_SIZE(s->flash); i++) {
> +        pflash_cfi01_legacy_drive(s->flash[i],
> +                                  drive_get(IF_PFLASH, 0, i));
> +    }
> +    virt_flash_map(s, system_memory);
> +
>      g_free(plic_hart_config);
>  }
>
> diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
> index 6e5fbe5d3b..2ca8bd3512 100644
> --- a/include/hw/riscv/virt.h
> +++ b/include/hw/riscv/virt.h
> @@ -21,6 +21,7 @@
>
>  #include "hw/riscv/riscv_hart.h"
>  #include "hw/sysbus.h"
> +#include "hw/block/flash.h"
>
>  typedef struct {
>      /*< private >*/
> @@ -29,6 +30,7 @@ typedef struct {
>      /*< public >*/
>      RISCVHartArrayState soc;
>      DeviceState *plic;
> +    PFlashCFI01 *flash[2];
>      void *fdt;
>      int fdt_size;
>  } RISCVVirtState;
> @@ -41,6 +43,7 @@ enum {
>      VIRT_PLIC,
>      VIRT_UART0,
>      VIRT_VIRTIO,
> +    VIRT_FLASH,
>      VIRT_DRAM,
>      VIRT_PCIE_MMIO,
>      VIRT_PCIE_PIO,
> --

Regards,
Bin
Alistair Francis Sept. 20, 2019, 10:12 p.m. UTC | #2
On Thu, Sep 19, 2019 at 10:15 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Fri, Sep 20, 2019 at 6:36 AM Alistair Francis
> <alistair.francis@wdc.com> wrote:
> >
> > Add the CFI01 PFlash to the RISC-V virt board. This is the same PFlash
> > from the ARM Virt board and the implementation is based on the ARM Virt
> > board. This allows users to specify flash files from the command line.
> >
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> >  hw/riscv/Kconfig        |  1 +
> >  hw/riscv/virt.c         | 81 +++++++++++++++++++++++++++++++++++++++++
> >  include/hw/riscv/virt.h |  3 ++
> >  3 files changed, 85 insertions(+)
> >
> > diff --git a/hw/riscv/Kconfig b/hw/riscv/Kconfig
> > index fb19b2df3a..b12660b9f8 100644
> > --- a/hw/riscv/Kconfig
> > +++ b/hw/riscv/Kconfig
> > @@ -36,4 +36,5 @@ config RISCV_VIRT
> >      select SERIAL
> >      select VIRTIO_MMIO
> >      select PCI_EXPRESS_GENERIC_BRIDGE
> > +    select PFLASH_CFI01
> >      select SIFIVE
> > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> > index d36f5625ec..ca002ecea7 100644
> > --- a/hw/riscv/virt.c
> > +++ b/hw/riscv/virt.c
> > @@ -26,6 +26,7 @@
> >  #include "hw/boards.h"
> >  #include "hw/loader.h"
> >  #include "hw/sysbus.h"
> > +#include "hw/qdev-properties.h"
> >  #include "hw/char/serial.h"
> >  #include "target/riscv/cpu.h"
> >  #include "hw/riscv/riscv_hart.h"
> > @@ -61,12 +62,72 @@ static const struct MemmapEntry {
> >      [VIRT_PLIC] =        {  0xc000000,     0x4000000 },
> >      [VIRT_UART0] =       { 0x10000000,         0x100 },
> >      [VIRT_VIRTIO] =      { 0x10001000,        0x1000 },
> > +    [VIRT_FLASH] =       { 0x20000000,     0x2000000 },
> >      [VIRT_DRAM] =        { 0x80000000,           0x0 },
> >      [VIRT_PCIE_MMIO] =   { 0x40000000,    0x40000000 },
> >      [VIRT_PCIE_PIO] =    { 0x03000000,    0x00010000 },
> >      [VIRT_PCIE_ECAM] =   { 0x30000000,    0x10000000 },
> >  };
> >
> > +#define VIRT_FLASH_SECTOR_SIZE (256 * KiB)
> > +
> > +static PFlashCFI01 *virt_flash_create1(RISCVVirtState *s,
> > +                                       const char *name,
> > +                                       const char *alias_prop_name)
> > +{
> > +    /*
> > +     * Create a single flash device.  We use the same parameters as
> > +     * the flash devices on the ARM virt board.
> > +     */
> > +    DeviceState *dev = qdev_create(NULL, TYPE_PFLASH_CFI01);
> > +
> > +    qdev_prop_set_uint64(dev, "sector-length", VIRT_FLASH_SECTOR_SIZE);
> > +    qdev_prop_set_uint8(dev, "width", 4);
> > +    qdev_prop_set_uint8(dev, "device-width", 2);
> > +    qdev_prop_set_bit(dev, "big-endian", false);
> > +    qdev_prop_set_uint16(dev, "id0", 0x89);
> > +    qdev_prop_set_uint16(dev, "id1", 0x18);
> > +    qdev_prop_set_uint16(dev, "id2", 0x00);
> > +    qdev_prop_set_uint16(dev, "id3", 0x00);
> > +    qdev_prop_set_string(dev, "name", name);
>
> alias_prop_name is unused? ARM virt has 2 more calls in the same function here.

Yep, you are right. I have removed this.

>
> > +
> > +    return PFLASH_CFI01(dev);
> > +}
> > +
> > +static void virt_flash_create(RISCVVirtState *s)
> > +{
> > +    s->flash[0] = virt_flash_create1(s, "virt.flash0", "pflash0");
> > +    s->flash[1] = virt_flash_create1(s, "virt.flash1", "pflash1");
>
> I don't think we should mirror what is used on ARM virt board to
> create 2 flash for sifive_u. For ARM virt, there are 2 flashes because
> they need distinguish secure and non-secure. For sifive_u, only one is
> enough.

I went back and forward about 1 or 2. Two seems more usable as maybe
someone wants to include two pflash files? The Xilinx machine also has
two so I'm kind of used to 2, but I'm not really fussed.

Unless anyone else wants two I will change it to 1.

Alistair

>
> > +}
> > +
> > +static void virt_flash_map1(PFlashCFI01 *flash,
> > +                            hwaddr base, hwaddr size,
> > +                            MemoryRegion *sysmem)
> > +{
> > +    DeviceState *dev = DEVICE(flash);
> > +
> > +    assert(size % VIRT_FLASH_SECTOR_SIZE == 0);
> > +    assert(size / VIRT_FLASH_SECTOR_SIZE <= UINT32_MAX);
> > +    qdev_prop_set_uint32(dev, "num-blocks", size / VIRT_FLASH_SECTOR_SIZE);
> > +    qdev_init_nofail(dev);
> > +
> > +    memory_region_add_subregion(sysmem, base,
> > +                                sysbus_mmio_get_region(SYS_BUS_DEVICE(dev),
> > +                                                       0));
> > +}
> > +
> > +static void virt_flash_map(RISCVVirtState *s,
> > +                           MemoryRegion *sysmem)
> > +{
> > +    hwaddr flashsize = virt_memmap[VIRT_FLASH].size / 2;
> > +    hwaddr flashbase = virt_memmap[VIRT_FLASH].base;
> > +
> > +    virt_flash_map1(s->flash[0], flashbase, flashsize,
> > +                    sysmem);
> > +    virt_flash_map1(s->flash[1], flashbase + flashsize, flashsize,
> > +                    sysmem);
> > +}
> > +
> >  static void create_pcie_irq_map(void *fdt, char *nodename,
> >                                  uint32_t plic_phandle)
> >  {
> > @@ -121,6 +182,8 @@ static void create_fdt(RISCVVirtState *s, const struct MemmapEntry *memmap,
> >      char *nodename;
> >      uint32_t plic_phandle, phandle = 1;
> >      int i;
> > +    hwaddr flashsize = virt_memmap[VIRT_FLASH].size / 2;
> > +    hwaddr flashbase = virt_memmap[VIRT_FLASH].base;
> >
> >      fdt = s->fdt = create_device_tree(&s->fdt_size);
> >      if (!fdt) {
> > @@ -316,6 +379,15 @@ static void create_fdt(RISCVVirtState *s, const struct MemmapEntry *memmap,
> >          qemu_fdt_setprop_string(fdt, "/chosen", "bootargs", cmdline);
> >      }
> >      g_free(nodename);
> > +
> > +    nodename = g_strdup_printf("/flash@%" PRIx64, flashbase);
> > +    qemu_fdt_add_subnode(s->fdt, nodename);
> > +    qemu_fdt_setprop_string(s->fdt, nodename, "compatible", "cfi-flash");
> > +    qemu_fdt_setprop_sized_cells(s->fdt, nodename, "reg",
> > +                                 2, flashbase, 2, flashsize,
> > +                                 2, flashbase + flashsize, 2, flashsize);
> > +    qemu_fdt_setprop_cell(s->fdt, nodename, "bank-width", 4);
> > +    g_free(nodename);
> >  }
> >
> >
> > @@ -496,6 +568,15 @@ static void riscv_virt_board_init(MachineState *machine)
> >          0, qdev_get_gpio_in(DEVICE(s->plic), UART0_IRQ), 399193,
> >          serial_hd(0), DEVICE_LITTLE_ENDIAN);
> >
> > +    virt_flash_create(s);
> > +
> > +    /* Map legacy -drive if=pflash to machine properties */
> > +    for (i = 0; i < ARRAY_SIZE(s->flash); i++) {
> > +        pflash_cfi01_legacy_drive(s->flash[i],
> > +                                  drive_get(IF_PFLASH, 0, i));
> > +    }
> > +    virt_flash_map(s, system_memory);
> > +
> >      g_free(plic_hart_config);
> >  }
> >
> > diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
> > index 6e5fbe5d3b..2ca8bd3512 100644
> > --- a/include/hw/riscv/virt.h
> > +++ b/include/hw/riscv/virt.h
> > @@ -21,6 +21,7 @@
> >
> >  #include "hw/riscv/riscv_hart.h"
> >  #include "hw/sysbus.h"
> > +#include "hw/block/flash.h"
> >
> >  typedef struct {
> >      /*< private >*/
> > @@ -29,6 +30,7 @@ typedef struct {
> >      /*< public >*/
> >      RISCVHartArrayState soc;
> >      DeviceState *plic;
> > +    PFlashCFI01 *flash[2];
> >      void *fdt;
> >      int fdt_size;
> >  } RISCVVirtState;
> > @@ -41,6 +43,7 @@ enum {
> >      VIRT_PLIC,
> >      VIRT_UART0,
> >      VIRT_VIRTIO,
> > +    VIRT_FLASH,
> >      VIRT_DRAM,
> >      VIRT_PCIE_MMIO,
> >      VIRT_PCIE_PIO,
> > --
>
> Regards,
> Bin
Bin Meng Sept. 22, 2019, 2:15 a.m. UTC | #3
On Sat, Sep 21, 2019 at 6:16 AM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Thu, Sep 19, 2019 at 10:15 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > On Fri, Sep 20, 2019 at 6:36 AM Alistair Francis
> > <alistair.francis@wdc.com> wrote:
> > >
> > > Add the CFI01 PFlash to the RISC-V virt board. This is the same PFlash
> > > from the ARM Virt board and the implementation is based on the ARM Virt
> > > board. This allows users to specify flash files from the command line.
> > >
> > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > > ---
> > >  hw/riscv/Kconfig        |  1 +
> > >  hw/riscv/virt.c         | 81 +++++++++++++++++++++++++++++++++++++++++
> > >  include/hw/riscv/virt.h |  3 ++
> > >  3 files changed, 85 insertions(+)
> > >
> > > diff --git a/hw/riscv/Kconfig b/hw/riscv/Kconfig
> > > index fb19b2df3a..b12660b9f8 100644
> > > --- a/hw/riscv/Kconfig
> > > +++ b/hw/riscv/Kconfig
> > > @@ -36,4 +36,5 @@ config RISCV_VIRT
> > >      select SERIAL
> > >      select VIRTIO_MMIO
> > >      select PCI_EXPRESS_GENERIC_BRIDGE
> > > +    select PFLASH_CFI01
> > >      select SIFIVE
> > > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> > > index d36f5625ec..ca002ecea7 100644
> > > --- a/hw/riscv/virt.c
> > > +++ b/hw/riscv/virt.c
> > > @@ -26,6 +26,7 @@
> > >  #include "hw/boards.h"
> > >  #include "hw/loader.h"
> > >  #include "hw/sysbus.h"
> > > +#include "hw/qdev-properties.h"
> > >  #include "hw/char/serial.h"
> > >  #include "target/riscv/cpu.h"
> > >  #include "hw/riscv/riscv_hart.h"
> > > @@ -61,12 +62,72 @@ static const struct MemmapEntry {
> > >      [VIRT_PLIC] =        {  0xc000000,     0x4000000 },
> > >      [VIRT_UART0] =       { 0x10000000,         0x100 },
> > >      [VIRT_VIRTIO] =      { 0x10001000,        0x1000 },
> > > +    [VIRT_FLASH] =       { 0x20000000,     0x2000000 },
> > >      [VIRT_DRAM] =        { 0x80000000,           0x0 },
> > >      [VIRT_PCIE_MMIO] =   { 0x40000000,    0x40000000 },
> > >      [VIRT_PCIE_PIO] =    { 0x03000000,    0x00010000 },
> > >      [VIRT_PCIE_ECAM] =   { 0x30000000,    0x10000000 },
> > >  };
> > >
> > > +#define VIRT_FLASH_SECTOR_SIZE (256 * KiB)
> > > +
> > > +static PFlashCFI01 *virt_flash_create1(RISCVVirtState *s,
> > > +                                       const char *name,
> > > +                                       const char *alias_prop_name)
> > > +{
> > > +    /*
> > > +     * Create a single flash device.  We use the same parameters as
> > > +     * the flash devices on the ARM virt board.
> > > +     */
> > > +    DeviceState *dev = qdev_create(NULL, TYPE_PFLASH_CFI01);
> > > +
> > > +    qdev_prop_set_uint64(dev, "sector-length", VIRT_FLASH_SECTOR_SIZE);
> > > +    qdev_prop_set_uint8(dev, "width", 4);
> > > +    qdev_prop_set_uint8(dev, "device-width", 2);
> > > +    qdev_prop_set_bit(dev, "big-endian", false);
> > > +    qdev_prop_set_uint16(dev, "id0", 0x89);
> > > +    qdev_prop_set_uint16(dev, "id1", 0x18);
> > > +    qdev_prop_set_uint16(dev, "id2", 0x00);
> > > +    qdev_prop_set_uint16(dev, "id3", 0x00);
> > > +    qdev_prop_set_string(dev, "name", name);
> >
> > alias_prop_name is unused? ARM virt has 2 more calls in the same function here.
>
> Yep, you are right. I have removed this.

Any reason of removing this?

>
> >
> > > +
> > > +    return PFLASH_CFI01(dev);
> > > +}
> > > +
> > > +static void virt_flash_create(RISCVVirtState *s)
> > > +{
> > > +    s->flash[0] = virt_flash_create1(s, "virt.flash0", "pflash0");
> > > +    s->flash[1] = virt_flash_create1(s, "virt.flash1", "pflash1");
> >
> > I don't think we should mirror what is used on ARM virt board to
> > create 2 flash for sifive_u. For ARM virt, there are 2 flashes because
> > they need distinguish secure and non-secure. For sifive_u, only one is
> > enough.
>
> I went back and forward about 1 or 2. Two seems more usable as maybe
> someone wants to include two pflash files? The Xilinx machine also has
> two so I'm kind of used to 2, but I'm not really fussed.
>
> Unless anyone else wants two I will change it to 1.

Regards,
Bin
Alistair Francis Sept. 23, 2019, 8:08 p.m. UTC | #4
On Sat, Sep 21, 2019 at 7:15 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Sat, Sep 21, 2019 at 6:16 AM Alistair Francis <alistair23@gmail.com> wrote:
> >
> > On Thu, Sep 19, 2019 at 10:15 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> > >
> > > On Fri, Sep 20, 2019 at 6:36 AM Alistair Francis
> > > <alistair.francis@wdc.com> wrote:
> > > >
> > > > Add the CFI01 PFlash to the RISC-V virt board. This is the same PFlash
> > > > from the ARM Virt board and the implementation is based on the ARM Virt
> > > > board. This allows users to specify flash files from the command line.
> > > >
> > > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > > > ---
> > > >  hw/riscv/Kconfig        |  1 +
> > > >  hw/riscv/virt.c         | 81 +++++++++++++++++++++++++++++++++++++++++
> > > >  include/hw/riscv/virt.h |  3 ++
> > > >  3 files changed, 85 insertions(+)
> > > >
> > > > diff --git a/hw/riscv/Kconfig b/hw/riscv/Kconfig
> > > > index fb19b2df3a..b12660b9f8 100644
> > > > --- a/hw/riscv/Kconfig
> > > > +++ b/hw/riscv/Kconfig
> > > > @@ -36,4 +36,5 @@ config RISCV_VIRT
> > > >      select SERIAL
> > > >      select VIRTIO_MMIO
> > > >      select PCI_EXPRESS_GENERIC_BRIDGE
> > > > +    select PFLASH_CFI01
> > > >      select SIFIVE
> > > > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> > > > index d36f5625ec..ca002ecea7 100644
> > > > --- a/hw/riscv/virt.c
> > > > +++ b/hw/riscv/virt.c
> > > > @@ -26,6 +26,7 @@
> > > >  #include "hw/boards.h"
> > > >  #include "hw/loader.h"
> > > >  #include "hw/sysbus.h"
> > > > +#include "hw/qdev-properties.h"
> > > >  #include "hw/char/serial.h"
> > > >  #include "target/riscv/cpu.h"
> > > >  #include "hw/riscv/riscv_hart.h"
> > > > @@ -61,12 +62,72 @@ static const struct MemmapEntry {
> > > >      [VIRT_PLIC] =        {  0xc000000,     0x4000000 },
> > > >      [VIRT_UART0] =       { 0x10000000,         0x100 },
> > > >      [VIRT_VIRTIO] =      { 0x10001000,        0x1000 },
> > > > +    [VIRT_FLASH] =       { 0x20000000,     0x2000000 },
> > > >      [VIRT_DRAM] =        { 0x80000000,           0x0 },
> > > >      [VIRT_PCIE_MMIO] =   { 0x40000000,    0x40000000 },
> > > >      [VIRT_PCIE_PIO] =    { 0x03000000,    0x00010000 },
> > > >      [VIRT_PCIE_ECAM] =   { 0x30000000,    0x10000000 },
> > > >  };
> > > >
> > > > +#define VIRT_FLASH_SECTOR_SIZE (256 * KiB)
> > > > +
> > > > +static PFlashCFI01 *virt_flash_create1(RISCVVirtState *s,
> > > > +                                       const char *name,
> > > > +                                       const char *alias_prop_name)
> > > > +{
> > > > +    /*
> > > > +     * Create a single flash device.  We use the same parameters as
> > > > +     * the flash devices on the ARM virt board.
> > > > +     */
> > > > +    DeviceState *dev = qdev_create(NULL, TYPE_PFLASH_CFI01);
> > > > +
> > > > +    qdev_prop_set_uint64(dev, "sector-length", VIRT_FLASH_SECTOR_SIZE);
> > > > +    qdev_prop_set_uint8(dev, "width", 4);
> > > > +    qdev_prop_set_uint8(dev, "device-width", 2);
> > > > +    qdev_prop_set_bit(dev, "big-endian", false);
> > > > +    qdev_prop_set_uint16(dev, "id0", 0x89);
> > > > +    qdev_prop_set_uint16(dev, "id1", 0x18);
> > > > +    qdev_prop_set_uint16(dev, "id2", 0x00);
> > > > +    qdev_prop_set_uint16(dev, "id3", 0x00);
> > > > +    qdev_prop_set_string(dev, "name", name);
> > >
> > > alias_prop_name is unused? ARM virt has 2 more calls in the same function here.
> >
> > Yep, you are right. I have removed this.
>
> Any reason of removing this?

The way the virt machine was structured didn't work with the options.
I have fixed that in v2 of the series.

Alistair

>
> >
> > >
> > > > +
> > > > +    return PFLASH_CFI01(dev);
> > > > +}
> > > > +
> > > > +static void virt_flash_create(RISCVVirtState *s)
> > > > +{
> > > > +    s->flash[0] = virt_flash_create1(s, "virt.flash0", "pflash0");
> > > > +    s->flash[1] = virt_flash_create1(s, "virt.flash1", "pflash1");
> > >
> > > I don't think we should mirror what is used on ARM virt board to
> > > create 2 flash for sifive_u. For ARM virt, there are 2 flashes because
> > > they need distinguish secure and non-secure. For sifive_u, only one is
> > > enough.
> >
> > I went back and forward about 1 or 2. Two seems more usable as maybe
> > someone wants to include two pflash files? The Xilinx machine also has
> > two so I'm kind of used to 2, but I'm not really fussed.
> >
> > Unless anyone else wants two I will change it to 1.
>
> Regards,
> Bin
Peter Maydell Sept. 23, 2019, 9:46 p.m. UTC | #5
On Fri, 20 Sep 2019 at 23:23, Alistair Francis <alistair23@gmail.com> wrote:
> On Thu, Sep 19, 2019 at 10:15 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> > I don't think we should mirror what is used on ARM virt board to
> > create 2 flash for sifive_u. For ARM virt, there are 2 flashes because
> > they need distinguish secure and non-secure. For sifive_u, only one is
> > enough.
>
> I went back and forward about 1 or 2. Two seems more usable as maybe
> someone wants to include two pflash files? The Xilinx machine also has
> two so I'm kind of used to 2, but I'm not really fussed.

One of the reasons for having 2 on the Arm board (we do this
even if we're not supporting secure vs non-secure) is that
then you can use one for a fixed read-only BIOS image (backed
by a file on the host filesystem shared between all VMs), and
one backed by a read-write per-VM file providing permanent
storage for BIOS environment variables. Notably UEFI likes to
work this way, but the idea applies in theory to other
boot loader or BIOSes I guess.

I would suggest also checking with Markus that your code
for instantiating the flash devices follows the current
recommendations so the backing storage can be configured
via -blockdev. (This is a fairly recent change from June or
so; current-in-master virt and sbsa boards provide an example
of doing the right thing, I think.)

thanks
-- PMM
Philippe Mathieu-Daudé Sept. 24, 2019, 9:32 a.m. UTC | #6
On 9/23/19 11:46 PM, Peter Maydell wrote:
> On Fri, 20 Sep 2019 at 23:23, Alistair Francis <alistair23@gmail.com> wrote:
>> On Thu, Sep 19, 2019 at 10:15 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>>> I don't think we should mirror what is used on ARM virt board to
>>> create 2 flash for sifive_u. For ARM virt, there are 2 flashes because
>>> they need distinguish secure and non-secure. For sifive_u, only one is
>>> enough.
>>
>> I went back and forward about 1 or 2. Two seems more usable as maybe
>> someone wants to include two pflash files? The Xilinx machine also has
>> two so I'm kind of used to 2, but I'm not really fussed.

The Xilinx machine has 2 because it matches the hardware.

> One of the reasons for having 2 on the Arm board (we do this
> even if we're not supporting secure vs non-secure) is that
> then you can use one for a fixed read-only BIOS image (backed
> by a file on the host filesystem shared between all VMs), and
> one backed by a read-write per-VM file providing permanent
> storage for BIOS environment variables. Notably UEFI likes to
> work this way, but the idea applies in theory to other
> boot loader or BIOSes I guess.

IIRC Laszlo's explanation, the only reason it is that way is because the
pflash_cfi01 model still doesn't implement sector locking. At the OVMF
emerged from EDK2, to have a safe ROM vs DATA storage it was decided to
use 2 different flashes.
When I understood when this config layout started, I suggested Laszlo to
use a real ROM to store the OVMF CODE since it is pointless to do
firmware upgrade in virtualized environment, but he said it was too late
to change the design.

If you don't plan to run UEFI "Capsule Update" on your Virt board, I
suggest using memory_region_init_rom() with your firmware CODE, and a
parallel/SPI flash for the data VARStore.

> I would suggest also checking with Markus that your code
> for instantiating the flash devices follows the current
> recommendations so the backing storage can be configured
> via -blockdev. (This is a fairly recent change from June or
> so; current-in-master virt and sbsa boards provide an example
> of doing the right thing, I think.)
> 
> thanks
> -- PMM
>
Laszlo Ersek Sept. 24, 2019, 5:12 p.m. UTC | #7
On 09/24/19 11:32, Philippe Mathieu-Daudé wrote:
> On 9/23/19 11:46 PM, Peter Maydell wrote:
>> On Fri, 20 Sep 2019 at 23:23, Alistair Francis <alistair23@gmail.com> wrote:
>>> On Thu, Sep 19, 2019 at 10:15 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>>>> I don't think we should mirror what is used on ARM virt board to
>>>> create 2 flash for sifive_u. For ARM virt, there are 2 flashes because
>>>> they need distinguish secure and non-secure. For sifive_u, only one is
>>>> enough.
>>>
>>> I went back and forward about 1 or 2. Two seems more usable as maybe
>>> someone wants to include two pflash files? The Xilinx machine also has
>>> two so I'm kind of used to 2, but I'm not really fussed.
> 
> The Xilinx machine has 2 because it matches the hardware.
> 
>> One of the reasons for having 2 on the Arm board (we do this
>> even if we're not supporting secure vs non-secure) is that
>> then you can use one for a fixed read-only BIOS image (backed
>> by a file on the host filesystem shared between all VMs), and
>> one backed by a read-write per-VM file providing permanent
>> storage for BIOS environment variables. Notably UEFI likes to
>> work this way, but the idea applies in theory to other
>> boot loader or BIOSes I guess.
> 
> IIRC Laszlo's explanation, the only reason it is that way is because the
> pflash_cfi01 model still doesn't implement sector locking. At the OVMF
> emerged from EDK2, to have a safe ROM vs DATA storage it was decided to
> use 2 different flashes.
> When I understood when this config layout started, I suggested Laszlo to
> use a real ROM to store the OVMF CODE since it is pointless to do
> firmware upgrade in virtualized environment, but he said it was too late
> to change the design.

Right, at that point I couldn't see how "-bios" *plus* "-pflash" could
have worked. In chronological order (for OVMF anyway), there was -bios,
then -pflash (R/W), then (with some QEMU changes) two -pflash flags (R/O
+ R/W, which OVMF wouldn't tell apart from the single R/W -pflash).

Thanks
Laszlo

> 
> If you don't plan to run UEFI "Capsule Update" on your Virt board, I
> suggest using memory_region_init_rom() with your firmware CODE, and a
> parallel/SPI flash for the data VARStore.
> 
>> I would suggest also checking with Markus that your code
>> for instantiating the flash devices follows the current
>> recommendations so the backing storage can be configured
>> via -blockdev. (This is a fairly recent change from June or
>> so; current-in-master virt and sbsa boards provide an example
>> of doing the right thing, I think.)
>>
>> thanks
>> -- PMM
>>
Alistair Francis Sept. 25, 2019, 12:54 a.m. UTC | #8
On Mon, Sep 23, 2019 at 2:46 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Fri, 20 Sep 2019 at 23:23, Alistair Francis <alistair23@gmail.com> wrote:
> > On Thu, Sep 19, 2019 at 10:15 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> > > I don't think we should mirror what is used on ARM virt board to
> > > create 2 flash for sifive_u. For ARM virt, there are 2 flashes because
> > > they need distinguish secure and non-secure. For sifive_u, only one is
> > > enough.
> >
> > I went back and forward about 1 or 2. Two seems more usable as maybe
> > someone wants to include two pflash files? The Xilinx machine also has
> > two so I'm kind of used to 2, but I'm not really fussed.
>
> One of the reasons for having 2 on the Arm board (we do this
> even if we're not supporting secure vs non-secure) is that
> then you can use one for a fixed read-only BIOS image (backed
> by a file on the host filesystem shared between all VMs), and
> one backed by a read-write per-VM file providing permanent
> storage for BIOS environment variables. Notably UEFI likes to
> work this way, but the idea applies in theory to other
> boot loader or BIOSes I guess.

This seems like a good reason to have two and there isn't really a
disadvantage so I have kept it with two.

>
> I would suggest also checking with Markus that your code
> for instantiating the flash devices follows the current
> recommendations so the backing storage can be configured
> via -blockdev. (This is a fairly recent change from June or
> so; current-in-master virt and sbsa boards provide an example
> of doing the right thing, I think.)

I have updated the code to more closely match the ARM virt machine, so
I think I'm doing it correctly.

Alistair

>
> thanks
> -- PMM
Alistair Francis Sept. 25, 2019, 12:55 a.m. UTC | #9
On Tue, Sep 24, 2019 at 2:32 AM Philippe Mathieu-Daudé
<philmd@redhat.com> wrote:
>
> On 9/23/19 11:46 PM, Peter Maydell wrote:
> > On Fri, 20 Sep 2019 at 23:23, Alistair Francis <alistair23@gmail.com> wrote:
> >> On Thu, Sep 19, 2019 at 10:15 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >>> I don't think we should mirror what is used on ARM virt board to
> >>> create 2 flash for sifive_u. For ARM virt, there are 2 flashes because
> >>> they need distinguish secure and non-secure. For sifive_u, only one is
> >>> enough.
> >>
> >> I went back and forward about 1 or 2. Two seems more usable as maybe
> >> someone wants to include two pflash files? The Xilinx machine also has
> >> two so I'm kind of used to 2, but I'm not really fussed.
>
> The Xilinx machine has 2 because it matches the hardware.
>
> > One of the reasons for having 2 on the Arm board (we do this
> > even if we're not supporting secure vs non-secure) is that
> > then you can use one for a fixed read-only BIOS image (backed
> > by a file on the host filesystem shared between all VMs), and
> > one backed by a read-write per-VM file providing permanent
> > storage for BIOS environment variables. Notably UEFI likes to
> > work this way, but the idea applies in theory to other
> > boot loader or BIOSes I guess.
>
> IIRC Laszlo's explanation, the only reason it is that way is because the
> pflash_cfi01 model still doesn't implement sector locking. At the OVMF
> emerged from EDK2, to have a safe ROM vs DATA storage it was decided to
> use 2 different flashes.
> When I understood when this config layout started, I suggested Laszlo to
> use a real ROM to store the OVMF CODE since it is pointless to do
> firmware upgrade in virtualized environment, but he said it was too late
> to change the design.
>
> If you don't plan to run UEFI "Capsule Update" on your Virt board, I
> suggest using memory_region_init_rom() with your firmware CODE, and a
> parallel/SPI flash for the data VARStore.

We might run that one day, who knows :)

Alistair

>
> > I would suggest also checking with Markus that your code
> > for instantiating the flash devices follows the current
> > recommendations so the backing storage can be configured
> > via -blockdev. (This is a fairly recent change from June or
> > so; current-in-master virt and sbsa boards provide an example
> > of doing the right thing, I think.)
> >
> > thanks
> > -- PMM
> >
Markus Armbruster Sept. 25, 2019, 9 a.m. UTC | #10
Alistair Francis <alistair23@gmail.com> writes:

> On Mon, Sep 23, 2019 at 2:46 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>>
>> On Fri, 20 Sep 2019 at 23:23, Alistair Francis <alistair23@gmail.com> wrote:
>> > On Thu, Sep 19, 2019 at 10:15 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>> > > I don't think we should mirror what is used on ARM virt board to
>> > > create 2 flash for sifive_u. For ARM virt, there are 2 flashes because
>> > > they need distinguish secure and non-secure. For sifive_u, only one is
>> > > enough.
>> >
>> > I went back and forward about 1 or 2. Two seems more usable as maybe
>> > someone wants to include two pflash files? The Xilinx machine also has
>> > two so I'm kind of used to 2, but I'm not really fussed.
>>
>> One of the reasons for having 2 on the Arm board (we do this
>> even if we're not supporting secure vs non-secure) is that
>> then you can use one for a fixed read-only BIOS image (backed
>> by a file on the host filesystem shared between all VMs), and
>> one backed by a read-write per-VM file providing permanent
>> storage for BIOS environment variables. Notably UEFI likes to
>> work this way, but the idea applies in theory to other
>> boot loader or BIOSes I guess.
>
> This seems like a good reason to have two and there isn't really a
> disadvantage so I have kept it with two.

Good.

Implementing sector locking would be even better.  I'm not asking you to
do that work.

>> I would suggest also checking with Markus that your code
>> for instantiating the flash devices follows the current
>> recommendations so the backing storage can be configured
>> via -blockdev. (This is a fairly recent change from June or
>> so; current-in-master virt and sbsa boards provide an example
>> of doing the right thing, I think.)
>
> I have updated the code to more closely match the ARM virt machine, so
> I think I'm doing it correctly.

You might want to consider omitting legacy configuration options -drive
if=pflash and -bios for a simpler interface.
Philippe Mathieu-Daudé Sept. 25, 2019, 11:15 a.m. UTC | #11
On 9/25/19 2:55 AM, Alistair Francis wrote:
> On Tue, Sep 24, 2019 at 2:32 AM Philippe Mathieu-Daudé
> <philmd@redhat.com> wrote:
>>
>> On 9/23/19 11:46 PM, Peter Maydell wrote:
>>> On Fri, 20 Sep 2019 at 23:23, Alistair Francis <alistair23@gmail.com> wrote:
>>>> On Thu, Sep 19, 2019 at 10:15 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>>>>> I don't think we should mirror what is used on ARM virt board to
>>>>> create 2 flash for sifive_u. For ARM virt, there are 2 flashes because
>>>>> they need distinguish secure and non-secure. For sifive_u, only one is
>>>>> enough.
>>>>
>>>> I went back and forward about 1 or 2. Two seems more usable as maybe
>>>> someone wants to include two pflash files? The Xilinx machine also has
>>>> two so I'm kind of used to 2, but I'm not really fussed.
>>
>> The Xilinx machine has 2 because it matches the hardware.
>>
>>> One of the reasons for having 2 on the Arm board (we do this
>>> even if we're not supporting secure vs non-secure) is that
>>> then you can use one for a fixed read-only BIOS image (backed
>>> by a file on the host filesystem shared between all VMs), and
>>> one backed by a read-write per-VM file providing permanent
>>> storage for BIOS environment variables. Notably UEFI likes to
>>> work this way, but the idea applies in theory to other
>>> boot loader or BIOSes I guess.
>>
>> IIRC Laszlo's explanation, the only reason it is that way is because the
>> pflash_cfi01 model still doesn't implement sector locking. At the OVMF
>> emerged from EDK2, to have a safe ROM vs DATA storage it was decided to
>> use 2 different flashes.
>> When I understood when this config layout started, I suggested Laszlo to
>> use a real ROM to store the OVMF CODE since it is pointless to do
>> firmware upgrade in virtualized environment, but he said it was too late
>> to change the design.
>>
>> If you don't plan to run UEFI "Capsule Update" on your Virt board, I
>> suggest using memory_region_init_rom() with your firmware CODE, and a
>> parallel/SPI flash for the data VARStore.
> 
> We might run that one day, who knows :)

You certainly want to run EDK2, I'm following RISCV progress on the list.

What doesn't make sense on a virtualized platform is the "Capsule
Update" feature IMO. Where it makes sense is on the SiFive E/U boards
models.

>>> I would suggest also checking with Markus that your code
>>> for instantiating the flash devices follows the current
>>> recommendations so the backing storage can be configured
>>> via -blockdev. (This is a fairly recent change from June or
>>> so; current-in-master virt and sbsa boards provide an example
>>> of doing the right thing, I think.)
>>>
>>> thanks
>>> -- PMM
>>>
Alistair Francis Sept. 27, 2019, 9:49 p.m. UTC | #12
On Wed, Sep 25, 2019 at 2:00 AM Markus Armbruster <armbru@redhat.com> wrote:
>
> Alistair Francis <alistair23@gmail.com> writes:
>
> > On Mon, Sep 23, 2019 at 2:46 PM Peter Maydell <peter.maydell@linaro.org> wrote:
> >>
> >> On Fri, 20 Sep 2019 at 23:23, Alistair Francis <alistair23@gmail.com> wrote:
> >> > On Thu, Sep 19, 2019 at 10:15 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >> > > I don't think we should mirror what is used on ARM virt board to
> >> > > create 2 flash for sifive_u. For ARM virt, there are 2 flashes because
> >> > > they need distinguish secure and non-secure. For sifive_u, only one is
> >> > > enough.
> >> >
> >> > I went back and forward about 1 or 2. Two seems more usable as maybe
> >> > someone wants to include two pflash files? The Xilinx machine also has
> >> > two so I'm kind of used to 2, but I'm not really fussed.
> >>
> >> One of the reasons for having 2 on the Arm board (we do this
> >> even if we're not supporting secure vs non-secure) is that
> >> then you can use one for a fixed read-only BIOS image (backed
> >> by a file on the host filesystem shared between all VMs), and
> >> one backed by a read-write per-VM file providing permanent
> >> storage for BIOS environment variables. Notably UEFI likes to
> >> work this way, but the idea applies in theory to other
> >> boot loader or BIOSes I guess.
> >
> > This seems like a good reason to have two and there isn't really a
> > disadvantage so I have kept it with two.
>
> Good.
>
> Implementing sector locking would be even better.  I'm not asking you to
> do that work.
>
> >> I would suggest also checking with Markus that your code
> >> for instantiating the flash devices follows the current
> >> recommendations so the backing storage can be configured
> >> via -blockdev. (This is a fairly recent change from June or
> >> so; current-in-master virt and sbsa boards provide an example
> >> of doing the right thing, I think.)
> >
> > I have updated the code to more closely match the ARM virt machine, so
> > I think I'm doing it correctly.
>
> You might want to consider omitting legacy configuration options -drive
> if=pflash and -bios for a simpler interface.

We just moved to -bios and it's been really helpful.

Doesn't -pflash use -drive if=pflash? How else should these be attached?

Alistair
diff mbox series

Patch

diff --git a/hw/riscv/Kconfig b/hw/riscv/Kconfig
index fb19b2df3a..b12660b9f8 100644
--- a/hw/riscv/Kconfig
+++ b/hw/riscv/Kconfig
@@ -36,4 +36,5 @@  config RISCV_VIRT
     select SERIAL
     select VIRTIO_MMIO
     select PCI_EXPRESS_GENERIC_BRIDGE
+    select PFLASH_CFI01
     select SIFIVE
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index d36f5625ec..ca002ecea7 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -26,6 +26,7 @@ 
 #include "hw/boards.h"
 #include "hw/loader.h"
 #include "hw/sysbus.h"
+#include "hw/qdev-properties.h"
 #include "hw/char/serial.h"
 #include "target/riscv/cpu.h"
 #include "hw/riscv/riscv_hart.h"
@@ -61,12 +62,72 @@  static const struct MemmapEntry {
     [VIRT_PLIC] =        {  0xc000000,     0x4000000 },
     [VIRT_UART0] =       { 0x10000000,         0x100 },
     [VIRT_VIRTIO] =      { 0x10001000,        0x1000 },
+    [VIRT_FLASH] =       { 0x20000000,     0x2000000 },
     [VIRT_DRAM] =        { 0x80000000,           0x0 },
     [VIRT_PCIE_MMIO] =   { 0x40000000,    0x40000000 },
     [VIRT_PCIE_PIO] =    { 0x03000000,    0x00010000 },
     [VIRT_PCIE_ECAM] =   { 0x30000000,    0x10000000 },
 };
 
+#define VIRT_FLASH_SECTOR_SIZE (256 * KiB)
+
+static PFlashCFI01 *virt_flash_create1(RISCVVirtState *s,
+                                       const char *name,
+                                       const char *alias_prop_name)
+{
+    /*
+     * Create a single flash device.  We use the same parameters as
+     * the flash devices on the ARM virt board.
+     */
+    DeviceState *dev = qdev_create(NULL, TYPE_PFLASH_CFI01);
+
+    qdev_prop_set_uint64(dev, "sector-length", VIRT_FLASH_SECTOR_SIZE);
+    qdev_prop_set_uint8(dev, "width", 4);
+    qdev_prop_set_uint8(dev, "device-width", 2);
+    qdev_prop_set_bit(dev, "big-endian", false);
+    qdev_prop_set_uint16(dev, "id0", 0x89);
+    qdev_prop_set_uint16(dev, "id1", 0x18);
+    qdev_prop_set_uint16(dev, "id2", 0x00);
+    qdev_prop_set_uint16(dev, "id3", 0x00);
+    qdev_prop_set_string(dev, "name", name);
+
+    return PFLASH_CFI01(dev);
+}
+
+static void virt_flash_create(RISCVVirtState *s)
+{
+    s->flash[0] = virt_flash_create1(s, "virt.flash0", "pflash0");
+    s->flash[1] = virt_flash_create1(s, "virt.flash1", "pflash1");
+}
+
+static void virt_flash_map1(PFlashCFI01 *flash,
+                            hwaddr base, hwaddr size,
+                            MemoryRegion *sysmem)
+{
+    DeviceState *dev = DEVICE(flash);
+
+    assert(size % VIRT_FLASH_SECTOR_SIZE == 0);
+    assert(size / VIRT_FLASH_SECTOR_SIZE <= UINT32_MAX);
+    qdev_prop_set_uint32(dev, "num-blocks", size / VIRT_FLASH_SECTOR_SIZE);
+    qdev_init_nofail(dev);
+
+    memory_region_add_subregion(sysmem, base,
+                                sysbus_mmio_get_region(SYS_BUS_DEVICE(dev),
+                                                       0));
+}
+
+static void virt_flash_map(RISCVVirtState *s,
+                           MemoryRegion *sysmem)
+{
+    hwaddr flashsize = virt_memmap[VIRT_FLASH].size / 2;
+    hwaddr flashbase = virt_memmap[VIRT_FLASH].base;
+
+    virt_flash_map1(s->flash[0], flashbase, flashsize,
+                    sysmem);
+    virt_flash_map1(s->flash[1], flashbase + flashsize, flashsize,
+                    sysmem);
+}
+
 static void create_pcie_irq_map(void *fdt, char *nodename,
                                 uint32_t plic_phandle)
 {
@@ -121,6 +182,8 @@  static void create_fdt(RISCVVirtState *s, const struct MemmapEntry *memmap,
     char *nodename;
     uint32_t plic_phandle, phandle = 1;
     int i;
+    hwaddr flashsize = virt_memmap[VIRT_FLASH].size / 2;
+    hwaddr flashbase = virt_memmap[VIRT_FLASH].base;
 
     fdt = s->fdt = create_device_tree(&s->fdt_size);
     if (!fdt) {
@@ -316,6 +379,15 @@  static void create_fdt(RISCVVirtState *s, const struct MemmapEntry *memmap,
         qemu_fdt_setprop_string(fdt, "/chosen", "bootargs", cmdline);
     }
     g_free(nodename);
+
+    nodename = g_strdup_printf("/flash@%" PRIx64, flashbase);
+    qemu_fdt_add_subnode(s->fdt, nodename);
+    qemu_fdt_setprop_string(s->fdt, nodename, "compatible", "cfi-flash");
+    qemu_fdt_setprop_sized_cells(s->fdt, nodename, "reg",
+                                 2, flashbase, 2, flashsize,
+                                 2, flashbase + flashsize, 2, flashsize);
+    qemu_fdt_setprop_cell(s->fdt, nodename, "bank-width", 4);
+    g_free(nodename);
 }
 
 
@@ -496,6 +568,15 @@  static void riscv_virt_board_init(MachineState *machine)
         0, qdev_get_gpio_in(DEVICE(s->plic), UART0_IRQ), 399193,
         serial_hd(0), DEVICE_LITTLE_ENDIAN);
 
+    virt_flash_create(s);
+
+    /* Map legacy -drive if=pflash to machine properties */
+    for (i = 0; i < ARRAY_SIZE(s->flash); i++) {
+        pflash_cfi01_legacy_drive(s->flash[i],
+                                  drive_get(IF_PFLASH, 0, i));
+    }
+    virt_flash_map(s, system_memory);
+
     g_free(plic_hart_config);
 }
 
diff --git a/include/hw/riscv/virt.h b/include/hw/riscv/virt.h
index 6e5fbe5d3b..2ca8bd3512 100644
--- a/include/hw/riscv/virt.h
+++ b/include/hw/riscv/virt.h
@@ -21,6 +21,7 @@ 
 
 #include "hw/riscv/riscv_hart.h"
 #include "hw/sysbus.h"
+#include "hw/block/flash.h"
 
 typedef struct {
     /*< private >*/
@@ -29,6 +30,7 @@  typedef struct {
     /*< public >*/
     RISCVHartArrayState soc;
     DeviceState *plic;
+    PFlashCFI01 *flash[2];
     void *fdt;
     int fdt_size;
 } RISCVVirtState;
@@ -41,6 +43,7 @@  enum {
     VIRT_PLIC,
     VIRT_UART0,
     VIRT_VIRTIO,
+    VIRT_FLASH,
     VIRT_DRAM,
     VIRT_PCIE_MMIO,
     VIRT_PCIE_PIO,