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

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

[schspa]

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

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;
         mr = sysbus_mmio_get_region(SYS_BUS_DEVICE(dev), 0);
         memory_region_add_subregion(&s->soc.mr_ps, base, mr);
         g_free(name);
-- 
2.30.0

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

[Philippe Mathieu-Daudé]

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...)

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")".

> 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);

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

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>

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

[Philippe Mathieu-Daudé]

On 2/4/21 10:04 AM, schspa wrote:
> 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.

Indeed, you found a design issue IMO:

Versal creates the "mr-ps-switch" to be explicitly different from
the main sysbus memory. TYPE_VIRTIO_MMIO is a SYSBUS device, thus
can not be created without being plugged on sysbus.
We want TYPE_VIRTIO_MMIO to be TYPE_USER_CREATABLE so we can create
it on the command line (like your usage). TYPE_SYSBUS allows such
automatic plug it on the main bus, but also maps to main memory.

Not sure where to go from here, Cc'ing Peter/Markus/Eduardo.

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

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

[Peter Maydell]

On Fri, 5 Feb 2021 at 07:53, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> Indeed, you found a design issue IMO:
>
> Versal creates the "mr-ps-switch" to be explicitly different from
> the main sysbus memory. TYPE_VIRTIO_MMIO is a SYSBUS device, thus
> can not be created without being plugged on sysbus.
> We want TYPE_VIRTIO_MMIO to be TYPE_USER_CREATABLE so we can create
> it on the command line (like your usage). TYPE_SYSBUS allows such
> automatic plug it on the main bus, but also maps to main memory.

That was never the design intent for the virtio mmio transport.
The idea was that the board creates a bunch of transports
(unconditionally). The user then uses command line options
to create virtio backends (blk, net, etc) which get plugged
into the virtio-bus buses that each transport has.

virtio-mmio is not user-creatable for the same reason that
all devices with MMIO memory regions and IRQ lines are not
user-creatable -- there's no good command line syntax for
the user to wire them up, and we don't want the user to have
to know "on this board address 0x50003000 is a good place to
put a device, and irq 43 is free".

thanks
-- PMM

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

[Philippe Mathieu-Daudé]

On 2/5/21 11:03 AM, Peter Maydell wrote:
> On Fri, 5 Feb 2021 at 07:53, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> Indeed, you found a design issue IMO:
>>
>> Versal creates the "mr-ps-switch" to be explicitly different from
>> the main sysbus memory. TYPE_VIRTIO_MMIO is a SYSBUS device, thus
>> can not be created without being plugged on sysbus.
>> We want TYPE_VIRTIO_MMIO to be TYPE_USER_CREATABLE so we can create
>> it on the command line (like your usage). TYPE_SYSBUS allows such
>> automatic plug it on the main bus, but also maps to main memory.
> 
> That was never the design intent for the virtio mmio transport.
> The idea was that the board creates a bunch of transports
> (unconditionally). The user then uses command line options
> to create virtio backends (blk, net, etc) which get plugged
> into the virtio-bus buses that each transport has.
> 
> virtio-mmio is not user-creatable for the same reason that
> all devices with MMIO memory regions and IRQ lines are not
> user-creatable -- there's no good command line syntax for
> the user to wire them up, and we don't want the user to have
> to know "on this board address 0x50003000 is a good place to
> put a device, and irq 43 is free".

IOW
1/ virtio-mmio must be sysbus-device,
2/ we can not sysbus-map out of main memory so private container
is incorrect, and Versal can not use "mr-ps-switch"?

> 
> thanks
> -- PMM
> 

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

[Peter Maydell]

On Fri, 5 Feb 2021 at 10:31, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> 1/ virtio-mmio must be sysbus-device,

Yes.

> 2/ we can not sysbus-map out of main memory so private container
> is incorrect, and Versal can not use "mr-ps-switch"?

No. If you have a sysbus device, and you want to map it somewhere
other than into system-memory-map, you can do that: you use
sysbus_mmio_get_region() to get the MemoryRegion*, and then map
it into whatever container you need with memory_region_add_subregion().

thanks
-- PMM

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

[Edgar E. Iglesias]

On Fri, Feb 05, 2021 at 11:18:28AM +0000, Peter Maydell wrote:
> On Fri, 5 Feb 2021 at 10:31, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> > 1/ virtio-mmio must be sysbus-device,
> 
> Yes.
> 
> > 2/ we can not sysbus-map out of main memory so private container
> > is incorrect, and Versal can not use "mr-ps-switch"?
> 
> No. If you have a sysbus device, and you want to map it somewhere
> other than into system-memory-map, you can do that: you use
> sysbus_mmio_get_region() to get the MemoryRegion*, and then map
> it into whatever container you need with memory_region_add_subregion().
>

Thanks, that matches how I thought things should work.

