[PATCH] virtio: Add corresponding memory_listener_unregister to unrealize

[PATCH] virtio: Add corresponding memory_listener_unregister to unrealize

[Eugenio Pérez]

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

Re: [PATCH] virtio: Add corresponding memory_listener_unregister to unrealize

[Peter Xu]

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,

-- 
Peter Xu

Re: [PATCH] virtio: Add corresponding memory_listener_unregister to unrealize

[Jason Wang]

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

Re: [PATCH] virtio: Add corresponding memory_listener_unregister to unrealize

[Eugenio Perez Martin]

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

Re: [PATCH] virtio: Add corresponding memory_listener_unregister to unrealize

[Peter Xu]

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,

-- 
Peter Xu

Re: [PATCH] virtio: Add corresponding memory_listener_unregister to unrealize

[Eugenio Perez Martin]

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
>