RE: [PATCH V4 1/3] iommu: Add support to change default domain of an iommu group

["Prakhya, Sai Praneeth" <sai.praneeth.prakhya@intel.com>]

Hi Joerg,

> -----Original Message-----
> From: Joerg Roedel <joro@8bytes.org>
> Sent: Tuesday, June 30, 2020 2:16 AM
> To: Prakhya, Sai Praneeth <sai.praneeth.prakhya@intel.com>
> Cc: iommu@lists.linux-foundation.org; Christoph Hellwig <hch@lst.de>; Raj,
> Ashok <ashok.raj@intel.com>; Will Deacon <will.deacon@arm.com>; Lu Baolu
> <baolu.lu@linux.intel.com>; Mehta, Sohil <sohil.mehta@intel.com>; Robin
> Murphy <robin.murphy@arm.com>; Jacob Pan <jacob.jun.pan@linux.intel.com>
> Subject: Re: [PATCH V4 1/3] iommu: Add support to change default domain of
> an iommu group
> 
> On Thu, Jun 04, 2020 at 06:32:06PM -0700, Sai Praneeth Prakhya wrote:
> > +static int iommu_change_dev_def_domain(struct iommu_group *group, int
> > +type) {
> > +	struct iommu_domain *prev_dom;
> > +	struct group_device *grp_dev;
> > +	const struct iommu_ops *ops;
> > +	int ret, dev_def_dom;
> > +	struct device *dev;
> > +
> > +	if (!group)
> > +		return -EINVAL;
> > +
> > +	mutex_lock(&group->mutex);
> > +
> > +	if (group->default_domain != group->domain) {
> > +		pr_err_ratelimited("Group assigned to user level for direct
> > +access\n");
> 
> Make this message: "Group not assigned to default domain\n".

Sure! I will change it

> > +		ret = -EBUSY;
> > +		goto out;
> > +	}
> > +
> > +	/*
> > +	 * iommu group wasn't locked while acquiring device lock in
> > +	 * iommu_group_store_type(). So, make sure that the device count
> hasn't
> > +	 * changed while acquiring device lock.
> > +	 *
> > +	 * Changing default domain of an iommu group with two or more
> devices
> > +	 * isn't supported because there could be a potential deadlock. Consider
> > +	 * the following scenario. T1 is trying to acquire device locks of all
> > +	 * the devices in the group and before it could acquire all of them,
> > +	 * there could be another thread T2 (from different sub-system and use
> > +	 * case) that has already acquired some of the device locks and might be
> > +	 * waiting for T1 to release other device locks.
> > +	 */
> > +	if (iommu_group_device_count(group) != 1) {
> > +		pr_err_ratelimited("Cannot change default domain of a group
> with
> > +two or more devices\n");
> 
> "Can not change default domain: Group has more than one device\n"

Ok.. make sense. I will change this.

> > +		ret = -EINVAL;
> > +		goto out;
> > +	}
> > +
> > +	/* Since group has only one device */
> > +	list_for_each_entry(grp_dev, &group->devices, list)
> > +		dev = grp_dev->dev;
> > +
> > +	prev_dom = group->default_domain;
> > +	if (!prev_dom || !prev_dom->ops || !prev_dom->ops-
> >def_domain_type) {
> > +		pr_err_ratelimited("'def_domain_type' call back isn't
> > +registered\n");
> 
> This message isn't needed.

Ok. I will remove it.

> > +	ret = __iommu_attach_device(group->default_domain, dev);
> > +	if (ret)
> > +		goto free_new_domain;
> > +
> > +	group->domain = group->default_domain;
> > +
> > +	ret = iommu_create_device_direct_mappings(group, dev);
> > +	if (ret)
> > +		goto free_new_domain;
> 
> You need to create the direct mappings before you attach the device to the new
> domain. Otherwise there might be a short time-window where RMRR regions
> are not mapped.

Ok.. makes sense. I will change this accordingly.

> > +static ssize_t iommu_group_store_type(struct iommu_group *group,
> > +				      const char *buf, size_t count) {
> > +	struct group_device *grp_dev;
> > +	struct device *dev;
> > +	int ret, req_type;
> > +
> > +	if (!capable(CAP_SYS_ADMIN) || !capable(CAP_SYS_RAWIO))
> > +		return -EACCES;
> > +
> > +	if (WARN_ON(!group))
> > +		return -EINVAL;
> > +
> > +	if (sysfs_streq(buf, "identity"))
> > +		req_type = IOMMU_DOMAIN_IDENTITY;
> > +	else if (sysfs_streq(buf, "DMA"))
> > +		req_type = IOMMU_DOMAIN_DMA;
> > +	else if (sysfs_streq(buf, "auto"))
> > +		req_type = 0;
> > +	else
> > +		return -EINVAL;
> > +
> > +	/*
> > +	 * Lock/Unlock the group mutex here before device lock to
> > +	 * 1. Make sure that the iommu group has only one device (this is a
> > +	 *    prerequisite for step 2)
> > +	 * 2. Get struct *dev which is needed to lock device
> > +	 */
> > +	mutex_lock(&group->mutex);
> > +	if (iommu_group_device_count(group) != 1) {
> > +		mutex_unlock(&group->mutex);
> > +		pr_err_ratelimited("Cannot change default domain of a group
> with two or more devices\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	/* Since group has only one device */
> > +	list_for_each_entry(grp_dev, &group->devices, list)
> > +		dev = grp_dev->dev;
> 
> Please use list_first_entry().

Ok.

> You also need to take a reference with get_device() and then drop the
> group->mutex.

Sure! I will change it.

> After device_lock() you need to verify that the device is still in the same group
> and that the group has still only one device in it.

Presently, iommu_change_dev_def_domain() checks if the iommu group still has
only one device or not. Hence, checking if iommu group has one device or not is done
twice, once before taking device_lock() and the other, after taking device_lock().

I agree that the code isn't checking if the iommu group still has the _same_ device or not.
One way, I could think of doing it is by storing "dev" temporarily and checking for it.
Do you think that's ok? Or would you rather suggest something else?

> Then you can call down to
> iommu_change_dev_def_domain() which does not need to take the group-
> mutex by itself.

The reason for taking iommu_group->mutex in the beginning of
iommu_change_dev_def_domain() is that the function

1. Checks if the group is being directly used by user level drivers
(i.e. if (group->default_domain != group->domain))

2. Uses iommu_ops
(prev_dom = group->default_domain;
if (!prev_dom || !prev_dom->ops || !prev_dom->ops->def_domain_type))

3. Sets iomu_group->domain to iommu_group->default_domain

I wanted to make sure that iommu_group->domain and
iommu_group->default_domain aren't changed by some other thread
while this thread is working on it. So, please let me know if I misunderstood something.

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