[PATCH] hw/riscv: virt: Remove size restriction for pflash

[PATCH] hw/riscv: virt: Remove size restriction for pflash

[Sunil V L]

The pflash implementation currently assumes fixed size of the
backend storage. Due to this, the backend storage file needs to be
exactly of size 32M. Otherwise, there will be an error like below.

"device requires 33554432 bytes, block backend provides 3145728 bytes"

Fix this issue by using the actual size of the backing store.

Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
---
 hw/riscv/virt.c | 33 +++++++++++++++++++++++++--------
 1 file changed, 25 insertions(+), 8 deletions(-)

diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index a5bc7353b4..aad175fa31 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -49,6 +49,7 @@
 #include "hw/pci/pci.h"
 #include "hw/pci-host/gpex.h"
 #include "hw/display/ramfb.h"
+#include "sysemu/block-backend.h"
 
 /*
  * The virt machine physical address space used by some of the devices
@@ -144,10 +145,17 @@ static void virt_flash_map1(PFlashCFI01 *flash,
                             MemoryRegion *sysmem)
 {
     DeviceState *dev = DEVICE(flash);
+    BlockBackend *blk;
+    hwaddr real_size;
 
-    assert(QEMU_IS_ALIGNED(size, VIRT_FLASH_SECTOR_SIZE));
-    assert(size / VIRT_FLASH_SECTOR_SIZE <= UINT32_MAX);
-    qdev_prop_set_uint32(dev, "num-blocks", size / VIRT_FLASH_SECTOR_SIZE);
+    blk = pflash_cfi01_get_blk(flash);
+
+    real_size = blk ? blk_getlength(blk): size;
+
+    assert(real_size);
+    assert(QEMU_IS_ALIGNED(real_size, VIRT_FLASH_SECTOR_SIZE));
+    assert(real_size / VIRT_FLASH_SECTOR_SIZE <= UINT32_MAX);
+    qdev_prop_set_uint32(dev, "num-blocks", real_size / VIRT_FLASH_SECTOR_SIZE);
     sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
 
     memory_region_add_subregion(sysmem, base,
@@ -971,15 +979,24 @@ static void create_fdt_flash(RISCVVirtState *s, const MemMapEntry *memmap)
 {
     char *name;
     MachineState *mc = MACHINE(s);
-    hwaddr flashsize = virt_memmap[VIRT_FLASH].size / 2;
-    hwaddr flashbase = virt_memmap[VIRT_FLASH].base;
+    MemoryRegion *flash_mem;
+    hwaddr flashsize[2];
+    hwaddr flashbase[2];
+
+    flash_mem = pflash_cfi01_get_memory(s->flash[0]);
+    flashbase[0] = flash_mem->addr;
+    flashsize[0] = flash_mem->size;
+
+    flash_mem = pflash_cfi01_get_memory(s->flash[1]);
+    flashbase[1] = flash_mem->addr;
+    flashsize[1] = flash_mem->size;
 
-    name = g_strdup_printf("/flash@%" PRIx64, flashbase);
+    name = g_strdup_printf("/flash@%" PRIx64, flashbase[0]);
     qemu_fdt_add_subnode(mc->fdt, name);
     qemu_fdt_setprop_string(mc->fdt, name, "compatible", "cfi-flash");
     qemu_fdt_setprop_sized_cells(mc->fdt, name, "reg",
-                                 2, flashbase, 2, flashsize,
-                                 2, flashbase + flashsize, 2, flashsize);
+                                 2, flashbase[0], 2, flashsize[0],
+                                 2, flashbase[1], 2, flashsize[1]);
     qemu_fdt_setprop_cell(mc->fdt, name, "bank-width", 4);
     g_free(name);
 }
-- 
2.38.0

Re: [PATCH] hw/riscv: virt: Remove size restriction for pflash

[Andrew Jones]

On Sun, Nov 06, 2022 at 08:09:00PM +0530, Sunil V L wrote:
> The pflash implementation currently assumes fixed size of the
> backend storage. Due to this, the backend storage file needs to be
> exactly of size 32M. Otherwise, there will be an error like below.
> 
> "device requires 33554432 bytes, block backend provides 3145728 bytes"
> 
> Fix this issue by using the actual size of the backing store.
> 
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> ---
>  hw/riscv/virt.c | 33 +++++++++++++++++++++++++--------
>  1 file changed, 25 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index a5bc7353b4..aad175fa31 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -49,6 +49,7 @@
>  #include "hw/pci/pci.h"
>  #include "hw/pci-host/gpex.h"
>  #include "hw/display/ramfb.h"
> +#include "sysemu/block-backend.h"
>  
>  /*
>   * The virt machine physical address space used by some of the devices
> @@ -144,10 +145,17 @@ static void virt_flash_map1(PFlashCFI01 *flash,
>                              MemoryRegion *sysmem)
>  {
>      DeviceState *dev = DEVICE(flash);
> +    BlockBackend *blk;
> +    hwaddr real_size;
>  
> -    assert(QEMU_IS_ALIGNED(size, VIRT_FLASH_SECTOR_SIZE));
> -    assert(size / VIRT_FLASH_SECTOR_SIZE <= UINT32_MAX);
> -    qdev_prop_set_uint32(dev, "num-blocks", size / VIRT_FLASH_SECTOR_SIZE);
> +    blk = pflash_cfi01_get_blk(flash);
> +
> +    real_size = blk ? blk_getlength(blk): size;
> +
> +    assert(real_size);
> +    assert(QEMU_IS_ALIGNED(real_size, VIRT_FLASH_SECTOR_SIZE));
> +    assert(real_size / VIRT_FLASH_SECTOR_SIZE <= UINT32_MAX);
> +    qdev_prop_set_uint32(dev, "num-blocks", real_size / VIRT_FLASH_SECTOR_SIZE);
>      sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>  
>      memory_region_add_subregion(sysmem, base,
> @@ -971,15 +979,24 @@ static void create_fdt_flash(RISCVVirtState *s, const MemMapEntry *memmap)
>  {
>      char *name;
>      MachineState *mc = MACHINE(s);
> -    hwaddr flashsize = virt_memmap[VIRT_FLASH].size / 2;
> -    hwaddr flashbase = virt_memmap[VIRT_FLASH].base;
> +    MemoryRegion *flash_mem;
> +    hwaddr flashsize[2];
> +    hwaddr flashbase[2];
> +
> +    flash_mem = pflash_cfi01_get_memory(s->flash[0]);
> +    flashbase[0] = flash_mem->addr;
> +    flashsize[0] = flash_mem->size;
> +
> +    flash_mem = pflash_cfi01_get_memory(s->flash[1]);
> +    flashbase[1] = flash_mem->addr;
> +    flashsize[1] = flash_mem->size;
>  
> -    name = g_strdup_printf("/flash@%" PRIx64, flashbase);
> +    name = g_strdup_printf("/flash@%" PRIx64, flashbase[0]);
>      qemu_fdt_add_subnode(mc->fdt, name);
>      qemu_fdt_setprop_string(mc->fdt, name, "compatible", "cfi-flash");
>      qemu_fdt_setprop_sized_cells(mc->fdt, name, "reg",
> -                                 2, flashbase, 2, flashsize,
> -                                 2, flashbase + flashsize, 2, flashsize);
> +                                 2, flashbase[0], 2, flashsize[0],
> +                                 2, flashbase[1], 2, flashsize[1]);
>      qemu_fdt_setprop_cell(mc->fdt, name, "bank-width", 4);
>      g_free(name);
>  }
> -- 
> 2.38.0
> 
>

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Re: [PATCH] hw/riscv: virt: Remove size restriction for pflash

[Mike Maslenkin]

Hello Sunil!

What about virt_machine_done() function?
kernel_entry variable still points to the second flash started from
virt_memmap[VIRT_FLASH].size / 2.

On Sun, Nov 6, 2022 at 5:41 PM Sunil V L <sunilvl@ventanamicro.com> wrote:
>
> The pflash implementation currently assumes fixed size of the
> backend storage. Due to this, the backend storage file needs to be
> exactly of size 32M. Otherwise, there will be an error like below.
>
> "device requires 33554432 bytes, block backend provides 3145728 bytes"
>
> Fix this issue by using the actual size of the backing store.
>
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> ---
>  hw/riscv/virt.c | 33 +++++++++++++++++++++++++--------
>  1 file changed, 25 insertions(+), 8 deletions(-)
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index a5bc7353b4..aad175fa31 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -49,6 +49,7 @@
>  #include "hw/pci/pci.h"
>  #include "hw/pci-host/gpex.h"
>  #include "hw/display/ramfb.h"
> +#include "sysemu/block-backend.h"
>
>  /*
>   * The virt machine physical address space used by some of the devices
> @@ -144,10 +145,17 @@ static void virt_flash_map1(PFlashCFI01 *flash,
>                              MemoryRegion *sysmem)
>  {
>      DeviceState *dev = DEVICE(flash);
> +    BlockBackend *blk;
> +    hwaddr real_size;
>
> -    assert(QEMU_IS_ALIGNED(size, VIRT_FLASH_SECTOR_SIZE));
> -    assert(size / VIRT_FLASH_SECTOR_SIZE <= UINT32_MAX);
> -    qdev_prop_set_uint32(dev, "num-blocks", size / VIRT_FLASH_SECTOR_SIZE);
> +    blk = pflash_cfi01_get_blk(flash);
> +
> +    real_size = blk ? blk_getlength(blk): size;
> +
> +    assert(real_size);
> +    assert(QEMU_IS_ALIGNED(real_size, VIRT_FLASH_SECTOR_SIZE));
> +    assert(real_size / VIRT_FLASH_SECTOR_SIZE <= UINT32_MAX);
> +    qdev_prop_set_uint32(dev, "num-blocks", real_size / VIRT_FLASH_SECTOR_SIZE);
>      sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>
>      memory_region_add_subregion(sysmem, base,
> @@ -971,15 +979,24 @@ static void create_fdt_flash(RISCVVirtState *s, const MemMapEntry *memmap)
>  {
>      char *name;
>      MachineState *mc = MACHINE(s);
> -    hwaddr flashsize = virt_memmap[VIRT_FLASH].size / 2;
> -    hwaddr flashbase = virt_memmap[VIRT_FLASH].base;
> +    MemoryRegion *flash_mem;
> +    hwaddr flashsize[2];
> +    hwaddr flashbase[2];
> +
> +    flash_mem = pflash_cfi01_get_memory(s->flash[0]);
> +    flashbase[0] = flash_mem->addr;
> +    flashsize[0] = flash_mem->size;
> +
> +    flash_mem = pflash_cfi01_get_memory(s->flash[1]);
> +    flashbase[1] = flash_mem->addr;
> +    flashsize[1] = flash_mem->size;
>
> -    name = g_strdup_printf("/flash@%" PRIx64, flashbase);
> +    name = g_strdup_printf("/flash@%" PRIx64, flashbase[0]);
>      qemu_fdt_add_subnode(mc->fdt, name);
>      qemu_fdt_setprop_string(mc->fdt, name, "compatible", "cfi-flash");
>      qemu_fdt_setprop_sized_cells(mc->fdt, name, "reg",
> -                                 2, flashbase, 2, flashsize,
> -                                 2, flashbase + flashsize, 2, flashsize);
> +                                 2, flashbase[0], 2, flashsize[0],
> +                                 2, flashbase[1], 2, flashsize[1]);
>      qemu_fdt_setprop_cell(mc->fdt, name, "bank-width", 4);
>      g_free(name);
>  }
> --
> 2.38.0
>
>

Re: [PATCH] hw/riscv: virt: Remove size restriction for pflash

[Sunil V L]

On Sun, Nov 06, 2022 at 10:20:57PM +0300, Mike Maslenkin wrote:
> Hello Sunil!
> 
> What about virt_machine_done() function?
> kernel_entry variable still points to the second flash started from
> virt_memmap[VIRT_FLASH].size / 2.
> 

The base address of the flash has not changed to keep things flexible. So, I
didn't change this portion of the code to keep the changes minimal.

Thanks
Sunil

Re: [PATCH] hw/riscv: virt: Remove size restriction for pflash

[Andrew Jones]

On Mon, Nov 07, 2022 at 11:16:00AM +0530, Sunil V L wrote:
> On Sun, Nov 06, 2022 at 10:20:57PM +0300, Mike Maslenkin wrote:
> > Hello Sunil!
> > 
> > What about virt_machine_done() function?
> > kernel_entry variable still points to the second flash started from
> > virt_memmap[VIRT_FLASH].size / 2.
> > 
> 
> The base address of the flash has not changed to keep things flexible. So, I
> didn't change this portion of the code to keep the changes minimal.

I think we should change virt_machine_done() to be consistent with
create_fdt_flash() and also add a comment in virt_flash_map() explaining
the base addresses are statically determined from virt_memmap[VIRT_FLASH],
but the sizes are variable and determined in virt_flash_map1().

Thanks,
drew

Re: [PATCH] hw/riscv: virt: Remove size restriction for pflash

[Sunil V L]

On Mon, Nov 07, 2022 at 09:48:03AM +0100, Andrew Jones wrote:
> On Mon, Nov 07, 2022 at 11:16:00AM +0530, Sunil V L wrote:
> > On Sun, Nov 06, 2022 at 10:20:57PM +0300, Mike Maslenkin wrote:
> > > Hello Sunil!
> > > 
> > > What about virt_machine_done() function?
> > > kernel_entry variable still points to the second flash started from
> > > virt_memmap[VIRT_FLASH].size / 2.
> > > 
> > 
> > The base address of the flash has not changed to keep things flexible. So, I
> > didn't change this portion of the code to keep the changes minimal.
> 
> I think we should change virt_machine_done() to be consistent with
> create_fdt_flash() and also add a comment in virt_flash_map() explaining
> the base addresses are statically determined from virt_memmap[VIRT_FLASH],
> but the sizes are variable and determined in virt_flash_map1().
> 

Sure Drew. Let me update and send V2.

Thanks
Sunil
> Thanks,
> drew

Re: [PATCH] hw/riscv: virt: Remove size restriction for pflash

[maobibo]

在 2022/11/6 22:39, Sunil V L 写道:
> The pflash implementation currently assumes fixed size of the
> backend storage. Due to this, the backend storage file needs to be
> exactly of size 32M. Otherwise, there will be an error like below.
> 
> "device requires 33554432 bytes, block backend provides 3145728 bytes"
> 
> Fix this issue by using the actual size of the backing store.
> 
> Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> ---
>  hw/riscv/virt.c | 33 +++++++++++++++++++++++++--------
>  1 file changed, 25 insertions(+), 8 deletions(-)
> 
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index a5bc7353b4..aad175fa31 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -49,6 +49,7 @@
>  #include "hw/pci/pci.h"
>  #include "hw/pci-host/gpex.h"
>  #include "hw/display/ramfb.h"
> +#include "sysemu/block-backend.h"
>  
>  /*
>   * The virt machine physical address space used by some of the devices
> @@ -144,10 +145,17 @@ static void virt_flash_map1(PFlashCFI01 *flash,
>                              MemoryRegion *sysmem)
>  {
>      DeviceState *dev = DEVICE(flash);
> +    BlockBackend *blk;
> +    hwaddr real_size;
>  
> -    assert(QEMU_IS_ALIGNED(size, VIRT_FLASH_SECTOR_SIZE));
> -    assert(size / VIRT_FLASH_SECTOR_SIZE <= UINT32_MAX);
> -    qdev_prop_set_uint32(dev, "num-blocks", size / VIRT_FLASH_SECTOR_SIZE);
> +    blk = pflash_cfi01_get_blk(flash);
> +
> +    real_size = blk ? blk_getlength(blk): size;
> +
> +    assert(real_size);
> +    assert(QEMU_IS_ALIGNED(real_size, VIRT_FLASH_SECTOR_SIZE));
> +    assert(real_size / VIRT_FLASH_SECTOR_SIZE <= UINT32_MAX);
How about add one sentence?
       assert(real_size <= size);   

As defined VIRT_FLASH memory space, the total memory space size 64M,
Pflash0/Pflash1 cannot be more than 32M. Supposing real size of pflash0
is 33M, there will be conflict with address space of pflash1.

regards
bibo, mao

> +    qdev_prop_set_uint32(dev, "num-blocks", real_size / VIRT_FLASH_SECTOR_SIZE);
>      sysbus_realize_and_unref(SYS_BUS_DEVICE(dev), &error_fatal);
>  
>      memory_region_add_subregion(sysmem, base,
> @@ -971,15 +979,24 @@ static void create_fdt_flash(RISCVVirtState *s, const MemMapEntry *memmap)
>  {
>      char *name;
>      MachineState *mc = MACHINE(s);
> -    hwaddr flashsize = virt_memmap[VIRT_FLASH].size / 2;
> -    hwaddr flashbase = virt_memmap[VIRT_FLASH].base;
> +    MemoryRegion *flash_mem;
> +    hwaddr flashsize[2];
> +    hwaddr flashbase[2];
> +
> +    flash_mem = pflash_cfi01_get_memory(s->flash[0]);
> +    flashbase[0] = flash_mem->addr;
> +    flashsize[0] = flash_mem->size;
> +
> +    flash_mem = pflash_cfi01_get_memory(s->flash[1]);
> +    flashbase[1] = flash_mem->addr;
> +    flashsize[1] = flash_mem->size;
>  
> -    name = g_strdup_printf("/flash@%" PRIx64, flashbase);
> +    name = g_strdup_printf("/flash@%" PRIx64, flashbase[0]);
>      qemu_fdt_add_subnode(mc->fdt, name);
>      qemu_fdt_setprop_string(mc->fdt, name, "compatible", "cfi-flash");
>      qemu_fdt_setprop_sized_cells(mc->fdt, name, "reg",
> -                                 2, flashbase, 2, flashsize,
> -                                 2, flashbase + flashsize, 2, flashsize);
> +                                 2, flashbase[0], 2, flashsize[0],
> +                                 2, flashbase[1], 2, flashsize[1]);
>      qemu_fdt_setprop_cell(mc->fdt, name, "bank-width", 4);
>      g_free(name);
>  }

Re: [PATCH] hw/riscv: virt: Remove size restriction for pflash

[Sunil V L]

On Mon, Nov 07, 2022 at 05:57:45PM +0800, maobibo wrote:
> 
> 
> 在 2022/11/6 22:39, Sunil V L 写道:
> > The pflash implementation currently assumes fixed size of the
> > backend storage. Due to this, the backend storage file needs to be
> > exactly of size 32M. Otherwise, there will be an error like below.
> > 
> > "device requires 33554432 bytes, block backend provides 3145728 bytes"
> > 
> > Fix this issue by using the actual size of the backing store.
> > 
> > Signed-off-by: Sunil V L <sunilvl@ventanamicro.com>
> > ---
> >  hw/riscv/virt.c | 33 +++++++++++++++++++++++++--------
> >  1 file changed, 25 insertions(+), 8 deletions(-)
> > 
> > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> > index a5bc7353b4..aad175fa31 100644
> > --- a/hw/riscv/virt.c
> > +++ b/hw/riscv/virt.c
> > @@ -49,6 +49,7 @@
> >  #include "hw/pci/pci.h"
> >  #include "hw/pci-host/gpex.h"
> >  #include "hw/display/ramfb.h"
> > +#include "sysemu/block-backend.h"
> >  
> >  /*
> >   * The virt machine physical address space used by some of the devices
> > @@ -144,10 +145,17 @@ static void virt_flash_map1(PFlashCFI01 *flash,
> >                              MemoryRegion *sysmem)
> >  {
> >      DeviceState *dev = DEVICE(flash);
> > +    BlockBackend *blk;
> > +    hwaddr real_size;
> >  
> > -    assert(QEMU_IS_ALIGNED(size, VIRT_FLASH_SECTOR_SIZE));
> > -    assert(size / VIRT_FLASH_SECTOR_SIZE <= UINT32_MAX);
> > -    qdev_prop_set_uint32(dev, "num-blocks", size / VIRT_FLASH_SECTOR_SIZE);
> > +    blk = pflash_cfi01_get_blk(flash);
> > +
> > +    real_size = blk ? blk_getlength(blk): size;
> > +
> > +    assert(real_size);
> > +    assert(QEMU_IS_ALIGNED(real_size, VIRT_FLASH_SECTOR_SIZE));
> > +    assert(real_size / VIRT_FLASH_SECTOR_SIZE <= UINT32_MAX);
> How about add one sentence?
>        assert(real_size <= size);   
> 
> As defined VIRT_FLASH memory space, the total memory space size 64M,
> Pflash0/Pflash1 cannot be more than 32M. Supposing real size of pflash0
> is 33M, there will be conflict with address space of pflash1.
> 
> regards
> bibo, mao
> 

Good catch!. Thank you. Will add it in V2.

Thanks
Sunil