linux-kernel - RE: [PATCH v3 3/4] iommu: Add iommu_aux_get_domain_for

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Fri, 31 Jul 2020 00:26:06 +0000
From:   "Tian, Kevin" <kevin.tian@...el.com>
To:     Alex Williamson <alex.williamson@...hat.com>
CC:     Lu Baolu <baolu.lu@...ux.intel.com>,
        Joerg Roedel <joro@...tes.org>,
        Robin Murphy <robin.murphy@....com>,
        Jean-Philippe Brucker <jean-philippe@...aro.org>,
        Cornelia Huck <cohuck@...hat.com>,
        "Raj, Ashok" <ashok.raj@...el.com>,
        "Jiang, Dave" <dave.jiang@...el.com>,
        "Liu, Yi L" <yi.l.liu@...el.com>,
        "iommu@...ts.linux-foundation.org" <iommu@...ts.linux-foundation.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>
Subject: RE: [PATCH v3 3/4] iommu: Add iommu_aux_get_domain_for_dev()

> From: Alex Williamson <alex.williamson@...hat.com>
> Sent: Friday, July 31, 2020 4:17 AM
> 
> On Wed, 29 Jul 2020 23:49:20 +0000
> "Tian, Kevin" <kevin.tian@...el.com> wrote:
> 
> > > From: Alex Williamson <alex.williamson@...hat.com>
> > > Sent: Thursday, July 30, 2020 4:25 AM
> > >
> > > On Tue, 14 Jul 2020 13:57:02 +0800
> > > Lu Baolu <baolu.lu@...ux.intel.com> wrote:
> > >
> > > > The device driver needs an API to get its aux-domain. A typical usage
> > > > scenario is:
> > > >
> > > >         unsigned long pasid;
> > > >         struct iommu_domain *domain;
> > > >         struct device *dev = mdev_dev(mdev);
> > > >         struct device *iommu_device = vfio_mdev_get_iommu_device(dev);
> > > >
> > > >         domain = iommu_aux_get_domain_for_dev(dev);
> > > >         if (!domain)
> > > >                 return -ENODEV;
> > > >
> > > >         pasid = iommu_aux_get_pasid(domain, iommu_device);
> > > >         if (pasid <= 0)
> > > >                 return -EINVAL;
> > > >
> > > >          /* Program the device context */
> > > >          ....
> > > >
> > > > This adds an API for such use case.
> > > >
> > > > Suggested-by: Alex Williamson <alex.williamson@...hat.com>
> > > > Signed-off-by: Lu Baolu <baolu.lu@...ux.intel.com>
> > > > ---
> > > >  drivers/iommu/iommu.c | 18 ++++++++++++++++++
> > > >  include/linux/iommu.h |  7 +++++++
> > > >  2 files changed, 25 insertions(+)
> > > >
> > > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> > > > index cad5a19ebf22..434bf42b6b9b 100644
> > > > --- a/drivers/iommu/iommu.c
> > > > +++ b/drivers/iommu/iommu.c
> > > > @@ -2817,6 +2817,24 @@ void iommu_aux_detach_group(struct
> > > iommu_domain *domain,
> > > >  }
> > > >  EXPORT_SYMBOL_GPL(iommu_aux_detach_group);
> > > >
> > > > +struct iommu_domain *iommu_aux_get_domain_for_dev(struct
> device
> > > *dev)
> > > > +{
> > > > +	struct iommu_domain *domain = NULL;
> > > > +	struct iommu_group *group;
> > > > +
> > > > +	group = iommu_group_get(dev);
> > > > +	if (!group)
> > > > +		return NULL;
> > > > +
> > > > +	if (group->aux_domain_attached)
> > > > +		domain = group->domain;
> > >
> > > Why wouldn't the aux domain flag be on the domain itself rather than
> > > the group?  Then if we wanted sanity checking in patch 1/ we'd only
> > > need to test the flag on the object we're provided.
> > >
> > > If we had such a flag, we could create an iommu_domain_is_aux()
> > > function and then simply use iommu_get_domain_for_dev() and test that
> > > it's an aux domain in the example use case.  It seems like that would
> >
> > IOMMU layer manages domains per parent device. Here given a
> 
> Is this the IOMMU layer or the VT-d driver?  I don't see any notion of
> managing domains relative to a parent in the IOMMU layer.  Please point
> to something more specific if I'm wrong here.

it's maintained in VT-d driver (include/linux/intel-iommu.h)

struct device_domain_info {
        struct list_head link;  /* link to domain siblings */
        struct list_head global; /* link to global list */
        struct list_head table; /* link to pasid table */
        struct list_head auxiliary_domains; /* auxiliary domains
                                             * attached to this device
                                             */
	...

> 
> > dev (of mdev), we need a way to find its associated domain under its
> > parent device. And we cannot simply use iommu_get_domain_for_dev
> > on the parent device of the mdev, as it will give us the primary domain
> > of parent device.
> 
> Not the parent device of the mdev, but the mdev_dev(mdev) device.
> Isn't that what this series is enabling, being able to return the
> domain from the group that contains the mdev_dev?  We shouldn't need to
> leave breadcrumbs on the group to know about the domain, the domain
> itself should be the source of knowledge, or provide a mechanism/ops to
> learn that knowledge.  Thanks,
> 
> Alex

It's the tradeoff between leaving breadcrumb in domain or in group. 
Today the domain has no knowledge of mdev. It just includes a list
of physical devices which are attached to the domain (either due to
the device is assigned in a whole or as the parent device of an assigned
mdev). Then we have two choices. One is to save the mdev_dev info
in device_domain_info and maintain a mapping between mdev_dev
and related aux domain. The other is to record the domain info directly
in group. Earlier we choose the latter one as it looks simpler. If you
prefer to the former one, we can think more and have a try.

Thanks
Kevin