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

[Alex Williamson <alex.williamson@redhat.com>]

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.

> 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,

Alex