On Wed, 15 Mar 2017 15:40:14 +1100
David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> 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