I wonder if virtio_mmio_bus_get_dev_path() really should be peeking into
Sysbus internals mmio[].addr?

Sysbus mmio[].addr looks like a candidate for removal if we ever get rid
of the default system_memory...

I don't have any good suggestions how to fix this. I guess we could wrap
memory_region_add_subregion() with a sysbus version of it that sets
mmio[].addr but that seems like a step backwards to me.
Perhaps there's a way fix this in virtio_mmio_bus_get_dev_path()?

Best regards,
Edgar

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

[schspa]

On Fri, 2021-02-05 at 15:08 +0100, Edgar E. Iglesias wrote:
> Thanks, that matches how I thought things should work.
> 
> I wonder if virtio_mmio_bus_get_dev_path() really should be peeking
> into
> Sysbus internals mmio[].addr?
> 
I think mmio[].addr needs to be given a meaningful value even if we
don't use it.

> Sysbus mmio[].addr looks like a candidate for removal if we ever get
> rid
> of the default system_memory...
> 
> I don't have any good suggestions how to fix this. I guess we could
> wrap
> memory_region_add_subregion() with a sysbus version of it that sets
> mmio[].addr but that seems like a step backwards to me.
> Perhaps there's a way fix this in virtio_mmio_bus_get_dev_path()?

I think we can change virtio_mmio_bus_get_dev_path() with the following
methods.

1. modify TYPE_VIRTIO_MMIO:
   add a prop to specify a unique device_path for virtio_mmio TypeInfo.
2. modify TYPE_VIRTIO_MMIO_BUS
   add a global static instance count to generate a unique device path.

-- 
schspa <schspa@gmail.com>

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

[Laszlo Ersek]

On 02/08/21 06:34, schspa wrote:
> On Fri, 2021-02-05 at 15:08 +0100, Edgar E. Iglesias wrote:
>> Thanks, that matches how I thought things should work.
>>
>> I wonder if virtio_mmio_bus_get_dev_path() really should be peeking
>> into
>> Sysbus internals mmio[].addr?
>>
> I think mmio[].addr needs to be given a meaningful value even if we
> don't use it.
> 
>> Sysbus mmio[].addr looks like a candidate for removal if we ever get
>> rid
>> of the default system_memory...
>>
>> I don't have any good suggestions how to fix this. I guess we could
>> wrap
>> memory_region_add_subregion() with a sysbus version of it that sets
>> mmio[].addr but that seems like a step backwards to me.
>> Perhaps there's a way fix this in virtio_mmio_bus_get_dev_path()?
> 
> I think we can change virtio_mmio_bus_get_dev_path() with the following
> methods.
> 
> 1. modify TYPE_VIRTIO_MMIO:
>    add a prop to specify a unique device_path for virtio_mmio TypeInfo.
> 2. modify TYPE_VIRTIO_MMIO_BUS
>    add a global static instance count to generate a unique device path.
> 

Please don't CC me out of the blue mid-thread, without at least a hint
as to why the thread could be relevant to me.

If you are CC'ing me because I authored f58b39d2d5b6 ("virtio-mmio:
format transport base address in BusClass.get_dev_path", 2016-07-14),
for <https://bugs.launchpad.net/qemu/+bug/1594239>, *and* now this patch
is deemed incorrect or obsolete, then:

Feel free to do whatever you want with those functions, as long as

(a) the entries in the "bootorder" fw_cfg file remain intact,
(b) LP#1594239 remains fixed.

Thanks
Laszlo

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

[Peter Maydell]

On Fri, 5 Feb 2021 at 14:08, Edgar E. Iglesias <edgar.iglesias@gmail.com> wrote:
> I wonder if virtio_mmio_bus_get_dev_path() really should be peeking into
> Sysbus internals mmio[].addr?

Nope, it should not.

> Sysbus mmio[].addr looks like a candidate for removal if we ever get rid
> of the default system_memory...
>
> I don't have any good suggestions how to fix this. I guess we could wrap
> memory_region_add_subregion() with a sysbus version of it that sets
> mmio[].addr but that seems like a step backwards to me.
> Perhaps there's a way fix this in virtio_mmio_bus_get_dev_path()?

I just suggested something on another thread: call memory_region_find()
and then look at the offset_within_address_space field of the returned
MemoryRegionSection. I think that should get you the offset of the
transport within the system address space regardless of how much
use of containers and other oddball mappings are involved. (If the
transport is not mapped into the system address space at all then
you'll get its offset within whatever that other address space is,
but I think we can reasonably ignore that unlikely corner case.)

thanks
-- PMM

[PATCH v4] virtio-mmio: improve virtio-mmio get_dev_path alog

[schspa]

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 commit f58b39d2d5b
("virtio-mmio: format transport base address in BusClass.get_dev_path")

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.

Besides, xlnx-versal can't add sysbus_mmio_map api call too, because
this will add memory region to system_memory, and it can't be added
to VersalVirt.soc.fpd.apu.mr again.

