Re: [PATCH v9 1/4] iommu: Move def_domain type check for untrusted device into core
On Mon, Nov 23, 2020 at 09:54:49PM +0800, Lu Baolu wrote: > Hi Will, > > On 2020/11/23 21:03, Will Deacon wrote: > > Hi Baolu, > > > > On Mon, Nov 23, 2020 at 08:55:17PM +0800, Lu Baolu wrote: > > > On 2020/11/23 20:04, Will Deacon wrote: > > > > On Sat, Nov 21, 2020 at 09:56:17PM +0800, Lu Baolu wrote: > > > > > @@ -1645,13 +1655,10 @@ struct __group_domain_type { > > > > >static int probe_get_default_domain_type(struct device *dev, void > > > > > *data) > > > > >{ > > > > > - const struct iommu_ops *ops = dev->bus->iommu_ops; > > > > > struct __group_domain_type *gtype = data; > > > > > unsigned int type = 0; > > > > > - if (ops->def_domain_type) > > > > > - type = ops->def_domain_type(dev); > > > > > - > > > > > + type = iommu_get_mandatory_def_domain_type(dev); > > > > > > > > afaict, this code is only called from probe_alloc_default_domain(), > > > > which > > > > has: > > > > > > > > /* Ask for default domain requirements of all devices in the > > > > group */ > > > > __iommu_group_for_each_dev(group, >ype, > > > > probe_get_default_domain_type); > > > > > > > > if (!gtype.type) > > > > gtype.type = iommu_def_domain_type; > > > > > > > > so is there actually a need to introduce the new > > > > iommu_get_mandatory_def_domain_type() function, given that a type of 0 > > > > always ends up resolving to the default domain type? > > > > > > Another consumer of this helper is in the next patch: > > > > > > + dev_def_dom = iommu_get_mandatory_def_domain_type(dev); > > > + > > > + /* Check if user requested domain is supported by the device or not */ > > > + if (!type) { > > > + /* > > > + * If the user hasn't requested any specific type of domain and > > > + * if the device supports both the domains, then default to the > > > + * domain the device was booted with > > > + */ > > > + type = iommu_get_def_domain_type(dev); > > > + } else if (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; > > > + goto out; > > > + } > > > > > > I also added the untrusted device check in > > > iommu_get_mandatory_def_domain_type(), so that we don't need to care > > > about this in multiple places. > > > > I see, but isn't this also setting the default domain type in the case that > > it gets back a type of 0? I think it would be nice to avoid having both > > iommu_get_mandatory_def_domain_type() and iommu_get_def_domain_type() of we > > can, as it's really not clear which one to use when and what is meant by > > "mandatory" imo. > > Yes, agreed. I will remove iommu_get_mandatory_def_domain_type() and > keep iommu_get_def_domain_type() as the only helper in the next version. Great, thanks! Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v9 1/4] iommu: Move def_domain type check for untrusted device into core
Hi Will, On 2020/11/23 21:03, Will Deacon wrote: Hi Baolu, On Mon, Nov 23, 2020 at 08:55:17PM +0800, Lu Baolu wrote: On 2020/11/23 20:04, Will Deacon wrote: On Sat, Nov 21, 2020 at 09:56:17PM +0800, Lu Baolu wrote: @@ -1645,13 +1655,10 @@ struct __group_domain_type { static int probe_get_default_domain_type(struct device *dev, void *data) { - const struct iommu_ops *ops = dev->bus->iommu_ops; struct __group_domain_type *gtype = data; unsigned int type = 0; - if (ops->def_domain_type) - type = ops->def_domain_type(dev); - + type = iommu_get_mandatory_def_domain_type(dev); afaict, this code is only called from probe_alloc_default_domain(), which has: /* Ask for default domain requirements of all devices in the group */ __iommu_group_for_each_dev(group, >ype, probe_get_default_domain_type); if (!gtype.type) gtype.type = iommu_def_domain_type; so is there actually a need to introduce the new iommu_get_mandatory_def_domain_type() function, given that a type of 0 always ends up resolving to the default domain type? Another consumer of this helper is in the next patch: + dev_def_dom = iommu_get_mandatory_def_domain_type(dev); + + /* Check if user requested domain is supported by the device or not */ + if (!type) { + /* +* If the user hasn't requested any specific type of domain and +* if the device supports both the domains, then default to the +* domain the device was booted with +*/ + type = iommu_get_def_domain_type(dev); + } else if (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; + goto out; + } I also added the untrusted device check in iommu_get_mandatory_def_domain_type(), so that we don't need to care about this in multiple places. I see, but isn't this also setting the default domain type in the case that it gets back a type of 0? I think it would be nice to avoid having both iommu_get_mandatory_def_domain_type() and iommu_get_def_domain_type() of we can, as it's really not clear which one to use when and what is meant by "mandatory" imo. Yes, agreed. I will remove iommu_get_mandatory_def_domain_type() and keep iommu_get_def_domain_type() as the only helper in the next version. Best regards, baolu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v9 1/4] iommu: Move def_domain type check for untrusted device into core
Hi Baolu, On Mon, Nov 23, 2020 at 08:55:17PM +0800, Lu Baolu wrote: > On 2020/11/23 20:04, Will Deacon wrote: > > On Sat, Nov 21, 2020 at 09:56:17PM +0800, Lu Baolu wrote: > > > @@ -1645,13 +1655,10 @@ struct __group_domain_type { > > > static int probe_get_default_domain_type(struct device *dev, void *data) > > > { > > > - const struct iommu_ops *ops = dev->bus->iommu_ops; > > > struct __group_domain_type *gtype = data; > > > unsigned int type = 0; > > > - if (ops->def_domain_type) > > > - type = ops->def_domain_type(dev); > > > - > > > + type = iommu_get_mandatory_def_domain_type(dev); > > > > afaict, this code is only called from probe_alloc_default_domain(), which > > has: > > > > /* Ask for default domain requirements of all devices in the group > > */ > > __iommu_group_for_each_dev(group, >ype, > > probe_get_default_domain_type); > > > > if (!gtype.type) > > gtype.type = iommu_def_domain_type; > > > > so is there actually a need to introduce the new > > iommu_get_mandatory_def_domain_type() function, given that a type of 0 > > always ends up resolving to the default domain type? > > Another consumer of this helper is in the next patch: > > + dev_def_dom = iommu_get_mandatory_def_domain_type(dev); > + > + /* Check if user requested domain is supported by the device or not */ > + if (!type) { > + /* > + * If the user hasn't requested any specific type of domain and > + * if the device supports both the domains, then default to the > + * domain the device was booted with > + */ > + type = iommu_get_def_domain_type(dev); > + } else if (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; > + goto out; > + } > > I also added the untrusted device check in > iommu_get_mandatory_def_domain_type(), so that we don't need to care > about this in multiple places. I see, but isn't this also setting the default domain type in the case that it gets back a type of 0? I think it would be nice to avoid having both iommu_get_mandatory_def_domain_type() and iommu_get_def_domain_type() of we can, as it's really not clear which one to use when and what is meant by "mandatory" imo. Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v9 1/4] iommu: Move def_domain type check for untrusted device into core
Hi Will, On 2020/11/23 20:04, Will Deacon wrote: On Sat, Nov 21, 2020 at 09:56:17PM +0800, Lu Baolu wrote: So that the vendor iommu drivers are no more required to provide the def_domain_type callback to always isolate the untrusted devices. Link: https://lore.kernel.org/linux-iommu/243ce89c33fe4b9da4c56ba35aceb...@huawei.com/ Cc: Shameerali Kolothum Thodi Signed-off-by: Lu Baolu --- drivers/iommu/intel/iommu.c | 7 --- drivers/iommu/iommu.c | 21 ++--- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c index af3abd285214..6711f78141a4 100644 --- a/drivers/iommu/intel/iommu.c +++ b/drivers/iommu/intel/iommu.c @@ -2916,13 +2916,6 @@ static int device_def_domain_type(struct device *dev) if (dev_is_pci(dev)) { struct pci_dev *pdev = to_pci_dev(dev); - /* -* Prevent any device marked as untrusted from getting -* placed into the statically identity mapping domain. -*/ - if (pdev->untrusted) - return IOMMU_DOMAIN_DMA; - if ((iommu_identity_mapping & IDENTMAP_AZALIA) && IS_AZALIA(pdev)) return IOMMU_DOMAIN_IDENTITY; diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index 88b0c9192d8c..3256784c0358 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -1457,13 +1457,23 @@ struct iommu_group *fsl_mc_device_group(struct device *dev) } EXPORT_SYMBOL_GPL(fsl_mc_device_group); -static int iommu_get_def_domain_type(struct device *dev) +/* Get the mandatary def_domain type for device. Otherwise, return 0. */ +static int iommu_get_mandatory_def_domain_type(struct device *dev) { const struct iommu_ops *ops = dev->bus->iommu_ops; - unsigned int type = 0; + + if (dev_is_pci(dev) && to_pci_dev(dev)->untrusted) + return IOMMU_DOMAIN_DMA; if (ops->def_domain_type) - type = ops->def_domain_type(dev); + return ops->def_domain_type(dev); + + return 0; +} + +static int iommu_get_def_domain_type(struct device *dev) +{ + int type = iommu_get_mandatory_def_domain_type(dev); return (type == 0) ? iommu_def_domain_type : type; } @@ -1645,13 +1655,10 @@ struct __group_domain_type { static int probe_get_default_domain_type(struct device *dev, void *data) { - const struct iommu_ops *ops = dev->bus->iommu_ops; struct __group_domain_type *gtype = data; unsigned int type = 0; - if (ops->def_domain_type) - type = ops->def_domain_type(dev); - + type = iommu_get_mandatory_def_domain_type(dev); afaict, this code is only called from probe_alloc_default_domain(), which has: /* Ask for default domain requirements of all devices in the group */ __iommu_group_for_each_dev(group, >ype, probe_get_default_domain_type); if (!gtype.type) gtype.type = iommu_def_domain_type; so is there actually a need to introduce the new iommu_get_mandatory_def_domain_type() function, given that a type of 0 always ends up resolving to the default domain type? Another consumer of this helper is in the next patch: + dev_def_dom = iommu_get_mandatory_def_domain_type(dev); + + /* Check if user requested domain is supported by the device or not */ + if (!type) { + /* +* If the user hasn't requested any specific type of domain and +* if the device supports both the domains, then default to the +* domain the device was booted with +*/ + type = iommu_get_def_domain_type(dev); + } else if (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; + goto out; + } I also added the untrusted device check in iommu_get_mandatory_def_domain_type(), so that we don't need to care about this in multiple places. Best regards, baolu ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v9 1/4] iommu: Move def_domain type check for untrusted device into core
On Sat, Nov 21, 2020 at 09:56:17PM +0800, Lu Baolu wrote: > So that the vendor iommu drivers are no more required to provide the > def_domain_type callback to always isolate the untrusted devices. > > Link: > https://lore.kernel.org/linux-iommu/243ce89c33fe4b9da4c56ba35aceb...@huawei.com/ > Cc: Shameerali Kolothum Thodi > Signed-off-by: Lu Baolu > --- > drivers/iommu/intel/iommu.c | 7 --- > drivers/iommu/iommu.c | 21 ++--- > 2 files changed, 14 insertions(+), 14 deletions(-) > > diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c > index af3abd285214..6711f78141a4 100644 > --- a/drivers/iommu/intel/iommu.c > +++ b/drivers/iommu/intel/iommu.c > @@ -2916,13 +2916,6 @@ static int device_def_domain_type(struct device *dev) > if (dev_is_pci(dev)) { > struct pci_dev *pdev = to_pci_dev(dev); > > - /* > - * Prevent any device marked as untrusted from getting > - * placed into the statically identity mapping domain. > - */ > - if (pdev->untrusted) > - return IOMMU_DOMAIN_DMA; > - > if ((iommu_identity_mapping & IDENTMAP_AZALIA) && > IS_AZALIA(pdev)) > return IOMMU_DOMAIN_IDENTITY; > > diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c > index 88b0c9192d8c..3256784c0358 100644 > --- a/drivers/iommu/iommu.c > +++ b/drivers/iommu/iommu.c > @@ -1457,13 +1457,23 @@ struct iommu_group *fsl_mc_device_group(struct device > *dev) > } > EXPORT_SYMBOL_GPL(fsl_mc_device_group); > > -static int iommu_get_def_domain_type(struct device *dev) > +/* Get the mandatary def_domain type for device. Otherwise, return 0. */ > +static int iommu_get_mandatory_def_domain_type(struct device *dev) > { > const struct iommu_ops *ops = dev->bus->iommu_ops; > - unsigned int type = 0; > + > + if (dev_is_pci(dev) && to_pci_dev(dev)->untrusted) > + return IOMMU_DOMAIN_DMA; > > if (ops->def_domain_type) > - type = ops->def_domain_type(dev); > + return ops->def_domain_type(dev); > + > + return 0; > +} > + > +static int iommu_get_def_domain_type(struct device *dev) > +{ > + int type = iommu_get_mandatory_def_domain_type(dev); > > return (type == 0) ? iommu_def_domain_type : type; > } > @@ -1645,13 +1655,10 @@ struct __group_domain_type { > > static int probe_get_default_domain_type(struct device *dev, void *data) > { > - const struct iommu_ops *ops = dev->bus->iommu_ops; > struct __group_domain_type *gtype = data; > unsigned int type = 0; > > - if (ops->def_domain_type) > - type = ops->def_domain_type(dev); > - > + type = iommu_get_mandatory_def_domain_type(dev); afaict, this code is only called from probe_alloc_default_domain(), which has: /* Ask for default domain requirements of all devices in the group */ __iommu_group_for_each_dev(group, >ype, probe_get_default_domain_type); if (!gtype.type) gtype.type = iommu_def_domain_type; so is there actually a need to introduce the new iommu_get_mandatory_def_domain_type() function, given that a type of 0 always ends up resolving to the default domain type? Will ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH v9 1/4] iommu: Move def_domain type check for untrusted device into core
So that the vendor iommu drivers are no more required to provide the def_domain_type callback to always isolate the untrusted devices. Link: https://lore.kernel.org/linux-iommu/243ce89c33fe4b9da4c56ba35aceb...@huawei.com/ Cc: Shameerali Kolothum Thodi Signed-off-by: Lu Baolu --- drivers/iommu/intel/iommu.c | 7 --- drivers/iommu/iommu.c | 21 ++--- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c index af3abd285214..6711f78141a4 100644 --- a/drivers/iommu/intel/iommu.c +++ b/drivers/iommu/intel/iommu.c @@ -2916,13 +2916,6 @@ static int device_def_domain_type(struct device *dev) if (dev_is_pci(dev)) { struct pci_dev *pdev = to_pci_dev(dev); - /* -* Prevent any device marked as untrusted from getting -* placed into the statically identity mapping domain. -*/ - if (pdev->untrusted) - return IOMMU_DOMAIN_DMA; - if ((iommu_identity_mapping & IDENTMAP_AZALIA) && IS_AZALIA(pdev)) return IOMMU_DOMAIN_IDENTITY; diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c index 88b0c9192d8c..3256784c0358 100644 --- a/drivers/iommu/iommu.c +++ b/drivers/iommu/iommu.c @@ -1457,13 +1457,23 @@ struct iommu_group *fsl_mc_device_group(struct device *dev) } EXPORT_SYMBOL_GPL(fsl_mc_device_group); -static int iommu_get_def_domain_type(struct device *dev) +/* Get the mandatary def_domain type for device. Otherwise, return 0. */ +static int iommu_get_mandatory_def_domain_type(struct device *dev) { const struct iommu_ops *ops = dev->bus->iommu_ops; - unsigned int type = 0; + + if (dev_is_pci(dev) && to_pci_dev(dev)->untrusted) + return IOMMU_DOMAIN_DMA; if (ops->def_domain_type) - type = ops->def_domain_type(dev); + return ops->def_domain_type(dev); + + return 0; +} + +static int iommu_get_def_domain_type(struct device *dev) +{ + int type = iommu_get_mandatory_def_domain_type(dev); return (type == 0) ? iommu_def_domain_type : type; } @@ -1645,13 +1655,10 @@ struct __group_domain_type { static int probe_get_default_domain_type(struct device *dev, void *data) { - const struct iommu_ops *ops = dev->bus->iommu_ops; struct __group_domain_type *gtype = data; unsigned int type = 0; - if (ops->def_domain_type) - type = ops->def_domain_type(dev); - + type = iommu_get_mandatory_def_domain_type(dev); if (type) { if (gtype->type && gtype->type != type) { dev_warn(dev, "Device needs domain type %s, but device %s in the same iommu group requires type %s - using default\n", -- 2.25.1 ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu