From: "Prakhya, Sai Praneeth" <sai.praneeth.prakhya@intel.com>
To: "Prakhya, Sai Praneeth" <sai.praneeth.prakhya@intel.com>,
Joerg Roedel <joro@8bytes.org>
Cc: Robin Murphy <robin.murphy@arm.com>,
"iommu@lists.linux-foundation.org"
<iommu@lists.linux-foundation.org>,
Will Deacon <will.deacon@arm.com>,
"Raj, Ashok" <ashok.raj@intel.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, 14 Jul 2020 18:23:54 +0000 [thread overview]
Message-ID: <FFF73D592F13FD46B8700F0A279B802F573DB164@ORSMSX114.amr.corp.intel.com> (raw)
In-Reply-To: <FFF73D592F13FD46B8700F0A279B802F573D8042@ORSMSX114.amr.corp.intel.com>
Hi Joerg,
Replying again because I noticed that I couldn't find this mail in the external iommu mailing list while I was able to find your comments on my patch. Also, could you please answer my two questions below?
> -----Original Message-----
> From: iommu <iommu-bounces@lists.linux-foundation.org> On Behalf Of
> Prakhya, Sai Praneeth
> Sent: Tuesday, June 30, 2020 8:04 PM
> To: Joerg Roedel <joro@8bytes.org>
> Cc: Raj, Ashok <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
>
> 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.
Q1:
> 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.
Q2:
> 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
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
next prev parent reply other threads:[~2020-07-14 18:24 UTC|newest]
Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-06-05 1:32 [PATCH V4 0/3] iommu: Add support to change default domain of an iommu group 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
2020-07-01 3:04 ` Prakhya, Sai Praneeth
2020-07-14 18:23 ` Prakhya, Sai Praneeth [this message]
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=FFF73D592F13FD46B8700F0A279B802F573DB164@ORSMSX114.amr.corp.intel.com \
--to=sai.praneeth.prakhya@intel.com \
--cc=ashok.raj@intel.com \
--cc=hch@lst.de \
--cc=iommu@lists.linux-foundation.org \
--cc=joro@8bytes.org \
--cc=robin.murphy@arm.com \
--cc=will.deacon@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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).