Re: [PATCH] arm: xlnx-versal: fix virtio-mmio base address assignment

[schspa]

On Thu, 2021-02-04 at 09:19 +0100, Philippe Mathieu-Daudé wrote:
> Hi,
> 
> Please Cc the maintainers when posting your patch:
> 
> ./scripts/get_maintainer.pl -f hw/arm/xlnx-versal-virt.c
> Alistair Francis <alistair@alistair23.me> (maintainer:Xilinx ZynqMP
> and...)
> "Edgar E. Iglesias" <edgar.iglesias@gmail.com> (maintainer:Xilinx
> ZynqMP
> and...)
> Peter Maydell <peter.maydell@linaro.org> (maintainer:Xilinx ZynqMP
> and...)
> qemu-arm@nongnu.org (open list:Xilinx ZynqMP and...)
> 

Thanks for reminding, I will pay attention next time

> On 2/4/21 7:58 AM, schspa wrote:
> > 
> > At the moment the following QEMU command line triggers an assertion
> > failure On xlnx-versal SOC:
> >   qemu-system-aarch64 \
> >       -machine xlnx-versal-virt -nographic -smp 2 -m 128 \
> >       -fsdev local,id=shareid,path=${HOME}/work,security_model=none
> > \
> >       -device virtio-9p-device,fsdev=shareid,mount_tag=share \
> >       -fsdev
> > local,id=shareid1,path=${HOME}/Music,security_model=none \
> >       -device virtio-9p-device,fsdev=shareid1,mount_tag=share1
> > 
> >   qemu-system-aarch64: ../migration/savevm.c:860:
> >   vmstate_register_with_alias_id:
> >   Assertion `!se->compat || se->instance_id == 0' failed.
> > 
> > This problem was fixed on arm virt platform in patch
> >  
> >  
> > https://lists.nongnu.org/archive/html/qemu-devel/2016-07/msg01119.html
> 
> Please use instead "in commit f58b39d2d5b ("virtio-mmio: format
> transport base address in BusClass.get_dev_path")".
> 

Thanks, I will upload a new patch to fix it if there is no need to do
further change for the next question.

> > It works perfectly on arm virt platform. but there is still there
> > on
> > xlnx-versal SOC.
> > 
> > The main difference between arm virt and xlnx-versal is they use
> > different way to create virtio-mmio qdev. on arm virt, it calls
> > sysbus_create_simple("virtio-mmio", base, pic[irq]); which will
> > call
> > sysbus_mmio_map internally and assign base address to subsys device
> > mmio correctly. but xlnx-versal's implements won't do this.
> > 
> > However, xlnx-versal can't switch to sysbus_create_simple() to
> > create
> > virtio-mmio device. It's because xlnx-versal's cpu use
> > VersalVirt.soc.fpd.apu.mr as it's memory. which is subregion of
> > system_memory. sysbus_create_simple will add virtio to
> > system_memory,
> > which can't be accessed by cpu.
> > 
> > We can solve this by simply assign mmio[0].addr directly. makes
> > virtio_mmio_bus_get_dev_path to produce correct unique device path.
> > 
> > Signed-off-by: schspa <schspa@gmail.com>
> > ---
> >  hw/arm/xlnx-versal-virt.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/hw/arm/xlnx-versal-virt.c b/hw/arm/xlnx-versal-virt.c
> > index 8482cd6196..87b92ec6c3 100644
> > --- a/hw/arm/xlnx-versal-virt.c
> > +++ b/hw/arm/xlnx-versal-virt.c
> > @@ -490,6 +490,7 @@ static void create_virtio_regions(VersalVirt
> > *s)
> >          object_property_add_child(OBJECT(&s->soc), name,
> > OBJECT(dev));
> >          sysbus_realize_and_unref(SYS_BUS_DEVICE(dev),
> > &error_fatal);
> >          sysbus_connect_irq(SYS_BUS_DEVICE(dev), 0, pic_irq);
> > +        SYS_BUS_DEVICE(dev)->mmio[0].addr = base;
> 
> The proper API call is:
> 
>            sysbus_mmio_map(SYS_BUS_DEVICE(dev), 0, base);
> 

Can't to call this API, because this api will map virtio device memory
region to system_map. and it can't be add to &s->soc.mr_ps again. I'm
willing to change it to proper api but can't find a proper one.

> >          mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0);
> >          memory_region_add_subregion(&s->soc.mr_ps, base, mr);
> >          g_free(name);
> > 
> 

-- 
schspa <schspa@gmail.com>