Re: [Patch V8 1/3] iommu: Add support to change default domain of an iommu group
Hi Shameer,
On 2020/11/20 19:27, Shameerali Kolothum Thodi wrote:
Hi Baolu/Ashok,
-Original Message-
From: iommu [mailto:iommu-boun...@lists.linux-foundation.org] On Behalf Of
Ashok Raj
Sent: 25 September 2020 20:06
To: Joerg Roedel ; iommu@lists.linux-foundation.org
Cc: Ashok Raj ; Will Deacon ;
Robin Murphy ; Christoph Hellwig
Subject: [Patch V8 1/3] iommu: Add support to change default domain of an
iommu group
From: Sai Praneeth Prakhya
Presently, the default domain of an iommu group is allocated during boot
time and it cannot be changed later. So, the device would typically be
either in identity (also known as pass_through) mode or the device would be
in DMA mode as long as the machine is up and running. There is no way to
change the default domain type dynamically i.e. after booting, a device
cannot switch between identity mode and DMA mode.
But, assume a use case wherein the user trusts the device and believes that
the OS is secure enough and hence wants *only* this device to bypass IOMMU
(so that it could be high performing) whereas all the other devices to go
through IOMMU (so that the system is protected). Presently, this use case
is not supported. It will be helpful if there is some way to change the
default domain of an iommu group dynamically. Hence, add such support.
A privileged user could request the kernel to change the default domain
type of a iommu group by writing to
"/sys/kernel/iommu_groups//type" file. Presently, only three values
are supported
1. identity: all the DMA transactions from the device in this group are
*not* translated by the iommu
2. DMA: all the DMA transactions from the device in this group are
translated by the iommu
3. auto: change to the type the device was booted with
Note:
1. Default domain of an iommu group with two or more devices cannot be
changed.
2. The device in the iommu group shouldn't be bound to any driver.
3. The device shouldn't be assigned to user for direct access.
4. The vendor iommu driver is required to add def_domain_type() callback.
The change request will fail if the request type conflicts with that
returned from the callback.
Currently Arm SMMUv3 driver doesn't provide the def_doman_type() callback.
And I have sent a patch[1] based on this series to just add that. But Robin made
couple of suggestions there which can be incorporated into this series so that
the
vendor driver no more required to provide the callback for this feature to work.
1. Include a generic checking for
if (dev_is_pci(dev)) {
if (pci_dev->untrusted)
return IOMMU_DOMAIN_DMA;
}
To be honest, I have the same idea. Okay! I can do this in the next
version.
2. Also if there is no def_doman_type() callback provided by vendor driver,
assume
that the dev supports both IDENTITY and DMA domain types.
It's true for boot case. I will assume this in this series.
If you plan to respin this series, could you please consider the above as well?
Please let me know.
Sure!
Thanks,
Shameer
Best regards,
baolu
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [Patch V8 1/3] iommu: Add support to change default domain of an iommu group
On Fri, Nov 20, 2020 at 10:11:58AM +0800, Lu Baolu wrote: > On 11/19/20 4:53 PM, Will Deacon wrote: > > On Thu, Nov 19, 2020 at 10:18:05AM +0800, Lu Baolu wrote: > > > On 11/18/20 9:51 PM, Will Deacon wrote: > > > > On Fri, Sep 25, 2020 at 12:06:18PM -0700, Ashok Raj wrote: > > > > > From: Sai Praneeth Prakhya > > > > [...] > > > > > > > +free_new_domain: > > > > > + iommu_domain_free(group->default_domain); > > > > > + group->default_domain = prev_dom; > > > > > + group->domain = prev_dom;i > > > > > > > > Hmm. This seems to rely on all users of group->default_domain holding > > > > the > > > > group->mutex. Have you confirmed that this is the case? There's a funny > > > > use of iommu_group_get() in the exynos IOMMU driver at least. > > > > > > Emm. This change happens within the area with group->mutex held. Or I > > > am not getting your point? > > > > Yeah, sorry, I wasn't very clear. This code holds the group->mutex, and it > > relies on _anybody_ else who wants to inspect group->default_domain also > > holding that mutex, otherwise they could observe a transient domain pointer > > which we free on the failure path here. > > Clear to me now. Thanks for explanation. :-) > > Changing default domain through sysfs requires the users to ubind any > driver from the devices in the group. There's a check code and return > failure if this requirement doesn't meet. > > So we only need to consider the device release path. device_lock(dev) is > used in this patch to guarantee that no device release happens at the > same time. Aha, thanks. Please can you add a comment for future reference? > > > > > My question is whether or not there is code that inspects > > group->default_domain without group->mutex held? The exynos case doesn't > > obviously hold it, and I'd like to make sure that there aren't others that > > we need to worry about. > > I searched the code. The exynos is the only case that inspects > group->default_domain without holding the mutex during run time. It's in > the device release path, so I think it's safe. Great, thanks for looking. Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [Patch V8 1/3] iommu: Add support to change default domain of an iommu group
Hi Will, On 11/19/20 4:53 PM, Will Deacon wrote: On Thu, Nov 19, 2020 at 10:18:05AM +0800, Lu Baolu wrote: The original author of this patch series has left Intel. I am now the backup. Ok, thanks for letting me know. On 11/18/20 9:51 PM, Will Deacon wrote: On Fri, Sep 25, 2020 at 12:06:18PM -0700, Ashok Raj wrote: From: Sai Praneeth Prakhya [...] +free_new_domain: + iommu_domain_free(group->default_domain); + group->default_domain = prev_dom; + group->domain = prev_dom;i Hmm. This seems to rely on all users of group->default_domain holding the group->mutex. Have you confirmed that this is the case? There's a funny use of iommu_group_get() in the exynos IOMMU driver at least. Emm. This change happens within the area with group->mutex held. Or I am not getting your point? Yeah, sorry, I wasn't very clear. This code holds the group->mutex, and it relies on _anybody_ else who wants to inspect group->default_domain also holding that mutex, otherwise they could observe a transient domain pointer which we free on the failure path here. Clear to me now. Thanks for explanation. :-) Changing default domain through sysfs requires the users to ubind any driver from the devices in the group. There's a check code and return failure if this requirement doesn't meet. So we only need to consider the device release path. device_lock(dev) is used in this patch to guarantee that no device release happens at the same time. My question is whether or not there is code that inspects group->default_domain without group->mutex held? The exynos case doesn't obviously hold it, and I'd like to make sure that there aren't others that we need to worry about. I searched the code. The exynos is the only case that inspects group->default_domain without holding the mutex during run time. It's in the device release path, so I think it's safe. Does that make more sense? Yes. Thanks! Thanks, Will Best regards, baolu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [Patch V8 1/3] iommu: Add support to change default domain of an iommu group
On Thu, Nov 19, 2020 at 10:18:05AM +0800, Lu Baolu wrote: > The original author of this patch series has left Intel. I am now the > backup. Ok, thanks for letting me know. > On 11/18/20 9:51 PM, Will Deacon wrote: > > On Fri, Sep 25, 2020 at 12:06:18PM -0700, Ashok Raj wrote: > > > From: Sai Praneeth Prakhya [...] > > > +free_new_domain: > > > + iommu_domain_free(group->default_domain); > > > + group->default_domain = prev_dom; > > > + group->domain = prev_dom;i > > > > Hmm. This seems to rely on all users of group->default_domain holding the > > group->mutex. Have you confirmed that this is the case? There's a funny > > use of iommu_group_get() in the exynos IOMMU driver at least. > > Emm. This change happens within the area with group->mutex held. Or I > am not getting your point? Yeah, sorry, I wasn't very clear. This code holds the group->mutex, and it relies on _anybody_ else who wants to inspect group->default_domain also holding that mutex, otherwise they could observe a transient domain pointer which we free on the failure path here. My question is whether or not there is code that inspects group->default_domain without group->mutex held? The exynos case doesn't obviously hold it, and I'd like to make sure that there aren't others that we need to worry about. Does that make more sense? Thanks, Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [Patch V8 1/3] iommu: Add support to change default domain of an iommu group
Hi Will,
The original author of this patch series has left Intel. I am now the
backup.
On 11/18/20 9:51 PM, Will Deacon wrote:
On Fri, Sep 25, 2020 at 12:06:18PM -0700, Ashok Raj wrote:
From: Sai Praneeth Prakhya
Presently, the default domain of an iommu group is allocated during boot
time and it cannot be changed later. So, the device would typically be
either in identity (also known as pass_through) mode or the device would be
in DMA mode as long as the machine is up and running. There is no way to
change the default domain type dynamically i.e. after booting, a device
cannot switch between identity mode and DMA mode.
But, assume a use case wherein the user trusts the device and believes that
the OS is secure enough and hence wants *only* this device to bypass IOMMU
(so that it could be high performing) whereas all the other devices to go
through IOMMU (so that the system is protected). Presently, this use case
is not supported. It will be helpful if there is some way to change the
default domain of an iommu group dynamically. Hence, add such support.
A privileged user could request the kernel to change the default domain
type of a iommu group by writing to
"/sys/kernel/iommu_groups//type" file. Presently, only three values
are supported
1. identity: all the DMA transactions from the device in this group are
*not* translated by the iommu
2. DMA: all the DMA transactions from the device in this group are
translated by the iommu
3. auto: change to the type the device was booted with
Note:
1. Default domain of an iommu group with two or more devices cannot be
changed.
2. The device in the iommu group shouldn't be bound to any driver.
3. The device shouldn't be assigned to user for direct access.
4. The vendor iommu driver is required to add def_domain_type() callback.
The change request will fail if the request type conflicts with that
returned from the callback.
Please see "Documentation/ABI/testing/sysfs-kernel-iommu_groups" for more
information.
Cc: Christoph Hellwig
Cc: Joerg Roedel
Cc: Ashok Raj
Cc: Will Deacon
Cc: Lu Baolu
Cc: Sohil Mehta
Cc: Robin Murphy
Cc: Jacob Pan
Reviewed-by: Lu Baolu
Signed-off-by: Sai Praneeth Prakhya
---
drivers/iommu/iommu.c | 225 +-
1 file changed, 224 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 6c14c88cd525..2e93c48ce248 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -93,6 +93,8 @@ static void __iommu_detach_group(struct iommu_domain *domain,
static int iommu_create_device_direct_mappings(struct iommu_group *group,
struct device *dev);
static struct iommu_group *iommu_group_get_for_dev(struct device *dev);
+static ssize_t iommu_group_store_type(struct iommu_group *group,
+ const char *buf, size_t count);
#define IOMMU_GROUP_ATTR(_name, _mode, _show, _store) \
struct iommu_group_attribute iommu_group_attr_##_name = \
@@ -525,7 +527,8 @@ static IOMMU_GROUP_ATTR(name, S_IRUGO,
iommu_group_show_name, NULL);
static IOMMU_GROUP_ATTR(reserved_regions, 0444,
iommu_group_show_resv_regions, NULL);
-static IOMMU_GROUP_ATTR(type, 0444, iommu_group_show_type, NULL);
+static IOMMU_GROUP_ATTR(type, 0644, iommu_group_show_type,
+ iommu_group_store_type);
static void iommu_group_release(struct kobject *kobj)
{
@@ -2849,3 +2852,223 @@ int iommu_sva_get_pasid(struct iommu_sva *handle)
return ops->sva_get_pasid(handle);
}
EXPORT_SYMBOL_GPL(iommu_sva_get_pasid);
+
+/*
+ * Changes the default domain of an iommu group that has *only* one device
+ *
+ * @group: The group for which the default domain should be changed
+ * @prev_dev: The device in the group (this is used to make sure that the
device
+ * hasn't changed after the caller has called this function)
+ * @type: The type of the new default domain that gets associated with the
group
+ *
+ * Returns 0 on success and error code on failure
+ *
+ * Note:
+ * 1. Presently, this function is called only when user requests to change the
+ *group's default domain type through
/sys/kernel/iommu_groups//type
+ *Please take a closer look if intended to use for other purposes.
+ */
+static int iommu_change_dev_def_domain(struct iommu_group *group,
+ struct device *prev_dev, 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(>mutex);
+
+ if (group->default_domain != group->domain) {
+ pr_err_ratelimited("Group not assigned to default domain\n");
This error is lacking any context. Can we use dev_err_ratelimited to make
Re: [Patch V8 1/3] iommu: Add support to change default domain of an iommu group
On Fri, Sep 25, 2020 at 12:06:18PM -0700, Ashok Raj wrote:
> From: Sai Praneeth Prakhya
>
> Presently, the default domain of an iommu group is allocated during boot
> time and it cannot be changed later. So, the device would typically be
> either in identity (also known as pass_through) mode or the device would be
> in DMA mode as long as the machine is up and running. There is no way to
> change the default domain type dynamically i.e. after booting, a device
> cannot switch between identity mode and DMA mode.
>
> But, assume a use case wherein the user trusts the device and believes that
> the OS is secure enough and hence wants *only* this device to bypass IOMMU
> (so that it could be high performing) whereas all the other devices to go
> through IOMMU (so that the system is protected). Presently, this use case
> is not supported. It will be helpful if there is some way to change the
> default domain of an iommu group dynamically. Hence, add such support.
>
> A privileged user could request the kernel to change the default domain
> type of a iommu group by writing to
> "/sys/kernel/iommu_groups//type" file. Presently, only three values
> are supported
> 1. identity: all the DMA transactions from the device in this group are
> *not* translated by the iommu
> 2. DMA: all the DMA transactions from the device in this group are
> translated by the iommu
> 3. auto: change to the type the device was booted with
>
> Note:
> 1. Default domain of an iommu group with two or more devices cannot be
>changed.
> 2. The device in the iommu group shouldn't be bound to any driver.
> 3. The device shouldn't be assigned to user for direct access.
> 4. The vendor iommu driver is required to add def_domain_type() callback.
>The change request will fail if the request type conflicts with that
>returned from the callback.
>
> Please see "Documentation/ABI/testing/sysfs-kernel-iommu_groups" for more
> information.
>
> Cc: Christoph Hellwig
> Cc: Joerg Roedel
> Cc: Ashok Raj
> Cc: Will Deacon
> Cc: Lu Baolu
> Cc: Sohil Mehta
> Cc: Robin Murphy
> Cc: Jacob Pan
> Reviewed-by: Lu Baolu
> Signed-off-by: Sai Praneeth Prakhya
> ---
> drivers/iommu/iommu.c | 225
> +-
> 1 file changed, 224 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 6c14c88cd525..2e93c48ce248 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -93,6 +93,8 @@ static void __iommu_detach_group(struct iommu_domain
> *domain,
> static int iommu_create_device_direct_mappings(struct iommu_group *group,
> struct device *dev);
> static struct iommu_group *iommu_group_get_for_dev(struct device *dev);
> +static ssize_t iommu_group_store_type(struct iommu_group *group,
> + const char *buf, size_t count);
>
> #define IOMMU_GROUP_ATTR(_name, _mode, _show, _store)\
> struct iommu_group_attribute iommu_group_attr_##_name = \
> @@ -525,7 +527,8 @@ static IOMMU_GROUP_ATTR(name, S_IRUGO,
> iommu_group_show_name, NULL);
> static IOMMU_GROUP_ATTR(reserved_regions, 0444,
> iommu_group_show_resv_regions, NULL);
>
> -static IOMMU_GROUP_ATTR(type, 0444, iommu_group_show_type, NULL);
> +static IOMMU_GROUP_ATTR(type, 0644, iommu_group_show_type,
> + iommu_group_store_type);
>
> static void iommu_group_release(struct kobject *kobj)
> {
> @@ -2849,3 +2852,223 @@ int iommu_sva_get_pasid(struct iommu_sva *handle)
> return ops->sva_get_pasid(handle);
> }
> EXPORT_SYMBOL_GPL(iommu_sva_get_pasid);
> +
> +/*
> + * Changes the default domain of an iommu group that has *only* one device
> + *
> + * @group: The group for which the default domain should be changed
> + * @prev_dev: The device in the group (this is used to make sure that the
> device
> + *hasn't changed after the caller has called this function)
> + * @type: The type of the new default domain that gets associated with the
> group
> + *
> + * Returns 0 on success and error code on failure
> + *
> + * Note:
> + * 1. Presently, this function is called only when user requests to change
> the
> + *group's default domain type through
> /sys/kernel/iommu_groups//type
> + *Please take a closer look if intended to use for other purposes.
> + */
> +static int iommu_change_dev_def_domain(struct iommu_group *group,
> +struct device *prev_dev, 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(>mutex);
> +
> + if (group->default_domain != group->domain) {
> + pr_err_ratelimited("Group not assigned to default domain\n");
This error
[Patch V8 1/3] iommu: Add support to change default domain of an iommu group
From: Sai Praneeth Prakhya
Presently, the default domain of an iommu group is allocated during boot
time and it cannot be changed later. So, the device would typically be
either in identity (also known as pass_through) mode or the device would be
in DMA mode as long as the machine is up and running. There is no way to
change the default domain type dynamically i.e. after booting, a device
cannot switch between identity mode and DMA mode.
But, assume a use case wherein the user trusts the device and believes that
the OS is secure enough and hence wants *only* this device to bypass IOMMU
(so that it could be high performing) whereas all the other devices to go
through IOMMU (so that the system is protected). Presently, this use case
is not supported. It will be helpful if there is some way to change the
default domain of an iommu group dynamically. Hence, add such support.
A privileged user could request the kernel to change the default domain
type of a iommu group by writing to
"/sys/kernel/iommu_groups//type" file. Presently, only three values
are supported
1. identity: all the DMA transactions from the device in this group are
*not* translated by the iommu
2. DMA: all the DMA transactions from the device in this group are
translated by the iommu
3. auto: change to the type the device was booted with
Note:
1. Default domain of an iommu group with two or more devices cannot be
changed.
2. The device in the iommu group shouldn't be bound to any driver.
3. The device shouldn't be assigned to user for direct access.
4. The vendor iommu driver is required to add def_domain_type() callback.
The change request will fail if the request type conflicts with that
returned from the callback.
Please see "Documentation/ABI/testing/sysfs-kernel-iommu_groups" for more
information.
Cc: Christoph Hellwig
Cc: Joerg Roedel
Cc: Ashok Raj
Cc: Will Deacon
Cc: Lu Baolu
Cc: Sohil Mehta
Cc: Robin Murphy
Cc: Jacob Pan
Reviewed-by: Lu Baolu
Signed-off-by: Sai Praneeth Prakhya
---
drivers/iommu/iommu.c | 225 +-
1 file changed, 224 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index 6c14c88cd525..2e93c48ce248 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -93,6 +93,8 @@ static void __iommu_detach_group(struct iommu_domain *domain,
static int iommu_create_device_direct_mappings(struct iommu_group *group,
struct device *dev);
static struct iommu_group *iommu_group_get_for_dev(struct device *dev);
+static ssize_t iommu_group_store_type(struct iommu_group *group,
+ const char *buf, size_t count);
#define IOMMU_GROUP_ATTR(_name, _mode, _show, _store) \
struct iommu_group_attribute iommu_group_attr_##_name =\
@@ -525,7 +527,8 @@ static IOMMU_GROUP_ATTR(name, S_IRUGO,
iommu_group_show_name, NULL);
static IOMMU_GROUP_ATTR(reserved_regions, 0444,
iommu_group_show_resv_regions, NULL);
-static IOMMU_GROUP_ATTR(type, 0444, iommu_group_show_type, NULL);
+static IOMMU_GROUP_ATTR(type, 0644, iommu_group_show_type,
+ iommu_group_store_type);
static void iommu_group_release(struct kobject *kobj)
{
@@ -2849,3 +2852,223 @@ int iommu_sva_get_pasid(struct iommu_sva *handle)
return ops->sva_get_pasid(handle);
}
EXPORT_SYMBOL_GPL(iommu_sva_get_pasid);
+
+/*
+ * Changes the default domain of an iommu group that has *only* one device
+ *
+ * @group: The group for which the default domain should be changed
+ * @prev_dev: The device in the group (this is used to make sure that the
device
+ * hasn't changed after the caller has called this function)
+ * @type: The type of the new default domain that gets associated with the
group
+ *
+ * Returns 0 on success and error code on failure
+ *
+ * Note:
+ * 1. Presently, this function is called only when user requests to change the
+ *group's default domain type through
/sys/kernel/iommu_groups//type
+ *Please take a closer look if intended to use for other purposes.
+ */
+static int iommu_change_dev_def_domain(struct iommu_group *group,
+ struct device *prev_dev, 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(>mutex);
+
+ if (group->default_domain != group->domain) {
+ pr_err_ratelimited("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.
+
