IOMMU Archive on lore.kernel.org
 help / color / Atom feed
From: Alex Williamson <alex.williamson@redhat.com>
To: Lu Baolu <baolu.lu@linux.intel.com>
Cc: Jean-Philippe Brucker <jean-philippe@linaro.org>,
	Kevin Tian <kevin.tian@intel.com>,
	Dave Jiang <dave.jiang@intel.com>,
	Ashok Raj <ashok.raj@intel.com>,
	kvm@vger.kernel.org, Cornelia Huck <cohuck@redhat.com>,
	linux-kernel@vger.kernel.org, iommu@lists.linux-foundation.org,
	Robin Murphy <robin.murphy@arm.com>
Subject: Re: [PATCH v3 2/4] iommu: Add iommu_aux_at(de)tach_group()
Date: Wed, 29 Jul 2020 14:03:36 -0600
Message-ID: <20200729140336.09d2bfe7@x1.home> (raw)
In-Reply-To: <435a2014-c2e8-06b9-3c9a-4afbf6607ffe@linux.intel.com>

On Thu, 16 Jul 2020 09:07:46 +0800
Lu Baolu <baolu.lu@linux.intel.com> wrote:

> Hi Jacob,
> 
> On 7/16/20 12:01 AM, Jacob Pan wrote:
> > On Wed, 15 Jul 2020 08:47:36 +0800
> > Lu Baolu <baolu.lu@linux.intel.com> wrote:
> >   
> >> Hi Jacob,
> >>
> >> On 7/15/20 12:39 AM, Jacob Pan wrote:  
> >>> On Tue, 14 Jul 2020 13:57:01 +0800
> >>> Lu Baolu<baolu.lu@linux.intel.com>  wrote:
> >>>      
> >>>> This adds two new aux-domain APIs for a use case like vfio/mdev
> >>>> where sub-devices derived from an aux-domain capable device are
> >>>> created and put in an iommu_group.
> >>>>
> >>>> /**
> >>>>    * iommu_aux_attach_group - attach an aux-domain to an iommu_group
> >>>> which
> >>>>    *                          contains sub-devices (for example
> >>>> mdevs) derived
> >>>>    *                          from @dev.
> >>>>    * @domain: an aux-domain;
> >>>>    * @group:  an iommu_group which contains sub-devices derived from
> >>>> @dev;
> >>>>    * @dev:    the physical device which supports IOMMU_DEV_FEAT_AUX.
> >>>>    *
> >>>>    * Returns 0 on success, or an error value.
> >>>>    */
> >>>> int iommu_aux_attach_group(struct iommu_domain *domain,
> >>>>                              struct iommu_group *group,
> >>>>                              struct device *dev)
> >>>>
> >>>> /**
> >>>>    * iommu_aux_detach_group - detach an aux-domain from an
> >>>> iommu_group *
> >>>>    * @domain: an aux-domain;
> >>>>    * @group:  an iommu_group which contains sub-devices derived from
> >>>> @dev;
> >>>>    * @dev:    the physical device which supports IOMMU_DEV_FEAT_AUX.
> >>>>    *
> >>>>    * @domain must have been attached to @group via
> >>>> iommu_aux_attach_group(). */
> >>>> void iommu_aux_detach_group(struct iommu_domain *domain,
> >>>>                               struct iommu_group *group,
> >>>>                               struct device *dev)
> >>>>
> >>>> It also adds a flag in the iommu_group data structure to identify
> >>>> an iommu_group with aux-domain attached from those normal ones.
> >>>>
> >>>> Signed-off-by: Lu Baolu<baolu.lu@linux.intel.com>
> >>>> ---
> >>>>    drivers/iommu/iommu.c | 58
> >>>> +++++++++++++++++++++++++++++++++++++++++++ include/linux/iommu.h |
> >>>> 17 +++++++++++++ 2 files changed, 75 insertions(+)
> >>>>
> >>>> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> >>>> index e1fdd3531d65..cad5a19ebf22 100644
> >>>> --- a/drivers/iommu/iommu.c
> >>>> +++ b/drivers/iommu/iommu.c
> >>>> @@ -45,6 +45,7 @@ struct iommu_group {
> >>>>    	struct iommu_domain *default_domain;
> >>>>    	struct iommu_domain *domain;
> >>>>    	struct list_head entry;
> >>>> +	unsigned int aux_domain_attached:1;
> >>>>    };
> >>>>    
> >>>>    struct group_device {
> >>>> @@ -2759,6 +2760,63 @@ int iommu_aux_get_pasid(struct iommu_domain
> >>>> *domain, struct device *dev) }
> >>>>    EXPORT_SYMBOL_GPL(iommu_aux_get_pasid);
> >>>>    
> >>>> +/**
> >>>> + * iommu_aux_attach_group - attach an aux-domain to an iommu_group
> >>>> which
> >>>> + *                          contains sub-devices (for example
> >>>> mdevs) derived
> >>>> + *                          from @dev.
> >>>> + * @domain: an aux-domain;
> >>>> + * @group:  an iommu_group which contains sub-devices derived from
> >>>> @dev;
> >>>> + * @dev:    the physical device which supports IOMMU_DEV_FEAT_AUX.
> >>>> + *
> >>>> + * Returns 0 on success, or an error value.
> >>>> + */
> >>>> +int iommu_aux_attach_group(struct iommu_domain *domain,
> >>>> +			   struct iommu_group *group, struct
> >>>> device *dev) +{
> >>>> +	int ret = -EBUSY;
> >>>> +
> >>>> +	mutex_lock(&group->mutex);
> >>>> +	if (group->domain)
> >>>> +		goto out_unlock;
> >>>> +  
> >>> Perhaps I missed something but are we assuming only one mdev per
> >>> mdev group? That seems to change the logic where vfio does:
> >>> iommu_group_for_each_dev()
> >>> 	iommu_aux_attach_device()
> >>>      
> >>
> >> It has been changed in PATCH 4/4:
> >>
> >> static int vfio_iommu_attach_group(struct vfio_domain *domain,
> >>                                      struct vfio_group *group)
> >> {
> >>           if (group->mdev_group)
> >>                   return iommu_aux_attach_group(domain->domain,
> >>                                                 group->iommu_group,
> >>                                                 group->iommu_device);
> >>           else
> >>                   return iommu_attach_group(domain->domain,
> >> group->iommu_group);
> >> }
> >>
> >> So, for both normal domain and aux-domain, we use the same concept:
> >> attach a domain to a group.
> >>  
> > I get that, but don't you have to attach all the devices within the  
> 
> This iommu_group includes only mediated devices derived from an
> IOMMU_DEV_FEAT_AUX-capable device. Different from iommu_attach_group(),
> iommu_aux_attach_group() doesn't need to attach the domain to each
> device in group, instead it only needs to attach the domain to the
> physical device where the mdev's were created from.
> 
> > group? Here you see the group already has a domain and exit.  
> 
> If the (group->domain) has been set, that means a domain has already
> attached to the group, so it returns -EBUSY.

I agree with Jacob, singleton groups should not be built into the IOMMU
API, we're not building an interface just for mdevs or current
limitations of mdevs.  This also means that setting a flag on the group
and passing a device that's assumed to be common for all devices within
the group, don't really make sense here.  Thanks,

Alex

_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

  reply index

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-14  5:56 [PATCH v3 0/4] iommu aux-domain APIs extensions Lu Baolu
2020-07-14  5:57 ` [PATCH v3 1/4] iommu: Check IOMMU_DEV_FEAT_AUX feature in aux api's Lu Baolu
2020-07-29 20:03   ` Alex Williamson
2020-07-30  1:46     ` Lu Baolu
2020-07-14  5:57 ` [PATCH v3 2/4] iommu: Add iommu_aux_at(de)tach_group() Lu Baolu
2020-07-14 16:39   ` Jacob Pan
2020-07-15  0:47     ` Lu Baolu
2020-07-15 16:01       ` Jacob Pan
2020-07-16  1:07         ` Lu Baolu
2020-07-29 20:03           ` Alex Williamson [this message]
2020-07-29 23:34             ` Tian, Kevin
2020-07-30 19:46               ` Alex Williamson
2020-07-31  5:47                 ` Lu Baolu
2020-07-31 18:05                   ` Alex Williamson
2020-08-03  1:57                     ` Lu Baolu
2020-07-14  5:57 ` [PATCH v3 3/4] iommu: Add iommu_aux_get_domain_for_dev() Lu Baolu
2020-07-29 20:25   ` Alex Williamson
2020-07-29 23:49     ` Tian, Kevin
2020-07-30 20:17       ` Alex Williamson
2020-07-31  0:26         ` Tian, Kevin
2020-07-31  2:17         ` Tian, Kevin
2020-07-31  6:30     ` Lu Baolu
2020-07-31 18:14       ` Alex Williamson
2020-08-03  2:15         ` Lu Baolu
2020-07-14  5:57 ` [PATCH v3 4/4] vfio/type1: Use iommu_aux_at(de)tach_group() APIs Lu Baolu
2020-07-14  8:25   ` Christoph Hellwig
2020-07-14 16:29     ` Jacob Pan
2020-07-15  1:00       ` Lu Baolu
2020-07-15  1:23         ` Tian, Kevin
2020-07-29 20:32   ` Alex Williamson
2020-07-30  2:41     ` Lu Baolu
2020-07-30 21:17       ` Alex Williamson
2020-07-31  1:37         ` Lu Baolu
2020-07-30  9:36   ` Liu, Yi L
2020-07-31  1:39     ` Lu Baolu
2020-07-23 13:55 ` [PATCH v3 0/4] iommu aux-domain APIs extensions Lu Baolu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200729140336.09d2bfe7@x1.home \
    --to=alex.williamson@redhat.com \
    --cc=ashok.raj@intel.com \
    --cc=baolu.lu@linux.intel.com \
    --cc=cohuck@redhat.com \
    --cc=dave.jiang@intel.com \
    --cc=iommu@lists.linux-foundation.org \
    --cc=jean-philippe@linaro.org \
    --cc=kevin.tian@intel.com \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=robin.murphy@arm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

IOMMU Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-iommu/0 linux-iommu/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-iommu linux-iommu/ https://lore.kernel.org/linux-iommu \
		iommu@lists.linux-foundation.org
	public-inbox-index linux-iommu

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.linux-foundation.lists.iommu


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git