Re: [PATCH 3/4] KVM: PPC: Add support for IOMMU in-kernel handling

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3827 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 198 bytes --]