From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexey Kardashevskiy Date: Thu, 20 Jun 2013 04:58:18 +0000 Subject: Re: [PATCH 3/4] KVM: PPC: Add support for IOMMU in-kernel handling Message-Id: <51C28BEA.8050501@ozlabs.ru> List-Id: References: <1370412673-1345-1-git-send-email-aik@ozlabs.ru> <1370412673-1345-4-git-send-email-aik@ozlabs.ru> <1371422343.21896.143.camel@pasglop> <1371438800.22681.38.camel@ul30vt.home> <1371441361.21896.152.camel@pasglop> <1371522772.22681.140.camel@ul30vt.home> <87txkun568.fsf@rustcorp.com.au> <1371617970.21896.232.camel@pasglop> <1371653443.21896.291.camel@pasglop> <1371656989.22659.98.camel@ul30vt.home> In-Reply-To: <1371656989.22659.98.camel@ul30vt.home> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable To: Alex Williamson Cc: Benjamin Herrenschmidt , Alexander Graf , linuxppc-dev@lists.ozlabs.org, David Gibson , Paul Mackerras , "kvm@vger.kernel.org mailing list" , open list , kvm-ppc@vger.kernel.org, Rusty Russell , Joerg Roedel On 06/20/2013 01:49 AM, Alex Williamson wrote: > On Thu, 2013-06-20 at 00:50 +1000, Benjamin Herrenschmidt wrote: >> On Wed, 2013-06-19 at 11:58 +0200, Alexander Graf wrote: >> >>>> Alex, any objection ? >>> >>> Which Alex? :) >> >> Heh, mostly Williamson in this specific case but your input is still >> welcome :-) >> >>> I think validate works, it keeps iteration logic out of the kernel >>> which is a good thing. There still needs to be an interface for >>> getting the iommu id in VFIO, but I suppose that one's for the other >>> Alex and J=C3=B6rg to comment on. >> >> I think getting the iommu fd is already covered by separate patches from >> Alexey. >> >>>> >>>> Do we need to make it a get/put interface instead ? >>>> >>>> vfio_validate_and_use_iommu(file, iommu_id); >>>> >>>> vfio_release_iommu(file, iommu_id); >>>> >>>> To ensure that the resource remains owned by the process until KVM >>>> is closed as well ? >>>> >>>> Or do we want to register with VFIO with a callback so that VFIO can >>>> call us if it needs us to give it up ? >>> >>> Can't we just register a handler on the fd and get notified when it >>> closes? Can you kill VFIO access without closing the fd? >> >> That sounds actually harder :-) >> >> The question is basically: When we validate that relationship between a >> specific VFIO struct file with an iommu, what is the lifetime of that >> and how do we handle this lifetime properly. >> >> There's two ways for that sort of situation: The notification model >> where we get notified when the relationship is broken, and the refcount >> model where we become a "user" and thus delay the breaking of the >> relationship until we have been disposed of as well. >> >> In this specific case, it's hard to tell what is the right model from my >> perspective, which is why I would welcome Alex (W.) input. >> >> In the end, the solution will end up being in the form of APIs exposed >> by VFIO for use by KVM (via that symbol lookup mechanism) so Alex (W), >> as owner of VFIO at this stage, what do you want those to look >> like ? :-) >=20 > My first thought is that we should use the same reference counting as we > have for vfio devices (group->container_users). An interface for that > might look like: >=20 > int vfio_group_add_external_user(struct file *filep) > { > struct vfio_group *group =3D filep->private_data; >=20 > if (filep->f_op !=3D &vfio_group_fops) > return -EINVAL; >=20 >=20 > if (!atomic_inc_not_zero(&group->container_users)) > return -EINVAL; >=20 > return 0; > } >=20 > void vfio_group_del_external_user(struct file *filep) > { > struct vfio_group *group =3D filep->private_data; >=20 > BUG_ON(filep->f_op !=3D &vfio_group_fops); >=20 > vfio_group_try_dissolve_container(group); > } >=20 > int vfio_group_iommu_id_from_file(struct file *filep) > { > struct vfio_group *group =3D filep->private_data; >=20 > BUG_ON(filep->f_op !=3D &vfio_group_fops); >=20 > return iommu_group_id(group->iommu_group); > } >=20 > Would that work? Thanks, Just out of curiosity - would not get_file() and fput_atomic() on a group's file* do the right job instead of vfio_group_add_external_user() and vfio_group_del_external_user()? --=20 Alexey From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pa0-x22a.google.com (mail-pa0-x22a.google.com [IPv6:2607:f8b0:400e:c03::22a]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority" (not verified)) by ozlabs.org (Postfix) with ESMTPS id 366432C02A7 for ; Thu, 20 Jun 2013 14:58:31 +1000 (EST) Received: by mail-pa0-f42.google.com with SMTP id rl6so5906373pac.1 for ; Wed, 19 Jun 2013 21:58:28 -0700 (PDT) Message-ID: <51C28BEA.8050501@ozlabs.ru> Date: Thu, 20 Jun 2013 14:58:18 +1000 From: Alexey Kardashevskiy MIME-Version: 1.0 To: Alex Williamson Subject: Re: [PATCH 3/4] KVM: PPC: Add support for IOMMU in-kernel handling References: <1370412673-1345-1-git-send-email-aik@ozlabs.ru> <1370412673-1345-4-git-send-email-aik@ozlabs.ru> <1371422343.21896.143.camel@pasglop> <1371438800.22681.38.camel@ul30vt.home> <1371441361.21896.152.camel@pasglop> <1371522772.22681.140.camel@ul30vt.home> <87txkun568.fsf@rustcorp.com.au> <1371617970.21896.232.camel@pasglop> <1371653443.21896.291.camel@pasglop> <1371656989.22659.98.camel@ul30vt.home> In-Reply-To: <1371656989.22659.98.camel@ul30vt.home> Content-Type: text/plain; charset=UTF-8 Cc: "kvm@vger.kernel.org mailing list" , Joerg Roedel , Rusty Russell , Alexander Graf , kvm-ppc@vger.kernel.org, open list , Paul Mackerras , linuxppc-dev@lists.ozlabs.org, David Gibson List-Id: Linux on PowerPC Developers Mail List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , On 06/20/2013 01:49 AM, Alex Williamson wrote: > On Thu, 2013-06-20 at 00:50 +1000, Benjamin Herrenschmidt wrote: >> On Wed, 2013-06-19 at 11:58 +0200, Alexander Graf wrote: >> >>>> Alex, any objection ? >>> >>> Which Alex? :) >> >> Heh, mostly Williamson in this specific case but your input is still >> welcome :-) >> >>> I think validate works, it keeps iteration logic out of the kernel >>> which is a good thing. There still needs to be an interface for >>> getting the iommu id in VFIO, but I suppose that one's for the other >>> Alex and Jörg to comment on. >> >> I think getting the iommu fd is already covered by separate patches from >> Alexey. >> >>>> >>>> Do we need to make it a get/put interface instead ? >>>> >>>> vfio_validate_and_use_iommu(file, iommu_id); >>>> >>>> vfio_release_iommu(file, iommu_id); >>>> >>>> To ensure that the resource remains owned by the process until KVM >>>> is closed as well ? >>>> >>>> Or do we want to register with VFIO with a callback so that VFIO can >>>> call us if it needs us to give it up ? >>> >>> Can't we just register a handler on the fd and get notified when it >>> closes? Can you kill VFIO access without closing the fd? >> >> That sounds actually harder :-) >> >> The question is basically: When we validate that relationship between a >> specific VFIO struct file with an iommu, what is the lifetime of that >> and how do we handle this lifetime properly. >> >> There's two ways for that sort of situation: The notification model >> where we get notified when the relationship is broken, and the refcount >> model where we become a "user" and thus delay the breaking of the >> relationship until we have been disposed of as well. >> >> In this specific case, it's hard to tell what is the right model from my >> perspective, which is why I would welcome Alex (W.) input. >> >> In the end, the solution will end up being in the form of APIs exposed >> by VFIO for use by KVM (via that symbol lookup mechanism) so Alex (W), >> as owner of VFIO at this stage, what do you want those to look >> like ? :-) > > My first thought is that we should use the same reference counting as we > have for vfio devices (group->container_users). An interface for that > might look like: > > int vfio_group_add_external_user(struct file *filep) > { > struct vfio_group *group = filep->private_data; > > if (filep->f_op != &vfio_group_fops) > return -EINVAL; > > > if (!atomic_inc_not_zero(&group->container_users)) > return -EINVAL; > > return 0; > } > > void vfio_group_del_external_user(struct file *filep) > { > struct vfio_group *group = filep->private_data; > > BUG_ON(filep->f_op != &vfio_group_fops); > > vfio_group_try_dissolve_container(group); > } > > int vfio_group_iommu_id_from_file(struct file *filep) > { > struct vfio_group *group = filep->private_data; > > BUG_ON(filep->f_op != &vfio_group_fops); > > return iommu_group_id(group->iommu_group); > } > > Would that work? Thanks, Just out of curiosity - would not get_file() and fput_atomic() on a group's file* do the right job instead of vfio_group_add_external_user() and vfio_group_del_external_user()? -- Alexey From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexey Kardashevskiy Subject: Re: [PATCH 3/4] KVM: PPC: Add support for IOMMU in-kernel handling Date: Thu, 20 Jun 2013 14:58:18 +1000 Message-ID: <51C28BEA.8050501@ozlabs.ru> References: <1370412673-1345-1-git-send-email-aik@ozlabs.ru> <1370412673-1345-4-git-send-email-aik@ozlabs.ru> <1371422343.21896.143.camel@pasglop> <1371438800.22681.38.camel@ul30vt.home> <1371441361.21896.152.camel@pasglop> <1371522772.22681.140.camel@ul30vt.home> <87txkun568.fsf@rustcorp.com.au> <1371617970.21896.232.camel@pasglop> <1371653443.21896.291.camel@pasglop> <1371656989.22659.98.camel@ul30vt.home> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Benjamin Herrenschmidt , Alexander Graf , linuxppc-dev@lists.ozlabs.org, David Gibson , Paul Mackerras , "kvm@vger.kernel.org mailing list" , open list , kvm-ppc@vger.kernel.org, Rusty Russell , Joerg Roedel To: Alex Williamson Return-path: In-Reply-To: <1371656989.22659.98.camel@ul30vt.home> Sender: kvm-ppc-owner@vger.kernel.org List-Id: kvm.vger.kernel.org On 06/20/2013 01:49 AM, Alex Williamson wrote: > On Thu, 2013-06-20 at 00:50 +1000, Benjamin Herrenschmidt wrote: >> On Wed, 2013-06-19 at 11:58 +0200, Alexander Graf wrote: >> >>>> Alex, any objection ? >>> >>> Which Alex? :) >> >> Heh, mostly Williamson in this specific case but your input is still >> welcome :-) >> >>> I think validate works, it keeps iteration logic out of the kernel >>> which is a good thing. There still needs to be an interface for >>> getting the iommu id in VFIO, but I suppose that one's for the othe= r >>> Alex and J=C3=B6rg to comment on. >> >> I think getting the iommu fd is already covered by separate patches = from >> Alexey. >> >>>> >>>> Do we need to make it a get/put interface instead ? >>>> >>>> vfio_validate_and_use_iommu(file, iommu_id); >>>> >>>> vfio_release_iommu(file, iommu_id); >>>> >>>> To ensure that the resource remains owned by the process until KVM >>>> is closed as well ? >>>> >>>> Or do we want to register with VFIO with a callback so that VFIO c= an >>>> call us if it needs us to give it up ? >>> >>> Can't we just register a handler on the fd and get notified when it >>> closes? Can you kill VFIO access without closing the fd? >> >> That sounds actually harder :-) >> >> The question is basically: When we validate that relationship betwee= n a >> specific VFIO struct file with an iommu, what is the lifetime of tha= t >> and how do we handle this lifetime properly. >> >> There's two ways for that sort of situation: The notification model >> where we get notified when the relationship is broken, and the refco= unt >> model where we become a "user" and thus delay the breaking of the >> relationship until we have been disposed of as well. >> >> In this specific case, it's hard to tell what is the right model fro= m my >> perspective, which is why I would welcome Alex (W.) input. >> >> In the end, the solution will end up being in the form of APIs expos= ed >> by VFIO for use by KVM (via that symbol lookup mechanism) so Alex (W= ), >> as owner of VFIO at this stage, what do you want those to look >> like ? :-) >=20 > My first thought is that we should use the same reference counting as= we > have for vfio devices (group->container_users). An interface for tha= t > might look like: >=20 > int vfio_group_add_external_user(struct file *filep) > { > struct vfio_group *group =3D filep->private_data; >=20 > if (filep->f_op !=3D &vfio_group_fops) > return -EINVAL; >=20 >=20 > if (!atomic_inc_not_zero(&group->container_users)) > return -EINVAL; >=20 > return 0; > } >=20 > void vfio_group_del_external_user(struct file *filep) > { > struct vfio_group *group =3D filep->private_data; >=20 > BUG_ON(filep->f_op !=3D &vfio_group_fops); >=20 > vfio_group_try_dissolve_container(group); > } >=20 > int vfio_group_iommu_id_from_file(struct file *filep) > { > struct vfio_group *group =3D filep->private_data; >=20 > BUG_ON(filep->f_op !=3D &vfio_group_fops); >=20 > return iommu_group_id(group->iommu_group); > } >=20 > Would that work? Thanks, Just out of curiosity - would not get_file() and fput_atomic() on a gro= up's file* do the right job instead of vfio_group_add_external_user() and vfio_group_del_external_user()? --=20 Alexey