Re: [RFC PATCH v2 08/10] vfio/type1: Add domain at(de)taching group helpers

[Lu Baolu]

Hi,

On 09/19/2018 07:26 AM, Tian, Kevin wrote:
> > From: Jean-Philippe Brucker [mailto:jean-philippe.brucker@xxxxxxx]
> > Sent: Tuesday, September 18, 2018 11:52 PM
> >
> > On 15/09/2018 03:36, Tian, Kevin wrote:
> > > > 4) Userspace opens another mdev.
> > > > -> iommu.c calls domain->ops->attach_dev(domain2, dev)
> > >
> > > another mdev in same VFIO container or different? I assume the
> > > latter since you mentioned a new domain2.
> >
> > I was thinking a different VFIO container actually. I used domain2 to
> > try to make the example clearer
> >
> > > > 1)? When the container is closed, VFIO calls
> > > > iommu_detach_device(domain2, parent_dev)
> > > > -> iommu.c calls default_domain->ops->attach_dev(default_domain,
> > dev)
> > > > Given that auxiliary domains are attached, the IOMMU driver could
> > deduce
> > > > that this actually means "detach an auxiliary domain". But which one?
> > >
> > > I didn't get this one. There is no need to stick to 1) behavior for
> > > 4), i.e. below is expected:
> > >  ÂÂÂÂÂÂÂ domain2->ops->detach_dev(domain2, dev)
> >
> > But in order to get that, the IOMMU core needs to know that domain2 is
> > auxiliary. Otherwise, detach_dev is never called when a default_domain
> > is present for the parent dev.
> >
> > I guess one solution is to add an "auxiliary" attribute to iommu_domain,
> > so __iommu_detach_group would do something like:
>
> this doesn't work. same domain can be also attached to another physical
> device as non-aux domain (e.g. passthrough) at the same time (vfio-pci
> device and vfio-mdev device in same container), then default domain
> tweak is required in that case. "aux" takes effect only per-device, not
> per-domain.

If we have below APIs for aux domain (the API names are just for
discussion purpose, subject to change):

iommu_querry_aux_domain_capability(dev)
iommu_enable_aux_domain(dev)
iommu_disable_aux_domain(dev)
iommu_check_aux_domain_status(dev)

then, we could do this like below:

diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index ab3d7d3b1583..3bfb652c78e8 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1469,12 +1469,31 @@ static int iommu_group_do_detach_device(struct 
device *dev, void *data)
        return 0;
 }

+static int iommu_group_check_aux_domain(struct device *dev, void *data)
+{
+       const struct iommu_ops *ops = dev->bus->iommu_ops;
+
+       if (ops && ops->check_auxd)
+               return !ops->check_auxd(dev);
+
+       return -EINVAL;
+}
+
+/*
+ *  Check whether devices in @group have aux domain enabled.
+ */
+static int iommu_group_aux_domain_enabled(struct iommu_group *group)
+{
+       return __iommu_group_for_each_dev(group, NULL,
+                                         iommu_group_check_aux_domain);
+}
+
 static void __iommu_detach_group(struct iommu_domain *domain,
                                 struct iommu_group *group)
 {
        int ret;

-       if (!group->default_domain) {
+       if (!group->default_domain || 
iommu_group_aux_domain_enabled(group)) {
                __iommu_group_for_each_dev(group, domain,
                                           iommu_group_do_detach_device);
                group->domain = NULL;

Best regards,
Lu Baolu

> > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > index 7113fe398b70..2b3e9b91aee7 100644
> > --- a/drivers/iommu/iommu.c
> > +++ b/drivers/iommu/iommu.c
> > @@ -1786,10 +1786,11 @@ static void __iommu_detach_group(struct
> > iommu_domain *domain,
> >   {
> >   	int ret;
> >
> > -	if (!group->default_domain) {
> > +	if (!group->default_domain || domain->auxiliary) {
> >   		__iommu_group_for_each_dev(group, domain,
> >   					   iommu_group_do_detach_device);
> > -		group->domain = NULL;
> > +		if (!domain->auxiliary)
> > +			group->domain = NULL;
> >   		return;
> >   	}
> >
> > Not sure who would set this "auxiliary" attribute... Maybe the IOMMU
> > driver, when attaching the domain to a device that has auxiliary mode
> > enabled?
> >
> > > why cannot ARM implement a detach_dev for aux_domain too? My
> > > feeling is that default domain twist is only for switch between 1/2/3
> > > in concept.
> >
> > If the core actually calls it, we can implement detach_dev :) The
> > problem is that the core never calls detach_dev when default_domain is
> > present (affects any IOMMU driver that relies on default_domain,
> > including AMD), and even in case 4) the default_domain is present for
> > the parent device
>
> Then can we change that core logic so detach_dev is invoked in all
> cases? yes there will be some changes in vendor drivers, but I expect
> this change trivial (especially considering the gain in IOMMU API
> simplicity side as described below).
>
> > > > So the proposed interface doesn't seem to work as is. If we want to use
> > > > iommu_attach/detach_device for auxiliary domains, the existing
> > behavior
> > > > of iommu.c, and IOMMU drivers that rely on it, have to change. Any
> > > > change I can think of right now seems more daunting than introducing a
> > > > different path for auxiliary domains, like iommu_attach_aux_domain for
> > > > example.
> > >
> > > introducing *aux* specific API will cause different VFIO code path to
> > > handle RID-based and PASID-based mdev, since RID-based still needs
> > > to use normal attach_domain that way.
> >
> > The PASID-based mdev still requires a special case to retrieve the
> > allocated PASID and program it in the parent device, so VFIO will need
> > to know the difference between the two
>
> that retrieve/program is down by parent driver, instead of VFIO.
>
> Thanks
> Kevin