About the instance_finalize callback in VFIO PCI

About the instance_finalize callback in VFIO PCI

[Yang Zhong]

Hello Alex and Paolo,

There is one instance_finalize callback definition in hw/vfio/pci.c, but
i find this callback(vfio_instance_finalize()) never be called during the
VM shutdown with close VM or "init 0" command in guest.

The Qemu related command:
   ......
   -device vfio-pci,host=d9:00.0
   ......

static const TypeInfo vfio_pci_dev_info = {
    .name = TYPE_VFIO_PCI,
    .parent = TYPE_PCI_DEVICE,
    .instance_size = sizeof(VFIOPCIDevice),
    .class_init = vfio_pci_dev_class_init,
    .instance_init = vfio_instance_init,
    .instance_finalize = vfio_instance_finalize,
    .interfaces = (InterfaceInfo[]) {
        { INTERFACE_PCIE_DEVICE },
        { INTERFACE_CONVENTIONAL_PCI_DEVICE },
        { }
    },
};

If my test method is wrong, would you please tell me how to trigger to
this callback when VM shutdown? thanks.

By the way, i also debugged other instance_finalize callback functions,
if my understanding is right, all instance_finalize callback should be
called from object_unref(object) from qemu_cleanup(void) in
./softmmu/runstate.c. But there is no VFIO related object_unref() call in
this cleanup function, So the instance_finalize callback in vfio pci
should be useless? thanks!

Regards,
Yang

Re: About the instance_finalize callback in VFIO PCI

[Cédric Le Goater]

On 3/20/23 10:31, Yang Zhong wrote:
> Hello Alex and Paolo,
> 
> There is one instance_finalize callback definition in hw/vfio/pci.c, but
> i find this callback(vfio_instance_finalize()) never be called during the
> VM shutdown with close VM or "init 0" command in guest.
> 
> The Qemu related command:
>     ......
>     -device vfio-pci,host=d9:00.0
>     ......

well, the finalize op is correctly called for hot unplugged devices, using
device_add.

> static const TypeInfo vfio_pci_dev_info = {
>      .name = TYPE_VFIO_PCI,
>      .parent = TYPE_PCI_DEVICE,
>      .instance_size = sizeof(VFIOPCIDevice),
>      .class_init = vfio_pci_dev_class_init,
>      .instance_init = vfio_instance_init,
>      .instance_finalize = vfio_instance_finalize,
>      .interfaces = (InterfaceInfo[]) {
>          { INTERFACE_PCIE_DEVICE },
>          { INTERFACE_CONVENTIONAL_PCI_DEVICE },
>          { }
>      },
> };
> 
> If my test method is wrong, would you please tell me how to trigger to
> this callback when VM shutdown? thanks

I would have thought that user_creatable_cleanup would have taken care
of it. But it's not. This needs some digging.

C.

  
> By the way, i also debugged other instance_finalize callback functions,
> if my understanding is right, all instance_finalize callback should be
> called from object_unref(object) from qemu_cleanup(void) in
> ./softmmu/runstate.c. But there is no VFIO related object_unref() call in
> this cleanup function, So the instance_finalize callback in vfio pci
> should be useless? thanks!
> 
> Regards,
> Yang
> 
> 

Re: About the instance_finalize callback in VFIO PCI

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 737 bytes --]

Il mar 21 mar 2023, 18:30 Cédric Le Goater <clg@kaod.org> ha scritto:

> I would have thought that user_creatable_cleanup would have taken care
> of it. But it's not. This needs some digging.
>

user_creatable_cleanup is only for -object, not for -device.

Paolo


> C.
>
>
> > By the way, i also debugged other instance_finalize callback functions,
> > if my understanding is right, all instance_finalize callback should be
> > called from object_unref(object) from qemu_cleanup(void) in
> > ./softmmu/runstate.c. But there is no VFIO related object_unref() call in
> > this cleanup function, So the instance_finalize callback in vfio pci
> > should be useless? thanks!
> >
> > Regards,
> > Yang
> >
> >
>
>

