* [PATCH v2 0/2] virtio-iommu-pci: Advertise the device as modern-only
@ 2020-09-08 19:33 Eric Auger
2020-09-08 19:33 ` [PATCH v2 1/2] virtio-iommu: Check gtrees are non null before destroying them Eric Auger
2020-09-08 19:33 ` [PATCH v2 2/2] virtio-iommu-pci: force virtio version 1 Eric Auger
0 siblings, 2 replies; 6+ messages in thread
From: Eric Auger @ 2020-09-08 19:33 UTC (permalink / raw)
To: eric.auger.pro, eric.auger, qemu-devel, cohuck, mst, qemu-stable
Cc: jean-philippe, thuth
Since commit 9b3a35ec82 ("virtio: verify that legacy support is not
accidentally on"), we are forced to use disable-legacy=on when
instantiating the virtio-iommu-pci device. This also revealed that
the unrealize() function is likely to call g_tree_destroy() on
NULL gtrees, which triggers assertions.
Best Regards
Eric
History:
v1 -> v2:
- Fix patch #1 commit message typos
- Add Connie's R-b on both patches
Eric Auger (2):
virtio-iommu: Check gtrees are non null before destroying them
virtio-iommu-pci: force virtio version 1
hw/virtio/virtio-iommu-pci.c | 2 +-
hw/virtio/virtio-iommu.c | 8 ++++++--
2 files changed, 7 insertions(+), 3 deletions(-)
--
2.21.3
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 1/2] virtio-iommu: Check gtrees are non null before destroying them
2020-09-08 19:33 [PATCH v2 0/2] virtio-iommu-pci: Advertise the device as modern-only Eric Auger
@ 2020-09-08 19:33 ` Eric Auger
2020-09-08 19:33 ` [PATCH v2 2/2] virtio-iommu-pci: force virtio version 1 Eric Auger
1 sibling, 0 replies; 6+ messages in thread
From: Eric Auger @ 2020-09-08 19:33 UTC (permalink / raw)
To: eric.auger.pro, eric.auger, qemu-devel, cohuck, mst, qemu-stable
Cc: jean-philippe, thuth
If realize fails, domains and endpoints trees may be NULL. On
unrealize(), this produces assertions:
"GLib: g_tree_destroy: assertion 'tree != NULL' failed"
Check that the trees are non NULL before destroying them.
Cc: qemu-stable@nongnu.org
Signed-off-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
---
v1 -> v2:
- fix the commit message
- Added Connie's R-b
---
hw/virtio/virtio-iommu.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/hw/virtio/virtio-iommu.c b/hw/virtio/virtio-iommu.c
index 5d56865e56..21ec63b108 100644
--- a/hw/virtio/virtio-iommu.c
+++ b/hw/virtio/virtio-iommu.c
@@ -801,8 +801,12 @@ static void virtio_iommu_device_unrealize(DeviceState *dev)
VirtIOIOMMU *s = VIRTIO_IOMMU(dev);
g_hash_table_destroy(s->as_by_busptr);
- g_tree_destroy(s->domains);
- g_tree_destroy(s->endpoints);
+ if (s->domains) {
+ g_tree_destroy(s->domains);
+ }
+ if (s->endpoints) {
+ g_tree_destroy(s->endpoints);
+ }
virtio_delete_queue(s->req_vq);
virtio_delete_queue(s->event_vq);
--
2.21.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH v2 2/2] virtio-iommu-pci: force virtio version 1
2020-09-08 19:33 [PATCH v2 0/2] virtio-iommu-pci: Advertise the device as modern-only Eric Auger
2020-09-08 19:33 ` [PATCH v2 1/2] virtio-iommu: Check gtrees are non null before destroying them Eric Auger
@ 2020-09-08 19:33 ` Eric Auger
2020-09-18 9:29 ` Cornelia Huck
1 sibling, 1 reply; 6+ messages in thread
From: Eric Auger @ 2020-09-08 19:33 UTC (permalink / raw)
To: eric.auger.pro, eric.auger, qemu-devel, cohuck, mst, qemu-stable
Cc: jean-philippe, thuth
Commit 9b3a35ec82 ("virtio: verify that legacy support is not
accidentally on") added a safety check that requires to set
'disable-legacy=on' on virtio-iommu-pci:
qemu-system-aarch64: -device virtio-iommu-pci: device is modern-only,
use disable-legacy=on
virtio-iommu was introduced after the release of VIRTIO 1.0
specifications, so it should be 'modern-only'.
This patch forces virtio version 1 and removes the 'transitional_name'
property removing the need to specify 'disable-legacy=on' on
virtio-iommu-pci device.
Cc: qemu-stable@nongnu.org
Signed-off-by: Eric Auger <eric.auger@redhat.com>
Reviewed-by: Cornelia Huck <cohuck@redhat.com>
---
v1 -> v2:
- Added Connie's R-b
---
hw/virtio/virtio-iommu-pci.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/hw/virtio/virtio-iommu-pci.c b/hw/virtio/virtio-iommu-pci.c
index ba62d60a0a..3b6f7a11c6 100644
--- a/hw/virtio/virtio-iommu-pci.c
+++ b/hw/virtio/virtio-iommu-pci.c
@@ -68,6 +68,7 @@ static void virtio_iommu_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
object_property_set_link(OBJECT(dev), "primary-bus",
OBJECT(pci_get_bus(&vpci_dev->pci_dev)),
&error_abort);
+ virtio_pci_force_virtio_1(vpci_dev);
qdev_realize(vdev, BUS(&vpci_dev->bus), errp);
}
@@ -97,7 +98,6 @@ static void virtio_iommu_pci_instance_init(Object *obj)
static const VirtioPCIDeviceTypeInfo virtio_iommu_pci_info = {
.base_name = TYPE_VIRTIO_IOMMU_PCI,
.generic_name = "virtio-iommu-pci",
- .transitional_name = "virtio-iommu-pci-transitional",
.non_transitional_name = "virtio-iommu-pci-non-transitional",
.instance_size = sizeof(VirtIOIOMMUPCI),
.instance_init = virtio_iommu_pci_instance_init,
--
2.21.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] virtio-iommu-pci: force virtio version 1
2020-09-08 19:33 ` [PATCH v2 2/2] virtio-iommu-pci: force virtio version 1 Eric Auger
@ 2020-09-18 9:29 ` Cornelia Huck
2020-09-18 10:24 ` Auger Eric
0 siblings, 1 reply; 6+ messages in thread
From: Cornelia Huck @ 2020-09-18 9:29 UTC (permalink / raw)
To: Eric Auger
Cc: jean-philippe, thuth, mst, qemu-stable, qemu-devel, eric.auger.pro
On Tue, 8 Sep 2020 21:33:09 +0200
Eric Auger <eric.auger@redhat.com> wrote:
> Commit 9b3a35ec82 ("virtio: verify that legacy support is not
> accidentally on") added a safety check that requires to set
> 'disable-legacy=on' on virtio-iommu-pci:
>
> qemu-system-aarch64: -device virtio-iommu-pci: device is modern-only,
> use disable-legacy=on
>
> virtio-iommu was introduced after the release of VIRTIO 1.0
> specifications, so it should be 'modern-only'.
>
> This patch forces virtio version 1 and removes the 'transitional_name'
> property removing the need to specify 'disable-legacy=on' on
> virtio-iommu-pci device.
Not sure whether this patch has been queued already, and how much we
care about migration compatibility for virtio-iommu, but would it make
sense to force modern on 5.1+ compat machines only? (see
https://lore.kernel.org/qemu-devel/20200918074710.27810-1-sgarzare@redhat.com/)
>
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Eric Auger <eric.auger@redhat.com>
> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>
> ---
> v1 -> v2:
> - Added Connie's R-b
> ---
> hw/virtio/virtio-iommu-pci.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/virtio/virtio-iommu-pci.c b/hw/virtio/virtio-iommu-pci.c
> index ba62d60a0a..3b6f7a11c6 100644
> --- a/hw/virtio/virtio-iommu-pci.c
> +++ b/hw/virtio/virtio-iommu-pci.c
> @@ -68,6 +68,7 @@ static void virtio_iommu_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
> object_property_set_link(OBJECT(dev), "primary-bus",
> OBJECT(pci_get_bus(&vpci_dev->pci_dev)),
> &error_abort);
> + virtio_pci_force_virtio_1(vpci_dev);
> qdev_realize(vdev, BUS(&vpci_dev->bus), errp);
> }
>
> @@ -97,7 +98,6 @@ static void virtio_iommu_pci_instance_init(Object *obj)
> static const VirtioPCIDeviceTypeInfo virtio_iommu_pci_info = {
> .base_name = TYPE_VIRTIO_IOMMU_PCI,
> .generic_name = "virtio-iommu-pci",
> - .transitional_name = "virtio-iommu-pci-transitional",
> .non_transitional_name = "virtio-iommu-pci-non-transitional",
> .instance_size = sizeof(VirtIOIOMMUPCI),
> .instance_init = virtio_iommu_pci_instance_init,
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] virtio-iommu-pci: force virtio version 1
2020-09-18 9:29 ` Cornelia Huck
@ 2020-09-18 10:24 ` Auger Eric
2020-09-18 12:58 ` Cornelia Huck
0 siblings, 1 reply; 6+ messages in thread
From: Auger Eric @ 2020-09-18 10:24 UTC (permalink / raw)
To: Cornelia Huck
Cc: jean-philippe, thuth, mst, qemu-devel, qemu-stable, eric.auger.pro
Hi Connie,
On 9/18/20 11:29 AM, Cornelia Huck wrote:
> On Tue, 8 Sep 2020 21:33:09 +0200
> Eric Auger <eric.auger@redhat.com> wrote:
>
>> Commit 9b3a35ec82 ("virtio: verify that legacy support is not
>> accidentally on") added a safety check that requires to set
>> 'disable-legacy=on' on virtio-iommu-pci:
>>
>> qemu-system-aarch64: -device virtio-iommu-pci: device is modern-only,
>> use disable-legacy=on
>>
>> virtio-iommu was introduced after the release of VIRTIO 1.0
>> specifications, so it should be 'modern-only'.
>>
>> This patch forces virtio version 1 and removes the 'transitional_name'
>> property removing the need to specify 'disable-legacy=on' on
>> virtio-iommu-pci device.
>
> Not sure whether this patch has been queued already, and how much we
> care about migration compatibility for virtio-iommu, but would it make
> sense to force modern on 5.1+ compat machines only? (see
> https://lore.kernel.org/qemu-devel/20200918074710.27810-1-sgarzare@redhat.com/)
I don't think it was pulled yet.
>
>>
>> Cc: qemu-stable@nongnu.org
The virtio-iommu-pci device only is usable on ARM in dt mode so I don't
think it has production users at the moment.
Thanks
Eric
>> Signed-off-by: Eric Auger <eric.auger@redhat.com>
>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>>
>> ---
>> v1 -> v2:
>> - Added Connie's R-b
>> ---
>> hw/virtio/virtio-iommu-pci.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/virtio/virtio-iommu-pci.c b/hw/virtio/virtio-iommu-pci.c
>> index ba62d60a0a..3b6f7a11c6 100644
>> --- a/hw/virtio/virtio-iommu-pci.c
>> +++ b/hw/virtio/virtio-iommu-pci.c
>> @@ -68,6 +68,7 @@ static void virtio_iommu_pci_realize(VirtIOPCIProxy *vpci_dev, Error **errp)
>> object_property_set_link(OBJECT(dev), "primary-bus",
>> OBJECT(pci_get_bus(&vpci_dev->pci_dev)),
>> &error_abort);
>> + virtio_pci_force_virtio_1(vpci_dev);
>> qdev_realize(vdev, BUS(&vpci_dev->bus), errp);
>> }
>>
>> @@ -97,7 +98,6 @@ static void virtio_iommu_pci_instance_init(Object *obj)
>> static const VirtioPCIDeviceTypeInfo virtio_iommu_pci_info = {
>> .base_name = TYPE_VIRTIO_IOMMU_PCI,
>> .generic_name = "virtio-iommu-pci",
>> - .transitional_name = "virtio-iommu-pci-transitional",
>> .non_transitional_name = "virtio-iommu-pci-non-transitional",
>> .instance_size = sizeof(VirtIOIOMMUPCI),
>> .instance_init = virtio_iommu_pci_instance_init,
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] virtio-iommu-pci: force virtio version 1
2020-09-18 10:24 ` Auger Eric
@ 2020-09-18 12:58 ` Cornelia Huck
0 siblings, 0 replies; 6+ messages in thread
From: Cornelia Huck @ 2020-09-18 12:58 UTC (permalink / raw)
To: Auger Eric
Cc: jean-philippe, thuth, mst, qemu-devel, qemu-stable, eric.auger.pro
On Fri, 18 Sep 2020 12:24:02 +0200
Auger Eric <eric.auger@redhat.com> wrote:
> Hi Connie,
>
> On 9/18/20 11:29 AM, Cornelia Huck wrote:
> > On Tue, 8 Sep 2020 21:33:09 +0200
> > Eric Auger <eric.auger@redhat.com> wrote:
> >
> >> Commit 9b3a35ec82 ("virtio: verify that legacy support is not
> >> accidentally on") added a safety check that requires to set
> >> 'disable-legacy=on' on virtio-iommu-pci:
> >>
> >> qemu-system-aarch64: -device virtio-iommu-pci: device is modern-only,
> >> use disable-legacy=on
> >>
> >> virtio-iommu was introduced after the release of VIRTIO 1.0
> >> specifications, so it should be 'modern-only'.
> >>
> >> This patch forces virtio version 1 and removes the 'transitional_name'
> >> property removing the need to specify 'disable-legacy=on' on
> >> virtio-iommu-pci device.
> >
> > Not sure whether this patch has been queued already, and how much we
> > care about migration compatibility for virtio-iommu, but would it make
> > sense to force modern on 5.1+ compat machines only? (see
> > https://lore.kernel.org/qemu-devel/20200918074710.27810-1-sgarzare@redhat.com/)
>
> I don't think it was pulled yet.
> >
> >>
> >> Cc: qemu-stable@nongnu.org
>
> The virtio-iommu-pci device only is usable on ARM in dt mode so I don't
> think it has production users at the moment.
OK, then we can keep this patch here, I guess.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-09-18 13:02 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-08 19:33 [PATCH v2 0/2] virtio-iommu-pci: Advertise the device as modern-only Eric Auger
2020-09-08 19:33 ` [PATCH v2 1/2] virtio-iommu: Check gtrees are non null before destroying them Eric Auger
2020-09-08 19:33 ` [PATCH v2 2/2] virtio-iommu-pci: force virtio version 1 Eric Auger
2020-09-18 9:29 ` Cornelia Huck
2020-09-18 10:24 ` Auger Eric
2020-09-18 12:58 ` Cornelia Huck
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).