iommu.lists.linux-foundation.org archive mirror
 help / color / mirror / Atom feed
From: Joerg Roedel <joro@8bytes.org>
To: Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>
Cc: Ashok Raj <ashok.raj@intel.com>,
	Will Deacon <will.deacon@arm.com>,
	iommu@lists.linux-foundation.org,
	Robin Murphy <robin.murphy@arm.com>,
	Christoph Hellwig <hch@lst.de>
Subject: Re: [PATCH V4 1/3] iommu: Add support to change default domain of an iommu group
Date: Tue, 30 Jun 2020 11:16:01 +0200	[thread overview]
Message-ID: <20200630091601.GB28824@8bytes.org> (raw)
In-Reply-To: <3d7f5cf067437593e681751108860ea692bb10b1.1591290586.git.sai.praneeth.prakhya@intel.com>

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".

> +		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"

> +		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.

> +	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.

> +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().
       
You also need to take a reference with get_device() and then drop the
group->mutex.

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. Then you
can call down to iommu_change_dev_def_domain() which does not need to
take the group-mutex by itself.

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

  parent reply	other threads:[~2020-06-30  9:16 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-06-05  1:32 [PATCH V4 0/3] " Sai Praneeth Prakhya
2020-06-05  1:32 ` [PATCH V4 1/3] " Sai Praneeth Prakhya
2020-06-08  1:49   ` Lu Baolu
2020-06-30  9:16   ` Joerg Roedel [this message]
2020-07-01  3:04     ` Prakhya, Sai Praneeth
2020-07-14 18:23       ` Prakhya, Sai Praneeth
2020-07-22 13:52         ` Joerg Roedel
2020-07-22 17:14           ` Prakhya, Sai Praneeth
2020-06-05  1:32 ` [PATCH V4 2/3] iommu: Take lock before reading iommu group default domain type Sai Praneeth Prakhya
2020-06-08  1:50   ` Lu Baolu
2020-06-05  1:32 ` [PATCH V4 3/3] iommu: Document usage of "/sys/kernel/iommu_groups/<grp_id>/type" file Sai Praneeth Prakhya
2020-06-08  1:50   ` Lu Baolu
2020-06-26 21:34 ` [PATCH V4 0/3] iommu: Add support to change default domain of an iommu group Prakhya, Sai Praneeth

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=20200630091601.GB28824@8bytes.org \
    --to=joro@8bytes.org \
    --cc=ashok.raj@intel.com \
    --cc=hch@lst.de \
    --cc=iommu@lists.linux-foundation.org \
    --cc=robin.murphy@arm.com \
    --cc=sai.praneeth.prakhya@intel.com \
    --cc=will.deacon@arm.com \
    --subject='Re: [PATCH V4 1/3] iommu: Add support to change default domain of an iommu group' \
    /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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).