On Fri, 2014-09-12 at 17:55 +0100, Will Deacon wrote:
> Hi Alex,
>
> Thanks for taking a look.
>
> On Thu, Sep 11, 2014 at 08:12:28PM +0100, Alex Williamson wrote:
> > On Tue, 2014-09-02 at 10:53 +0100, Will Deacon wrote:
> > > diff --git a/drivers/vfio/vfio_iommu_type1.c
> > > b/drivers/vfio/vfio_iommu_type1.c
> > > index 0734fbe5b651..50aa56c7b6a7 100644
> > > --- a/drivers/vfio/vfio_iommu_type1.c
> > > +++ b/drivers/vfio/vfio_iommu_type1.c
> > > @@ -53,11 +53,15 @@ module_param_named(disable_hugepages,
> > > MODULE_PARM_DESC(disable_hugepages,
> > > "Disable VFIO IOMMU support for IOMMU hugepages.");
> > >
> > > +/* Feature flags for VFIO Type-1 IOMMUs */
> > > +#define VFIO_IOMMU_FEAT_V2 (1 << 0)
> > > +#define VFIO_IOMMU_FEAT_NESTING (1 << 1)
> > > +
> > > struct vfio_iommu {
> > > struct list_head domain_list;
> > > struct mutex lock;
> > > struct rb_root dma_list;
> > > - bool v2;
> > > + int features;
> >
> > Personally I'd rather add a nesting bool than mask out flag bits.
>
> Sure.
>
> > > @@ -705,6 +710,9 @@ static int vfio_iommu_type1_attach_group(void
> > > *iommu_data,
> > > goto out_free;
> > > }
> > >
> > > + if (iommu->features & VFIO_IOMMU_FEAT_NESTING)
> > > + iommu_domain_set_attr(domain->domain, DOMAIN_ATTR_NESTING,
> > > &attr);
> > > +
> >
> > Shouldn't we fail if this doesn't work?
>
> The reason I don't fail is because device passthrough can still work with an
> IOMMU that isn't capable of nesting. The part that won't work is attempting
> to instantiate a virtual SMMU interface for the guest.
>
> However, it just struck me that userspace knows whether or not it's going to
> want a virtual SMMU interface. If it does, then failure to set NESTING is
> fatal. If it doesn't, then it doesn't need to set the flag so there is no
> issue.
>
> > > ret = iommu_attach_group(domain->domain, iommu_group);
> > > if (ret)
> > > goto out_domain;
> > > @@ -815,12 +823,50 @@ done:
> > > mutex_unlock(&iommu->lock);
> > > }
> > >
> > > +static int vfio_iommus_have_nesting(void)
> > > +{
> > > + static struct bus_type *bus_types[] = { &pci_bus_type };
> >
> > Hmm, we just got rid of all traces of pci in this file, so I'm not too
> > excited about adding it back. Don't we no longer include pci.h? I'm
> > surprised this doesn't throw a warning. We should try to be independent
> > of CONFIG_PCI here.
>
> I don't see any warning here.
>
> > > + if (!iommu_present(bus))
> > > + continue;
> > > +
> > > + domain = iommu_domain_alloc(bus);
> > > + if (!domain)
> > > + continue;
> > > +
> > > + if (!iommu_domain_set_attr(domain, DOMAIN_ATTR_NESTING, &attr))
> > > + ret = 1;
> > > +
> > > + iommu_domain_free(domain);
> > > + }
> >
> > Can this support Joerg's iommu_capable() interface so we can test w/o
> > creating a domain? We still have the bus_type problem though.
>
> [...]
>
> > > @@ -886,6 +955,10 @@ static long vfio_iommu_type1_ioctl(void *iommu_data,
> > > case VFIO_TYPE1_IOMMU:
> > > case VFIO_TYPE1v2_IOMMU:
> > > return 1;
> > > + case VFIO_TYPE1_NESTING_IOMMU:
> > > + if (!iommu)
> > > + return vfio_iommus_have_nesting();
> > > + return vfio_domains_have_iommu_nesting(iommu);
> >
> > I'd just about prefer to statically return 1 for NESTING and then fail
> > to add groups if we can't set the attribute. It sucks, but here we test
> > an arbitrary bus type to determine whether nesting is supported, allow a
> > nesting iommu type to be set regardless of what kind of devices are
> > about to go into it, and require that the user test to see whether the
> > requested IOMMU attribute actually stuck. That's not a very intuitive
> > interface. Thanks,
>
> Yes, I totally agree that the interface is clunky. Returning 1 for NESTING
> solves both the pci bus problem and the iommu_capable issue, so that sounds
> like a great simplification of the code.
>
> That means userspace:
>
> - Can always open a VFIO_TYPE1_NESTING_IOMMU
> - VFIO_CHECK_EXTENSION will always return 1
> - When adding groups (e.g. SET_IOMMU), the iommu_domain_set_attr can
> fail, which will cause the corresponding ioctl to fail
>
> If nesting is not required because there is no virtual SMMU interface,
> then the usual TYPE1 IOMMU can be used.
>
> Sound good?
It's not 100% satisfying, but I can't think of anything better.
VFIO_TYPE1_NESTING_IOMMU is then just a VFIO level indication of support
with no indication of underlying hardware support. The user doesn't
know until they add a group if it's really possible. Not ideal from a
user perspective, but VFIO has always left itself the option of
disallowing groups from being added to a container. Thanks,
Alex
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
