> From: Tian, Kevin <kevin.tian@xxxxxxxxx>
> Sent: Thursday, January 19, 2023 5:30 PM
>
> > From: Eric Auger <eric.auger@xxxxxxxxxx>
> > Sent: Thursday, January 19, 2023 5:13 PM
> >
> > Hi Yi,
> >
> > On 1/17/23 14:49, Yi Liu wrote:
> > > This is to avoid a circular refcount problem between the kvm struct and
> > > the device file. KVM modules holds device/group file reference when
> the
> > > device/group is added and releases it per removal or the last kvm
> reference
> > > is released. This reference model is ok for the group since there is no
> > > kvm reference in the group paths.
> > >
> > > But it is a problem for device file since the vfio devices may get kvm
> > > reference in the device open path and put it in the device file release.
> > > e.g. Intel kvmgt. This would result in a circular issue since the kvm
> > > side won't put the device file reference if kvm reference is not 0, while
> > > the vfio device side needs to put kvm reference in the release callback.
> > >
> > > To solve this problem for device file, let vfio provide release() which
> > > would be called once kvm file is closed, it won't depend on the last kvm
> > > reference. Hence avoid circular refcount problem.
> > >
> > > Suggested-by: Kevin Tian <kevin.tian@xxxxxxxxx>
> > > Signed-off-by: Yi Liu <yi.l.liu@xxxxxxxxx>
> > > ---
> > > virt/kvm/vfio.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
> > > index 0f54b9d308d7..525efe37ab6d 100644
> > > --- a/virt/kvm/vfio.c
> > > +++ b/virt/kvm/vfio.c
> > > @@ -364,7 +364,7 @@ static int kvm_vfio_create(struct kvm_device
> *dev,
> > u32 type);
> > > static struct kvm_device_ops kvm_vfio_ops = {
> > > .name = "kvm-vfio",
> > > .create = kvm_vfio_create,
> > > - .destroy = kvm_vfio_destroy,
> > Is it safe to simply remove the destroy cb as it is called from
> > kvm_destroy_vm/kvm_destroy_devices?
> >
Perhaps better to keep it. kvm_vfio_device is only one kind of kvm_device_type
For kvm_vfio_device, it is now considered to better provide a release cb.
While other kvm_device may better to have destroy cb.
>
> According to the definition .release is considered as an alternative
> method to free the device:
>
> /*
> * Destroy is responsible for freeing dev.
> *
> * Destroy may be called before or after destructors are called
> * on emulated I/O regions, depending on whether a reference is
> * held by a vcpu or other kvm component that gets destroyed
> * after the emulated I/O.
> */
> void (*destroy)(struct kvm_device *dev);
>
> /*
> * Release is an alternative method to free the device. It is
> * called when the device file descriptor is closed. Once
> * release is called, the destroy method will not be called
> * anymore as the device is removed from the device list of
> * the VM. kvm->lock is held.
> */
> void (*release)(struct kvm_device *dev);
>
> Did you see any specific problem of moving this stuff to release?
It should only affect kvm_vfio_device itself. 😊
Regards,
Yi Liu
