Re: [PATCH kernel v8 10/10] KVM: PPC: VFIO: Add in-kernel acceleration for VFIO

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

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

On Wed, Mar 15, 2017 at 10:18:18AM -0600, Alex Williamson wrote:
> On Wed, 15 Mar 2017 15:40:14 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> > > > diff --git a/arch/powerpc/kvm/book3s_64_vio.c b/arch/powerpc/kvm/book3s_64_vio.c
> > > > index e96a4590464c..be18cda01e1b 100644
> > > > --- a/arch/powerpc/kvm/book3s_64_vio.c
> > > > +++ b/arch/powerpc/kvm/book3s_64_vio.c
> > > > @@ -28,6 +28,10 @@
> > > >  #include <linux/hugetlb.h>
> > > >  #include <linux/list.h>
> > > >  #include <linux/anon_inodes.h>
> > > > +#include <linux/iommu.h>
> > > > +#include <linux/file.h>
> > > > +#include <linux/vfio.h>
> > > > +#include <linux/module.h>
> > > >  
> > > >  #include <asm/tlbflush.h>
> > > >  #include <asm/kvm_ppc.h>
> > > > @@ -40,6 +44,36 @@
> > > >  #include <asm/udbg.h>
> > > >  #include <asm/iommu.h>
> > > >  #include <asm/tce.h>
> > > > +#include <asm/mmu_context.h>
> > > > +
> > > > +static void kvm_vfio_group_put_external_user(struct vfio_group *vfio_group)
> > > > +{
> > > > +	void (*fn)(struct vfio_group *);
> > > > +
> > > > +	fn = symbol_get(vfio_group_put_external_user);
> > > > +	if (WARN_ON(!fn))
> > > > +		return;
> > > > +
> > > > +	fn(vfio_group);
> > > > +
> > > > +	symbol_put(vfio_group_put_external_user);
> > > > +}
> > > > +
> > > > +static int kvm_vfio_external_user_iommu_id(struct vfio_group *vfio_group)
> > > > +{
> > > > +	int (*fn)(struct vfio_group *);
> > > > +	int ret = -1;
> > > > +
> > > > +	fn = symbol_get(vfio_external_user_iommu_id);
> > > > +	if (!fn)
> > > > +		return ret;
> > > > +
> > > > +	ret = fn(vfio_group);
> > > > +
> > > > +	symbol_put(vfio_external_user_iommu_id);
> > > > +
> > > > +	return ret;
> > > > +}  
> > > 
> > > 
> > > Ugh.  This feels so wrong.  Why can't you have kvm-vfio pass the
> > > iommu_group?  Why do you need to hold this additional vfio_group
> > > reference?  
> > 
> > Keeping the vfio_group reference makes sense to me, since we don't
> > want the vfio context for the group to go away while it's attached to
> > the LIOBN.
> 
> But there's already a reference for that, it's taken by
> KVM_DEV_VFIO_GROUP_ADD and held until KVM_DEV_VFIO_GROUP_DEL.  Both the
> DEL path and the cleanup path call kvm_spapr_tce_release_iommu_group()
> before releasing that reference, so it seems entirely redundant.

Oh, good point.  And we already verify that the group has been ADDed
before setting the LIOBN association.

> > However, going via the iommu_id rather than just having an interface
> > to directly grab the iommu group from the vfio_group seems bizarre to
> > me.  I'm ok with cleaning that up later, however.
> 
> We have kvm_spapr_tce_attach_iommu_group() and
> kvm_spapr_tce_release_iommu_group(), but both take a vfio_group, not an
> iommu_group as a parameter.  I don't particularly have a problem with
> the vfio_group -> iommu ID -> iommu_group, but if we drop the extra
> vfio_group reference and pass the iommu_group itself to these functions
> then we can keep all the symbol reference stuff in the kvm-vfio glue
> layer.  Thanks,

Makes sense.

-- 
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: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]