Re: [v7 3/3] kvm: set/clear kvm to/from vfio_group when group add/delete

[Jike Song <jike.song@intel.com>]

On 12/01/2016 01:02 AM, Alex Williamson wrote:
> On Tue, 22 Nov 2016 14:09:33 +0800
> Jike Song <jike.song@intel.com> wrote:
> 
>> Sometimes users need to be aware when a vfio_group attaches to a
>> KVM or detaches from it. KVM already calls get/put method from vfio to
>> manipulate the vfio_group reference, it can notify vfio_group in
>> a similar way.
>>
>> Cc: Alex Williamson <alex.williamson@redhat.com>
>> Cc: Kirti Wankhede <kwankhede@nvidia.com>
>> Cc: Paolo Bonzini <pbonzini@redhat.com>
>> Cc: Xiao Guangrong <guangrong.xiao@linux.intel.com>
>> Signed-off-by: Jike Song <jike.song@intel.com>
>> ---
>>  virt/kvm/vfio.c | 31 +++++++++++++++++++++++++++++++
>>  1 file changed, 31 insertions(+)
>>
>> diff --git a/virt/kvm/vfio.c b/virt/kvm/vfio.c
>> index 1dd087d..ea5b4bb 100644
>> --- a/virt/kvm/vfio.c
>> +++ b/virt/kvm/vfio.c
>> @@ -60,6 +60,32 @@ static void kvm_vfio_group_put_external_user(struct vfio_group *vfio_group)
>>  	symbol_put(vfio_group_put_external_user);
>>  }
>>  
>> +static void kvm_vfio_group_set_kvm(struct vfio_group *group, struct kvm *kvm)
>> +{
>> +	void (*fn)(struct vfio_group *, struct kvm *);
>> +
>> +	fn = symbol_get(vfio_group_set_kvm);
>> +	if (!fn)
>> +		return;
>> +
>> +	fn(group, kvm);
>> +
>> +	symbol_put(vfio_group_set_kvm);
>> +}
>> +
>> +static void kvm_vfio_group_clear_kvm(struct vfio_group *group, struct kvm *kvm)
>> +{
>> +	void (*fn)(struct vfio_group *, struct kvm *);
>> +
>> +	fn = symbol_get(vfio_group_set_kvm);
>> +	if (!fn)
>> +		return;
>> +
>> +	fn(group, NULL);
>> +
>> +	symbol_put(vfio_group_set_kvm);
>> +}
> 
> Why do we need this function?  Can't we simply call
> kvm_vfio_group_set_kvm(vfio_group, NULL)?  Not to mention that the
> struct kvm arg is unused here otherwise.

This was the place to call kvm_put_kvm in previous series, but yes,
it's not necessary in current series, will drop it.

>> +
>>  static bool kvm_vfio_group_is_coherent(struct vfio_group *vfio_group)
>>  {
>>  	long (*fn)(struct vfio_group *, unsigned long);
>> @@ -155,6 +181,8 @@ static int kvm_vfio_set_group(struct kvm_device *dev, long attr, u64 arg)
>>  		list_add_tail(&kvg->node, &kv->group_list);
>>  		kvg->vfio_group = vfio_group;
>>  
>> +		kvm_vfio_group_set_kvm(vfio_group, dev->kvm);
>> +
>>  		kvm_arch_start_assignment(dev->kvm);
>>  
>>  		mutex_unlock(&kv->lock);
>> @@ -196,6 +224,8 @@ static int kvm_vfio_set_group(struct kvm_device *dev, long attr, u64 arg)
>>  
>>  		mutex_unlock(&kv->lock);
>>  
>> +		kvm_vfio_group_clear_kvm(vfio_group, dev->kvm);
>> +
> 
> 
> Why did we set_kvm within kv->lock, but clear it outside of kv->lock?
> I'm not sure kv->lock is particularly relevant to us but we might as
> well be consistent.

kv->lock is not to protect the vfio_group anyway, so being out of the lock
should be safe. But placing set_kvm before acquiring kv->lock will generate
more code in error path, so I guess the better way is to place it after
unlocking. Will reflect that in next version.

>>  		kvm_vfio_group_put_external_user(vfio_group);
>>  
>>  		kvm_vfio_update_coherency(dev);
>> @@ -240,6 +270,7 @@ static void kvm_vfio_destroy(struct kvm_device *dev)
>>  	struct kvm_vfio_group *kvg, *tmp;
>>  
>>  	list_for_each_entry_safe(kvg, tmp, &kv->group_list, node) {
>> +		kvm_vfio_group_clear_kvm(kvg->vfio_group, dev->kvm);
>>  		kvm_vfio_group_put_external_user(kvg->vfio_group);
>>  		list_del(&kvg->node);
>>  		kfree(kvg);
> 

--
Thanks,
Jike