Re: [PATCH 3/4] KVM: PPC: Add support for IOMMU in-kernel handling
On Thu, Jun 20, 2013 at 02:58:18PM +1000, Alexey Kardashevskiy wrote:
> 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()?
I was thinking that too. Grabbing a file reference would certainly be
the usual way of handling this sort of thing.
--
David Gibson| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
pgpMnnXLgdz0e.pgp
Description: PGP signature
Re: [PATCH 3/4] KVM: PPC: Add support for IOMMU in-kernel handling
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
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] KVM: PPC: Add support for IOMMU in-kernel handling
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,
Alex
--
To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] KVM: PPC: Add support for IOMMU in-kernel handling
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 ? :-) Cheers, Ben. > > Alex > > -- > To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in > the body of a message to majord...@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] KVM: PPC: Add support for IOMMU in-kernel handling
On 19.06.2013, at 06:59, Benjamin Herrenschmidt wrote: > On Wed, 2013-06-19 at 13:05 +0930, Rusty Russell wrote: >> symbol_get() won't try to load a module; it'll just fail. This is what >> you want, since they must have vfio in the kernel to get a valid fd... > > Ok, cool. I suppose what we want here Alexey is slightly higher level, > something like: > > vfio_validate_iommu_id(file, iommu_id) > > Which verifies that the file that was passed in is allowed to use > that iommu_id. > > That's a simple and flexible interface (ie, it will work even if we > support multiple iommu IDs in the future for a vfio, for example > for DDW windows etc...), the logic to know about the ID remains > in qemu, this is strictly a validation call. > > That way we also don't have to expose the containing vfio struct etc... > just that simple function. > > Alex, any objection ? Which Alex? :) 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. > > 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? Alex -- To unsubscribe from this list: send the line "unsubscribe kvm-ppc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
