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