[-- Attachment #2: Type: text/html, Size: 1363 bytes --]

Re: About the instance_finalize callback in VFIO PCI

[Yang Zhong]

On Tue, Mar 21, 2023 at 09:44:18PM +0100, Paolo Bonzini wrote:
> Il mar 21 mar 2023, 18:30 Cédric Le Goater <clg@kaod.org> ha scritto:
> 
> > I would have thought that user_creatable_cleanup would have taken care
> > of it. But it's not. This needs some digging.
> >
> 
> user_creatable_cleanup is only for -object, not for -device.
>

  Paolo, thanks for helping to clarify this issue.

  Maybe i am clear now, the vfio_instance_finalize() in the
  hw/vfio/pci.c is only for unhotplug vfio pci device from monitor to
  cleanup resource. For static "-device vfio-pci ....." command, the
  cleanup resource is responsibility of kernel exit system, not the qemu
  vfio. Once we close Qemu process, the kernel will call do_exit() to
  release these resource, so the vfio module in kernel will handle
  these cleanup work. thanks!

  Yang


> Paolo
> 
> 
> > C.
> >
> >
> > > By the way, i also debugged other instance_finalize callback functions,
> > > if my understanding is right, all instance_finalize callback should be
> > > called from object_unref(object) from qemu_cleanup(void) in
> > > ./softmmu/runstate.c. But there is no VFIO related object_unref() call in
> > > this cleanup function, So the instance_finalize callback in vfio pci
> > > should be useless? thanks!
> > >
> > > Regards,
> > > Yang
> > >
> > >
> >
> >

Re: About the instance_finalize callback in VFIO PCI

[Yang Zhong]

On Tue, Mar 21, 2023 at 06:30:14PM +0100, Cédric Le Goater wrote:
> On 3/20/23 10:31, Yang Zhong wrote:
> > Hello Alex and Paolo,
> > 
> > There is one instance_finalize callback definition in hw/vfio/pci.c, but
> > i find this callback(vfio_instance_finalize()) never be called during the
> > VM shutdown with close VM or "init 0" command in guest.
> > 
> > The Qemu related command:
> >     ......
> >     -device vfio-pci,host=d9:00.0
> >     ......
> 
> well, the finalize op is correctly called for hot unplugged devices, using
> device_add.
> 
   Thanks Cédric, i can use device_del command in the monitor to
   trigger this instance_finalize callback function in the VFIO PCI.
   thanks!

   Yang
> 

Re: About the instance_finalize callback in VFIO PCI

[Cédric Le Goater]

On 3/22/23 13:28, Yang Zhong wrote:
> On Tue, Mar 21, 2023 at 06:30:14PM +0100, Cédric Le Goater wrote:
>> On 3/20/23 10:31, Yang Zhong wrote:
>>> Hello Alex and Paolo,
>>>
>>> There is one instance_finalize callback definition in hw/vfio/pci.c, but
>>> i find this callback(vfio_instance_finalize()) never be called during the
>>> VM shutdown with close VM or "init 0" command in guest.
>>>
>>> The Qemu related command:
>>>      ......
>>>      -device vfio-pci,host=d9:00.0
>>>      ......
>>
>> well, the finalize op is correctly called for hot unplugged devices, using
>> device_add.
>>
>     Thanks Cédric, i can use device_del command in the monitor to
>     trigger this instance_finalize callback function in the VFIO PCI.
>     thanks!

yes. I think that in the shutdown case, QEMU simply relies on exit() to
do the cleanup. On the kernel side, unmaps, fds being closed trigger any
allocated resources.

Out of curiosity, what were you trying to achieve in the finalize op ?

Thanks,

C.

Re: About the instance_finalize callback in VFIO PCI

[Yang Zhong]

On Wed, Mar 22, 2023 at 01:56:13PM +0100, Cédric Le Goater wrote:
> On 3/22/23 13:28, Yang Zhong wrote:
> > On Tue, Mar 21, 2023 at 06:30:14PM +0100, Cédric Le Goater wrote:
> > > On 3/20/23 10:31, Yang Zhong wrote:
> > > > Hello Alex and Paolo,
> > > > 
> > > > There is one instance_finalize callback definition in hw/vfio/pci.c, but
> > > > i find this callback(vfio_instance_finalize()) never be called during the
> > > > VM shutdown with close VM or "init 0" command in guest.
> > > > 
> > > > The Qemu related command:
> > > >      ......
> > > >      -device vfio-pci,host=d9:00.0
> > > >      ......
> > > 
> > > well, the finalize op is correctly called for hot unplugged devices, using
> > > device_add.
> > > 
> >     Thanks Cédric, i can use device_del command in the monitor to
> >     trigger this instance_finalize callback function in the VFIO PCI.
> >     thanks!
> 
> yes. I think that in the shutdown case, QEMU simply relies on exit() to
> do the cleanup. On the kernel side, unmaps, fds being closed trigger any
> allocated resources.
> 
> Out of curiosity, what were you trying to achieve in the finalize op ?
> 
 
 We are doing one new feature, which need this callback to do some
 cleanup work with VFIO/iommufd kernel module. thanks!

 Yang


> Thanks,
> 
> C.

Re: About the instance_finalize callback in VFIO PCI

[Alex Williamson]

On Wed, 22 Mar 2023 09:10:20 -0400
Yang Zhong <yang.zhong@linux.intel.com> wrote:

> On Wed, Mar 22, 2023 at 01:56:13PM +0100, Cédric Le Goater wrote:
> > On 3/22/23 13:28, Yang Zhong wrote:  
> > > On Tue, Mar 21, 2023 at 06:30:14PM +0100, Cédric Le Goater wrote:  
> > > > On 3/20/23 10:31, Yang Zhong wrote:  
> > > > > Hello Alex and Paolo,
> > > > > 
> > > > > There is one instance_finalize callback definition in hw/vfio/pci.c, but
> > > > > i find this callback(vfio_instance_finalize()) never be called during the
> > > > > VM shutdown with close VM or "init 0" command in guest.
> > > > > 
> > > > > The Qemu related command:
> > > > >      ......
> > > > >      -device vfio-pci,host=d9:00.0
> > > > >      ......  
> > > > 
> > > > well, the finalize op is correctly called for hot unplugged devices, using
> > > > device_add.
> > > >   
> > >     Thanks Cédric, i can use device_del command in the monitor to
> > >     trigger this instance_finalize callback function in the VFIO PCI.
> > >     thanks!  
> > 
> > yes. I think that in the shutdown case, QEMU simply relies on exit() to
> > do the cleanup. On the kernel side, unmaps, fds being closed trigger any
> > allocated resources.
> > 
> > Out of curiosity, what were you trying to achieve in the finalize op ?
> >   
>  
>  We are doing one new feature, which need this callback to do some
>  cleanup work with VFIO/iommufd kernel module. thanks!

This sounds dangerously like relying on userspace for cleanup.  Kernel
drivers need to be able to perform all cleanup themselves when file
descriptors are closed.  They must expect that userspace can be killed
at any point in time w/o an opportunity to do cleanup work.  Thanks,

Alex

Re: About the instance_finalize callback in VFIO PCI

[Yang Zhong]

On Wed, Mar 22, 2023 at 12:22:27PM -0600, Alex Williamson wrote:
> On Wed, 22 Mar 2023 09:10:20 -0400
> Yang Zhong <yang.zhong@linux.intel.com> wrote:
> 
> > On Wed, Mar 22, 2023 at 01:56:13PM +0100, Cédric Le Goater wrote:
> > > On 3/22/23 13:28, Yang Zhong wrote:  
> > > > On Tue, Mar 21, 2023 at 06:30:14PM +0100, Cédric Le Goater wrote:  
> > > > > On 3/20/23 10:31, Yang Zhong wrote:  
> > > > > > Hello Alex and Paolo,
> > > > > > 
> > > > > > There is one instance_finalize callback definition in hw/vfio/pci.c, but
> > > > > > i find this callback(vfio_instance_finalize()) never be called during the
> > > > > > VM shutdown with close VM or "init 0" command in guest.
> > > > > > 
> > > > > > The Qemu related command:
> > > > > >      ......
> > > > > >      -device vfio-pci,host=d9:00.0
> > > > > >      ......  
> > > > > 
> > > > > well, the finalize op is correctly called for hot unplugged devices, using
> > > > > device_add.
> > > > >   
> > > >     Thanks Cédric, i can use device_del command in the monitor to
> > > >     trigger this instance_finalize callback function in the VFIO PCI.
> > > >     thanks!  
> > > 
> > > yes. I think that in the shutdown case, QEMU simply relies on exit() to
> > > do the cleanup. On the kernel side, unmaps, fds being closed trigger any
> > > allocated resources.
> > > 
> > > Out of curiosity, what were you trying to achieve in the finalize op ?
> > >   
> >  
> >  We are doing one new feature, which need this callback to do some
> >  cleanup work with VFIO/iommufd kernel module. thanks!
> 
> This sounds dangerously like relying on userspace for cleanup.  Kernel
> drivers need to be able to perform all cleanup themselves when file
> descriptors are closed.  They must expect that userspace can be killed
> at any point in time w/o an opportunity to do cleanup work.  Thanks,
> 

  Thanks Alex, yes, we have moved these cleanup to kernel driver side.
  I was just curious about what scenario this instance_finalize callback 
  is used in VFIO PCI, now it is clear, thanks a lot!

  Regards,
  Yang


> Alex
>