Re: [PATCH 5/7] iommu: Add domain_type_supported() callback in iommu_ops
Robin, On 6/14/2022 4:51 PM, Robin Murphy wrote: On 2022-06-13 15:38, Suthikulpanit, Suravee wrote: Robin, On 6/13/2022 4:31 PM, Robin Murphy wrote: Introducing check_domain_type_supported() callback in iommu_ops, which allows IOMMU generic layer to check with vendor-specific IOMMU driver whether the requested type is supported. This allows user to request types other than the default type. Note also that you're only adding this in the sysfs path - what about the "iommu.passthrough=" parameter or CONFIG_IOMMU_DEFAULT_PASSTHROUGH? For SNP case, we cannot enable SNP if iommu=off or iommu=pt or iommu.passthrough=1 or CONFIG_IOMMU_DEFAULT_PASSTHROUGH=y. So, when another driver tries to enable SNP, the IOMMU driver prevents it (see iommu_sev_snp_supported() in patch 3). Ugh, I hadn't looked too closely at the other patches, but an interface that looks like a simple "is this feature supported?" check with a secret side-effect of changing global behaviour as well? Yuck :( What external drivers are expected to have the authority to affect the entire system and call that? The fact that you're exporting it suggests they could be loaded from modules *after* v2 features have been enabled and/or the user has configured a non-default identity domain for a group via sysfs... Fun! I see your point. Currently, the function to enable SNP will be called from SEV driver when it tries to enable SNP support globally on the system. This is done during fs_initcall(), which is early in the boot process. I can also add a guard code to make sure that this won't be done after a certain phase. Instead, if we boot with iommu.passhthrough=0, when another driver tries to enable SNP, the IOMMU driver allows this and switch to SNP enable mode. Subsequently, if user tries to switch a domain (via sysfs) to IOMMU_DOMAIN_IDENTITY, the IOMMU needs to prevent this because it has already switch to SNP-enabled mode. AFAICS there shouldn't need to be any core-level changes to support this. We already have drivers which don't support passthrough at all, so conditionally not supporting it should be no big deal. What should happen currently is that def_domain_type returns 0 for "don't care", then domain_alloc rejects IOMMU_DOMAIN_IDENTITY and and returns NULL, so iommu_group_alloc_default_domain() falls back to IOMMU_DOMAIN_DMA. Technically, we can do it the way you suggest. But isn't this confusing? At first, def_domain_type() returns 0 for "don't care", but then it rejects the request to change to IOMMU_DOMAIN_IDENTITY when trying to call domain_alloc(). Yes, that's how it works; def_domain_type is responsible for quirking individual *devices* that need to have a specific domain type (in practice, devices which need identity mapping), while domain_alloc is responsible for saying which domain types the driver supports as a whole, by allocating them or not as appropriate. We don't have a particularly neat way to achieve the negative of def_domain_type - i.e. saying that a specific device *can't* use a specific otherwise-supported domain type - other than subsequently failing in attach_dev, but so far we've not needed such a thing. And if SNP is expected to be mutually exclusive with identity domain support globally, then we still shouldn't need it. Thanks for your feedback. Suravee ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 5/7] iommu: Add domain_type_supported() callback in iommu_ops
On 2022-06-13 15:38, Suthikulpanit, Suravee wrote: Robin, On 6/13/2022 4:31 PM, Robin Murphy wrote: On 2022-06-13 02:25, Suravee Suthikulpanit wrote: When user requests to change IOMMU domain to a new type, IOMMU generic layer checks the requested type against the default domain type returned by vendor-specific IOMMU driver. However, there is only one default domain type, and current mechanism does not allow if the requested type does not match the default type. I don't really follow the reasoning here. If a driver's def_domain_type callback returns a specific type, it's saying that the device *has* to have that specific domain type for driver/platform-specific reasons. Agree, and I understand this part. If that's not the case, then the driver shouldn't say so in the first place. Considering the case: 1. Boot w/ default domain = IOMMU_DOMAIN_DMA_FQ 2. User wants to change to IOMMU_DOMAIN_IDENTITY, which is not supported by IOMMU driver. In this case, IOMMU driver can return IOMMU_DOMAIN_DMA_FQ and prevent the mode change. 3. However, if user want to change to IOMMU_DOMAIN_DMA. The driver can support this. However, since the def_domain_type() returns IOMMU_DOMAIN_DMA_FQ, it ends up prevent the mode change. Why would a driver be forcing IOMMU_DOMAIN_DMA_FQ for a device though? Nobody's doing that today, and semantically it wouldn't really make sense - forcing translation to deny passthrough on a device-specific basis (beyond the common handling of untrusted devices) *might* be a thing, but the performance/strictness tradeoff of using a flush queue or not is surely a subjective user decision, not an objective platform one. IIUC, we should support step 3 above. Basically, with the newly proposed interface, it allows us to check with IOMMU driver if it can support certain domain types before trying to allocate the domain. Indeed we could do that - as a much more comprehensive change to the internal domain_alloc interfaces - but do we really need to? If we succeed at allocating a domain then we know it's supported; if it fails then we can't give the user what they asked for, regardless of the exact reason why - what do we gain from doubling the number of potential failure paths that we have to handle? Introducing check_domain_type_supported() callback in iommu_ops, which allows IOMMU generic layer to check with vendor-specific IOMMU driver whether the requested type is supported. This allows user to request types other than the default type. Note also that you're only adding this in the sysfs path - what about the "iommu.passthrough=" parameter or CONFIG_IOMMU_DEFAULT_PASSTHROUGH? For SNP case, we cannot enable SNP if iommu=off or iommu=pt or iommu.passthrough=1 or CONFIG_IOMMU_DEFAULT_PASSTHROUGH=y. So, when another driver tries to enable SNP, the IOMMU driver prevents it (see iommu_sev_snp_supported() in patch 3). Ugh, I hadn't looked too closely at the other patches, but an interface that looks like a simple "is this feature supported?" check with a secret side-effect of changing global behaviour as well? Yuck :( What external drivers are expected to have the authority to affect the entire system and call that? The fact that you're exporting it suggests they could be loaded from modules *after* v2 features have been enabled and/or the user has configured a non-default identity domain for a group via sysfs... Fun! Instead, if we boot with iommu.passhthrough=0, when another driver tries to enable SNP, the IOMMU driver allows this and switch to SNP enable mode. Subsequently, if user tries to switch a domain (via sysfs) to IOMMU_DOMAIN_IDENTITY, the IOMMU needs to prevent this because it has already switch to SNP-enabled mode. AFAICS there shouldn't need to be any core-level changes to support this. We already have drivers which don't support passthrough at all, so conditionally not supporting it should be no big deal. What should happen currently is that def_domain_type returns 0 for "don't care", then domain_alloc rejects IOMMU_DOMAIN_IDENTITY and and returns NULL, so iommu_group_alloc_default_domain() falls back to IOMMU_DOMAIN_DMA. Technically, we can do it the way you suggest. But isn't this confusing? At first, def_domain_type() returns 0 for "don't care", but then it rejects the request to change to IOMMU_DOMAIN_IDENTITY when trying to call domain_alloc(). Yes, that's how it works; def_domain_type is responsible for quirking individual *devices* that need to have a specific domain type (in practice, devices which need identity mapping), while domain_alloc is responsible for saying which domain types the driver supports as a whole, by allocating them or not as appropriate. We don't have a particularly neat way to achieve the negative of def_domain_type - i.e. saying that a specific device *can't* use a specific otherwise-supported domain type - other than subsequently failing in attach_dev, but so far we've not
Re: [PATCH 5/7] iommu: Add domain_type_supported() callback in iommu_ops
Robin, On 6/13/2022 4:31 PM, Robin Murphy wrote: On 2022-06-13 02:25, Suravee Suthikulpanit wrote: When user requests to change IOMMU domain to a new type, IOMMU generic layer checks the requested type against the default domain type returned by vendor-specific IOMMU driver. However, there is only one default domain type, and current mechanism does not allow if the requested type does not match the default type. I don't really follow the reasoning here. If a driver's def_domain_type callback returns a specific type, it's saying that the device *has* to have that specific domain type for driver/platform-specific reasons. Agree, and I understand this part. If that's not the case, then the driver shouldn't say so in the first place. Considering the case: 1. Boot w/ default domain = IOMMU_DOMAIN_DMA_FQ 2. User wants to change to IOMMU_DOMAIN_IDENTITY, which is not supported by IOMMU driver. In this case, IOMMU driver can return IOMMU_DOMAIN_DMA_FQ and prevent the mode change. 3. However, if user want to change to IOMMU_DOMAIN_DMA. The driver can support this. However, since the def_domain_type() returns IOMMU_DOMAIN_DMA_FQ, it ends up prevent the mode change. IIUC, we should support step 3 above. Basically, with the newly proposed interface, it allows us to check with IOMMU driver if it can support certain domain types before trying to allocate the domain. Introducing check_domain_type_supported() callback in iommu_ops, which allows IOMMU generic layer to check with vendor-specific IOMMU driver whether the requested type is supported. This allows user to request types other than the default type. Note also that you're only adding this in the sysfs path - what about the "iommu.passthrough=" parameter or CONFIG_IOMMU_DEFAULT_PASSTHROUGH? For SNP case, we cannot enable SNP if iommu=off or iommu=pt or iommu.passthrough=1 or CONFIG_IOMMU_DEFAULT_PASSTHROUGH=y. So, when another driver tries to enable SNP, the IOMMU driver prevents it (see iommu_sev_snp_supported() in patch 3). Instead, if we boot with iommu.passhthrough=0, when another driver tries to enable SNP, the IOMMU driver allows this and switch to SNP enable mode. Subsequently, if user tries to switch a domain (via sysfs) to IOMMU_DOMAIN_IDENTITY, the IOMMU needs to prevent this because it has already switch to SNP-enabled mode. AFAICS there shouldn't need to be any core-level changes to support this. We already have drivers which don't support passthrough at all, so conditionally not supporting it should be no big deal. What should happen currently is that def_domain_type returns 0 for "don't care", then domain_alloc rejects IOMMU_DOMAIN_IDENTITY and and returns NULL, so iommu_group_alloc_default_domain() falls back to IOMMU_DOMAIN_DMA. Technically, we can do it the way you suggest. But isn't this confusing? At first, def_domain_type() returns 0 for "don't care", but then it rejects the request to change to IOMMU_DOMAIN_IDENTITY when trying to call domain_alloc(). Please let me know if I am missing something. Best Regards, Suravee ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 5/7] iommu: Add domain_type_supported() callback in iommu_ops
On 2022-06-13 02:25, Suravee Suthikulpanit wrote:
When user requests to change IOMMU domain to a new type, IOMMU generic
layer checks the requested type against the default domain type returned
by vendor-specific IOMMU driver.
However, there is only one default domain type, and current mechanism
does not allow if the requested type does not match the default type.
I don't really follow the reasoning here. If a driver's def_domain_type
callback returns a specific type, it's saying that the device *has* to
have that specific domain type for driver/platform-specific reasons. If
that's not the case, then the driver shouldn't say so in the first place.
Introducing check_domain_type_supported() callback in iommu_ops,
which allows IOMMU generic layer to check with vendor-specific IOMMU driver
whether the requested type is supported. This allows user to request
types other than the default type.
Note also that you're only adding this in the sysfs path - what about
the "iommu.passthrough=" parameter or CONFIG_IOMMU_DEFAULT_PASSTHROUGH?
AFAICS there shouldn't need to be any core-level changes to support
this. We already have drivers which don't support passthrough at all, so
conditionally not supporting it should be no big deal. What should
happen currently is that def_domain_type returns 0 for "don't care",
then domain_alloc rejects IOMMU_DOMAIN_IDENTITY and and returns NULL, so
iommu_group_alloc_default_domain() falls back to IOMMU_DOMAIN_DMA.
Thanks,
Robin.
Signed-off-by: Suravee Suthikulpanit
---
drivers/iommu/iommu.c | 13 -
include/linux/iommu.h | 2 ++
2 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index f2c45b85b9fc..4afb956ce083 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1521,6 +1521,16 @@ struct iommu_group *fsl_mc_device_group(struct device
*dev)
}
EXPORT_SYMBOL_GPL(fsl_mc_device_group);
+static bool iommu_domain_type_supported(struct device *dev, int type)
+{
+ const struct iommu_ops *ops = dev_iommu_ops(dev);
+
+ if (ops->domain_type_supported)
+ return ops->domain_type_supported(dev, type);
+
+ return true;
+}
+
static int iommu_get_def_domain_type(struct device *dev)
{
const struct iommu_ops *ops = dev_iommu_ops(dev);
@@ -2937,7 +2947,8 @@ static int iommu_change_dev_def_domain(struct iommu_group
*group,
* domain the device was booted with
*/
type = dev_def_dom ? : iommu_def_domain_type;
- } else if (dev_def_dom && type != dev_def_dom) {
+ } else if (!iommu_domain_type_supported(dev, type) ||
+ (dev_def_dom && type != dev_def_dom)) {
dev_err_ratelimited(prev_dev, "Device cannot be in %s domain\n",
iommu_domain_type_str(type));
ret = -EINVAL;
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index fecb72e1b11b..40c47ab15005 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -214,6 +214,7 @@ struct iommu_iotlb_gather {
*- IOMMU_DOMAIN_IDENTITY: must use an identity domain
*- IOMMU_DOMAIN_DMA: must use a dma domain
*- 0: use the default setting
+ * @domain_type_supported: check if the specified domain type is supported
* @default_domain_ops: the default ops for domains
* @pgsize_bitmap: bitmap of all possible supported page sizes
* @owner: Driver module providing these ops
@@ -252,6 +253,7 @@ struct iommu_ops {
struct iommu_page_response *msg);
int (*def_domain_type)(struct device *dev);
+ bool (*domain_type_supported)(struct device *dev, int type);
const struct iommu_domain_ops *default_domain_ops;
unsigned long pgsize_bitmap;
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 5/7] iommu: Add domain_type_supported() callback in iommu_ops
When user requests to change IOMMU domain to a new type, IOMMU generic
layer checks the requested type against the default domain type returned
by vendor-specific IOMMU driver.
However, there is only one default domain type, and current mechanism
does not allow if the requested type does not match the default type.
Introducing check_domain_type_supported() callback in iommu_ops,
which allows IOMMU generic layer to check with vendor-specific IOMMU driver
whether the requested type is supported. This allows user to request
types other than the default type.
Signed-off-by: Suravee Suthikulpanit
---
drivers/iommu/iommu.c | 13 -
include/linux/iommu.h | 2 ++
2 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
index f2c45b85b9fc..4afb956ce083 100644
--- a/drivers/iommu/iommu.c
+++ b/drivers/iommu/iommu.c
@@ -1521,6 +1521,16 @@ struct iommu_group *fsl_mc_device_group(struct device
*dev)
}
EXPORT_SYMBOL_GPL(fsl_mc_device_group);
+static bool iommu_domain_type_supported(struct device *dev, int type)
+{
+ const struct iommu_ops *ops = dev_iommu_ops(dev);
+
+ if (ops->domain_type_supported)
+ return ops->domain_type_supported(dev, type);
+
+ return true;
+}
+
static int iommu_get_def_domain_type(struct device *dev)
{
const struct iommu_ops *ops = dev_iommu_ops(dev);
@@ -2937,7 +2947,8 @@ static int iommu_change_dev_def_domain(struct iommu_group
*group,
* domain the device was booted with
*/
type = dev_def_dom ? : iommu_def_domain_type;
- } else if (dev_def_dom && type != dev_def_dom) {
+ } else if (!iommu_domain_type_supported(dev, type) ||
+ (dev_def_dom && type != dev_def_dom)) {
dev_err_ratelimited(prev_dev, "Device cannot be in %s domain\n",
iommu_domain_type_str(type));
ret = -EINVAL;
diff --git a/include/linux/iommu.h b/include/linux/iommu.h
index fecb72e1b11b..40c47ab15005 100644
--- a/include/linux/iommu.h
+++ b/include/linux/iommu.h
@@ -214,6 +214,7 @@ struct iommu_iotlb_gather {
* - IOMMU_DOMAIN_IDENTITY: must use an identity domain
* - IOMMU_DOMAIN_DMA: must use a dma domain
* - 0: use the default setting
+ * @domain_type_supported: check if the specified domain type is supported
* @default_domain_ops: the default ops for domains
* @pgsize_bitmap: bitmap of all possible supported page sizes
* @owner: Driver module providing these ops
@@ -252,6 +253,7 @@ struct iommu_ops {
struct iommu_page_response *msg);
int (*def_domain_type)(struct device *dev);
+ bool (*domain_type_supported)(struct device *dev, int type);
const struct iommu_domain_ops *default_domain_ops;
unsigned long pgsize_bitmap;
--
2.32.0
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