We can solve this by assign correct base address offset on dev_path.

This path was test on aarch64 virt & xlnx-versal platform.

Signed-off-by: schspa <schspa@gmail.com>
---
 hw/virtio/virtio-mmio.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/hw/virtio/virtio-mmio.c b/hw/virtio/virtio-mmio.c
index 610661d6a5..6990b9879c 100644
--- a/hw/virtio/virtio-mmio.c
+++ b/hw/virtio/virtio-mmio.c
@@ -737,8 +737,8 @@ static char *virtio_mmio_bus_get_dev_path(DeviceState *dev)
     BusState *virtio_mmio_bus;
     VirtIOMMIOProxy *virtio_mmio_proxy;
     char *proxy_path;
-    SysBusDevice *proxy_sbd;
     char *path;
+    MemoryRegionSection section;
 
     virtio_mmio_bus = qdev_get_parent_bus(dev);
     virtio_mmio_proxy = VIRTIO_MMIO(virtio_mmio_bus->parent);
@@ -757,17 +757,18 @@ static char *virtio_mmio_bus_get_dev_path(DeviceState *dev)
     }
 
     /* Otherwise, we append the base address of the transport. */
-    proxy_sbd = SYS_BUS_DEVICE(virtio_mmio_proxy);
-    assert(proxy_sbd->num_mmio == 1);
-    assert(proxy_sbd->mmio[0].memory == &virtio_mmio_proxy->iomem);
+    section = memory_region_find(&virtio_mmio_proxy->iomem, 0, 0x200);
+    assert(section.mr);
 
     if (proxy_path) {
         path = g_strdup_printf("%s/virtio-mmio@" TARGET_FMT_plx, proxy_path,
-                               proxy_sbd->mmio[0].addr);
+                               section.offset_within_address_space);
     } else {
         path = g_strdup_printf("virtio-mmio@" TARGET_FMT_plx,
-                               proxy_sbd->mmio[0].addr);
+                               section.offset_within_address_space);
     }
+    memory_region_unref(section.mr);
+
     g_free(proxy_path);
     return path;
 }
-- 
2.30.1

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

[schspa]

On Mon, 2021-02-08 at 12:59 +0000, Peter Maydell wrote:
> I just suggested something on another thread: call
> memory_region_find()
> and then look at the offset_within_address_space field of the
> returned
> MemoryRegionSection. I think that should get you the offset of the
> transport within the system address space regardless of how much
> use of containers and other oddball mappings are involved. (If the
> transport is not mapped into the system address space at all then
> you'll get its offset within whatever that other address space is,
> but I think we can reasonably ignore that unlikely corner case.)


Thanks for your suggestions, I have tried it on both arm virt & the
Xilinx platform works perfectly.

I have upload a new patch v4 for it.
-- 
schspa <schspa@gmail.com>

Re: [PATCH v4] virtio-mmio: improve virtio-mmio get_dev_path alog

[Peter Maydell]

On Thu, 25 Feb 2021 at 05:36, schspa <schspa@gmail.com> 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 commit f58b39d2d5b
> ("virtio-mmio: format transport base address in BusClass.get_dev_path")
>
> 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.
>
> Besides, xlnx-versal can't add sysbus_mmio_map api call too, because
> this will add memory region to system_memory, and it can't be added
> to VersalVirt.soc.fpd.apu.mr again.
>
> We can solve this by assign correct base address offset on dev_path.
>
> This path was test on aarch64 virt & xlnx-versal platform.
>
> Signed-off-by: schspa <schspa@gmail.com>
> ---

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

Applied to target-arm.next, thanks (unless MST would rather it
go in via another route).

-- PMM

Re: [PATCH v4] virtio-mmio: improve virtio-mmio get_dev_path alog

[Shi Schspa]

[-- Attachment #1: Type: text/plain, Size: 2358 bytes --]

Thank you very much.

Best regards.


Peter Maydell <peter.maydell@linaro.org> 于2021年3月5日周五 下午7:57写道：

> On Thu, 25 Feb 2021 at 05:36, schspa <schspa@gmail.com> 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 commit f58b39d2d5b
> > ("virtio-mmio: format transport base address in BusClass.get_dev_path")
> >
> > 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.
> >
> > Besides, xlnx-versal can't add sysbus_mmio_map api call too, because
> > this will add memory region to system_memory, and it can't be added
> > to VersalVirt.soc.fpd.apu.mr again.
> >
> > We can solve this by assign correct base address offset on dev_path.
> >
> > This path was test on aarch64 virt & xlnx-versal platform.
> >
> > Signed-off-by: schspa <schspa@gmail.com>
> > ---
>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
>
> Applied to target-arm.next, thanks (unless MST would rather it
> go in via another route).
>
> -- PMM
>

[-- Attachment #2: Type: text/html, Size: 3346 bytes --]