Re: About the instance_finalize callback in VFIO PCI

2023-03-22 Thread 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  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
> 



Re: About the instance_finalize callback in VFIO PCI

2023-03-22 Thread Alex Williamson
On Wed, 22 Mar 2023 09:10:20 -0400
Yang Zhong  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

2023-03-22 Thread 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

2023-03-22 Thread 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

2023-03-22 Thread 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  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

2023-03-22 Thread 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

2023-03-21 Thread Paolo Bonzini
Il mar 21 mar 2023, 18:30 Cédric Le Goater  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
> >
> >
>
>


Re: About the instance_finalize callback in VFIO PCI

2023-03-21 Thread 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







About the instance_finalize callback in VFIO PCI

2023-03-20 Thread 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