From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jike Song Subject: Re: [v7 3/3] kvm: set/clear kvm to/from vfio_group when group add/delete Date: Thu, 01 Dec 2016 10:47:13 +0800 Message-ID: <583F8F31.6060700@intel.com> References: <1479794973-11910-1-git-send-email-jike.song@intel.com> <1479794973-11910-4-git-send-email-jike.song@intel.com> <20161130100239.6689cf7f@t450s.home> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Cc: pbonzini@redhat.com, guangrong.xiao@linux.intel.com, kevin.tian@intel.com, kwankhede@nvidia.com, cjia@nvidia.com, kvm@vger.kernel.org To: Alex Williamson Return-path: Received: from mga01.intel.com ([192.55.52.88]:63296 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754884AbcLACrg (ORCPT ); Wed, 30 Nov 2016 21:47:36 -0500 In-Reply-To: <20161130100239.6689cf7f@t450s.home> Sender: kvm-owner@vger.kernel.org List-ID: On 12/01/2016 01:02 AM, Alex Williamson wrote: > On Tue, 22 Nov 2016 14:09:33 +0800 > Jike Song 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 >> Cc: Kirti Wankhede >> Cc: Paolo Bonzini >> Cc: Xiao Guangrong >> Signed-off-by: Jike Song >> --- >> 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