* Re: [PATCH] hw/riscv: virt: Remove size restriction for pflash
2022-11-06 14:39 [PATCH] hw/riscv: virt: Remove size restriction for pflash Sunil V L
@ 2022-11-06 15:07 ` Andrew Jones
2022-11-06 19:20 ` Mike Maslenkin
2022-11-07 9:57 ` maobibo
2 siblings, 0 replies; 8+ messages in thread
From: Andrew Jones @ 2022-11-06 15:07 UTC (permalink / raw)
To: Sunil V L
Cc: Palmer Dabbelt, Alistair Francis, Bin Meng, Gerd Hoffmann,
qemu-riscv, qemu-devel
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>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] hw/riscv: virt: Remove size restriction for pflash
2022-11-06 14:39 [PATCH] hw/riscv: virt: Remove size restriction for pflash Sunil V L
2022-11-06 15:07 ` Andrew Jones
@ 2022-11-06 19:20 ` Mike Maslenkin
2022-11-07 5:46 ` Sunil V L
2022-11-07 9:57 ` maobibo
2 siblings, 1 reply; 8+ messages in thread
From: Mike Maslenkin @ 2022-11-06 19:20 UTC (permalink / raw)
To: Sunil V L
Cc: Palmer Dabbelt, Alistair Francis, Bin Meng, Gerd Hoffmann,
qemu-riscv, qemu-devel
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
>
>
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] hw/riscv: virt: Remove size restriction for pflash
2022-11-06 19:20 ` Mike Maslenkin
@ 2022-11-07 5:46 ` Sunil V L
2022-11-07 8:48 ` Andrew Jones
0 siblings, 1 reply; 8+ messages in thread
From: Sunil V L @ 2022-11-07 5:46 UTC (permalink / raw)
To: Mike Maslenkin
Cc: Palmer Dabbelt, Alistair Francis, Bin Meng, Gerd Hoffmann,
qemu-riscv, qemu-devel, Pawel Polawski
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] hw/riscv: virt: Remove size restriction for pflash
2022-11-07 5:46 ` Sunil V L
@ 2022-11-07 8:48 ` Andrew Jones
2022-11-07 9:08 ` Sunil V L
0 siblings, 1 reply; 8+ messages in thread
From: Andrew Jones @ 2022-11-07 8:48 UTC (permalink / raw)
To: Sunil V L
Cc: Mike Maslenkin, Palmer Dabbelt, Alistair Francis, Bin Meng,
Gerd Hoffmann, qemu-riscv, qemu-devel, Pawel Polawski
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] hw/riscv: virt: Remove size restriction for pflash
2022-11-07 8:48 ` Andrew Jones
@ 2022-11-07 9:08 ` Sunil V L
0 siblings, 0 replies; 8+ messages in thread
From: Sunil V L @ 2022-11-07 9:08 UTC (permalink / raw)
To: Andrew Jones
Cc: Mike Maslenkin, Palmer Dabbelt, Alistair Francis, Bin Meng,
Gerd Hoffmann, qemu-riscv, qemu-devel, Pawel Polawski
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] hw/riscv: virt: Remove size restriction for pflash
2022-11-06 14:39 [PATCH] hw/riscv: virt: Remove size restriction for pflash Sunil V L
2022-11-06 15:07 ` Andrew Jones
2022-11-06 19:20 ` Mike Maslenkin
@ 2022-11-07 9:57 ` maobibo
2022-11-07 11:23 ` Sunil V L
2 siblings, 1 reply; 8+ messages in thread
From: maobibo @ 2022-11-07 9:57 UTC (permalink / raw)
To: Sunil V L, Palmer Dabbelt, Alistair Francis, Bin Meng,
Gerd Hoffmann
Cc: qemu-riscv, qemu-devel
在 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);
> }
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] hw/riscv: virt: Remove size restriction for pflash
2022-11-07 9:57 ` maobibo
@ 2022-11-07 11:23 ` Sunil V L
0 siblings, 0 replies; 8+ messages in thread
From: Sunil V L @ 2022-11-07 11:23 UTC (permalink / raw)
To: maobibo
Cc: Palmer Dabbelt, Alistair Francis, Bin Meng, Gerd Hoffmann,
qemu-riscv, qemu-devel
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
^ permalink raw reply [flat|nested] 8+ messages in thread