| Message ID | 20221106143900.2229449-1-sunilvl@ventanamicro.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | hw/riscv: virt: Remove size restriction for pflash | expand |
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>
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 > >
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
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
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
在 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); > }
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
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); }
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(-)