| Message ID | 20210122200851.345259-1-eperezma@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | virtio: Add corresponding memory_listener_unregister to unrealize | expand |
On Fri, Jan 22, 2021 at 09:08:51PM +0100, Eugenio Pérez wrote: > Cannot destroy address spaces of IOMMU-aware virtio devices without it, > since they can contain memory listeners. > > Fixes: c611c76417f ("virtio: add MemoryListener to cache ring translations") > Buglink: https://bugs.launchpad.net/qemu/+bug/1912846 > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > --- > hw/virtio/virtio.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index b308026596..67efd2c301 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -3680,6 +3680,7 @@ static void virtio_device_unrealize(DeviceState *dev) > VirtIODevice *vdev = VIRTIO_DEVICE(dev); > VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(dev); > > + memory_listener_unregister(&vdev->listener); > virtio_bus_device_unplugged(vdev); > > if (vdc->unrealize != NULL) { > -- > 2.27.0 > Reviewed-by: Peter Xu <peterx@redhat.com> CC stable, assuming that's what we need too. Thanks,
On 2021/1/23 上午4:08, Eugenio Pérez wrote: > Cannot destroy address spaces of IOMMU-aware virtio devices without it, > since they can contain memory listeners. It's better to explain why the one in finalize doesn't work here. Thanks > > Fixes: c611c76417f ("virtio: add MemoryListener to cache ring translations") > Buglink: https://bugs.launchpad.net/qemu/+bug/1912846 > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > --- > hw/virtio/virtio.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index b308026596..67efd2c301 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -3680,6 +3680,7 @@ static void virtio_device_unrealize(DeviceState *dev) > VirtIODevice *vdev = VIRTIO_DEVICE(dev); > VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(dev); > > + memory_listener_unregister(&vdev->listener); > virtio_bus_device_unplugged(vdev); > > if (vdc->unrealize != NULL) {
On Mon, Jan 25, 2021 at 4:15 AM Jason Wang <jasowang@redhat.com> wrote: > > > On 2021/1/23 上午4:08, Eugenio Pérez wrote: > > Cannot destroy address spaces of IOMMU-aware virtio devices without it, > > since they can contain memory listeners. > > > It's better to explain why the one in finalize doesn't work here. > Hi Jason! Good point. The other call is at virtio_device_instance_finalize. Function virtio_device_instance_finalize is called after address_space_destroy if we follow steps of buglink. Address_space_destroy is called by pci_qdev_unrealize/do_pci_unregister_device, after call to virtio_device_unrealize. After that call, virtio_device_instance_finalize is called through object_deinit, freeing the bus. Also, memory_listener_unregister can be called again because it checks for listener->address_space != NULL at start, and sets it to NULL at end. In regular shutdown, nothing of this is called, so maybe we could safely delete the call to memory_listener_unregister at virtio_device_instance_finalize? If not, should I send again the patch with a new commit message? Thanks! > Thanks > > > > > > Fixes: c611c76417f ("virtio: add MemoryListener to cache ring translations") > > Buglink: https://bugs.launchpad.net/qemu/+bug/1912846 > > Signed-off-by: Eugenio Pérez <eperezma@redhat.com> > > --- > > hw/virtio/virtio.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > > index b308026596..67efd2c301 100644 > > --- a/hw/virtio/virtio.c > > +++ b/hw/virtio/virtio.c > > @@ -3680,6 +3680,7 @@ static void virtio_device_unrealize(DeviceState *dev) > > VirtIODevice *vdev = VIRTIO_DEVICE(dev); > > VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(dev); > > > > + memory_listener_unregister(&vdev->listener); > > virtio_bus_device_unplugged(vdev); > > > > if (vdc->unrealize != NULL) { >
On Mon, Jan 25, 2021 at 05:55:35PM +0100, Eugenio Perez Martin wrote: > On Mon, Jan 25, 2021 at 4:15 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > On 2021/1/23 上午4:08, Eugenio Pérez wrote: > > > Cannot destroy address spaces of IOMMU-aware virtio devices without it, > > > since they can contain memory listeners. > > > > > > It's better to explain why the one in finalize doesn't work here. > > > > Hi Jason! Good point. The other call is at virtio_device_instance_finalize. > > Function virtio_device_instance_finalize is called after > address_space_destroy if we follow steps of buglink. > > Address_space_destroy is called by > pci_qdev_unrealize/do_pci_unregister_device, after call to > virtio_device_unrealize. After that call, > virtio_device_instance_finalize is called through object_deinit, > freeing the bus. > > Also, memory_listener_unregister can be called again because it checks > for listener->address_space != NULL at start, and sets it to NULL at > end. > > In regular shutdown, nothing of this is called, so maybe we could > safely delete the call to memory_listener_unregister at > virtio_device_instance_finalize? I didn't notice this; if so we'd better remove that call if it's destined to be a noop after all. > > If not, should I send again the patch with a new commit message? Maybe attach the full backtrace too along with above? The assertion itself could be a very good explanation of what's happened. Thanks,
On Mon, Jan 25, 2021 at 6:18 PM Peter Xu <peterx@redhat.com> wrote: > > On Mon, Jan 25, 2021 at 05:55:35PM +0100, Eugenio Perez Martin wrote: > > On Mon, Jan 25, 2021 at 4:15 AM Jason Wang <jasowang@redhat.com> wrote: > > > > > > > > > On 2021/1/23 上午4:08, Eugenio Pérez wrote: > > > > Cannot destroy address spaces of IOMMU-aware virtio devices without it, > > > > since they can contain memory listeners. > > > > > > > > > It's better to explain why the one in finalize doesn't work here. > > > > > > > Hi Jason! Good point. The other call is at virtio_device_instance_finalize. > > > > Function virtio_device_instance_finalize is called after > > address_space_destroy if we follow steps of buglink. > > > > Address_space_destroy is called by > > pci_qdev_unrealize/do_pci_unregister_device, after call to > > virtio_device_unrealize. After that call, > > virtio_device_instance_finalize is called through object_deinit, > > freeing the bus. > > > > Also, memory_listener_unregister can be called again because it checks > > for listener->address_space != NULL at start, and sets it to NULL at > > end. > > > > In regular shutdown, nothing of this is called, so maybe we could > > safely delete the call to memory_listener_unregister at > > virtio_device_instance_finalize? > > I didn't notice this; if so we'd better remove that call if it's destined to be > a noop after all. > > > > > If not, should I send again the patch with a new commit message? > > Maybe attach the full backtrace too along with above? The assertion itself > could be a very good explanation of what's happened. > I agree, sending v2. Thanks! > Thanks, > > -- > Peter Xu >
diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index b308026596..67efd2c301 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -3680,6 +3680,7 @@ static void virtio_device_unrealize(DeviceState *dev) VirtIODevice *vdev = VIRTIO_DEVICE(dev); VirtioDeviceClass *vdc = VIRTIO_DEVICE_GET_CLASS(dev); + memory_listener_unregister(&vdev->listener); virtio_bus_device_unplugged(vdev); if (vdc->unrealize != NULL) {
Cannot destroy address spaces of IOMMU-aware virtio devices without it, since they can contain memory listeners. Fixes: c611c76417f ("virtio: add MemoryListener to cache ring translations") Buglink: https://bugs.launchpad.net/qemu/+bug/1912846 Signed-off-by: Eugenio Pérez <eperezma@redhat.com> --- hw/virtio/virtio.c | 1 + 1 file changed, 1 insertion(+)