| Message ID | 3362132240927a23ecca7b9d8cfd6e4130509eea.camel@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm: xlnx-versal: fix virtio-mmio base address assignment | expand |
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); >
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); > > >
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); >>> >> >
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
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 >
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
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
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.
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
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
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.
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);
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(+)