[PATCH] iommu/arm-smmu-v3: Add def_domain_type callback

[PATCH] iommu/arm-smmu-v3: Add def_domain_type callback

[Shameer Kolothum]

With the introduction of def_domain_type in iommu_ops, vendor
drivers can now inform the iommu generic layer about any specific
default domain requirement for a device. Any pci dev marked as
untrusted is now prevented from having an IDENTITY mapping
domain. 

The callback is also required when the support for dynamically
changing the default domain of a group is available.

Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
---
 -Only devices downstream from externally exposed PCIe hierarchies
  (such as Thunderbolt outside the platform) are currently marked
  as "untrusted". Not aware of any ARM64 platforms that may use
  this type of device.  

  Nevertheless, the main motivation for this patch is to have the
  flexibility of changing the iommu default domain for a group based
  on the series[1] "iommu: Add support to change default domain of an
  iommu group" and that mandates vendor iommu driver to provide this
  callback.

 -This is tested along with [1] and was able to change the default
  domain of an iommu group on an HiSilicon D06 hardware.
 
1. https://lore.kernel.org/linux-iommu/20200925190620.18732-1-ashok.raj@intel.com/
---
 drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 26 +++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
index e634bbe60573..d5dbcee995db 100644
--- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
+++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
@@ -2567,6 +2567,31 @@ static int arm_smmu_dev_disable_feature(struct device *dev,
 	}
 }
 
+/*
+ * Return the required default domain type for a specific device.
+ *
+ * @dev: the device in query
+ *
+ * Returns:
+ *  - IOMMU_DOMAIN_DMA: device requires a dynamic mapping domain
+ *  - 0: both identity and dynamic domains work for this device
+ */
+static int arm_smmu_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 Identity mapping domain.
+		 */
+		if (pdev->untrusted)
+			return IOMMU_DOMAIN_DMA;
+	}
+
+	return 0;
+}
+
 static struct iommu_ops arm_smmu_ops = {
 	.capable		= arm_smmu_capable,
 	.domain_alloc		= arm_smmu_domain_alloc,
@@ -2589,6 +2614,7 @@ static struct iommu_ops arm_smmu_ops = {
 	.dev_feat_enabled	= arm_smmu_dev_feature_enabled,
 	.dev_enable_feat	= arm_smmu_dev_enable_feature,
 	.dev_disable_feat	= arm_smmu_dev_disable_feature,
+	.def_domain_type	= arm_smmu_def_domain_type,
 	.pgsize_bitmap		= -1UL, /* Restricted during device attach */
 };
 
-- 
2.17.1

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

Re: [PATCH] iommu/arm-smmu-v3: Add def_domain_type callback

[Robin Murphy]

On 2020-10-29 15:41, Shameer Kolothum wrote:
> With the introduction of def_domain_type in iommu_ops, vendor
> drivers can now inform the iommu generic layer about any specific
> default domain requirement for a device. Any pci dev marked as
> untrusted is now prevented from having an IDENTITY mapping
> domain.
> 
> The callback is also required when the support for dynamically
> changing the default domain of a group is available.

Yes, we want to allow the group type control to work for all drivers, 
ideally...

> Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> ---
>   -Only devices downstream from externally exposed PCIe hierarchies
>    (such as Thunderbolt outside the platform) are currently marked
>    as "untrusted". Not aware of any ARM64 platforms that may use
>    this type of device.
> 
>    Nevertheless, the main motivation for this patch is to have the
>    flexibility of changing the iommu default domain for a group based
>    on the series[1] "iommu: Add support to change default domain of an
>    iommu group" and that mandates vendor iommu driver to provide this
>    callback.
> 
>   -This is tested along with [1] and was able to change the default
>    domain of an iommu group on an HiSilicon D06 hardware.
>   
> 1. https://lore.kernel.org/linux-iommu/20200925190620.18732-1-ashok.raj@intel.com/
> ---
>   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 26 +++++++++++++++++++++
>   1 file changed, 26 insertions(+)
> 
> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index e634bbe60573..d5dbcee995db 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -2567,6 +2567,31 @@ static int arm_smmu_dev_disable_feature(struct device *dev,
>   	}
>   }
>   
> +/*
> + * Return the required default domain type for a specific device.
> + *
> + * @dev: the device in query
> + *
> + * Returns:
> + *  - IOMMU_DOMAIN_DMA: device requires a dynamic mapping domain
> + *  - 0: both identity and dynamic domains work for this device
> + */
> +static int arm_smmu_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 Identity mapping domain.
> +		 */
> +		if (pdev->untrusted)
> +			return IOMMU_DOMAIN_DMA;
> +	}

This should be somewhere in core code - it has nothing to do with SMMUv3.

> +
> +	return 0;

I don't strictly object to adding a stub callback for that bit, but why 
can't iommu_change_dev_def_domain() simply assume it from a NULL 
callback? That works for everyone, for no extra cost ;)

Robin.

> +}
> +
>   static struct iommu_ops arm_smmu_ops = {
>   	.capable		= arm_smmu_capable,
>   	.domain_alloc		= arm_smmu_domain_alloc,
> @@ -2589,6 +2614,7 @@ static struct iommu_ops arm_smmu_ops = {
>   	.dev_feat_enabled	= arm_smmu_dev_feature_enabled,
>   	.dev_enable_feat	= arm_smmu_dev_enable_feature,
>   	.dev_disable_feat	= arm_smmu_dev_disable_feature,
> +	.def_domain_type	= arm_smmu_def_domain_type,
>   	.pgsize_bitmap		= -1UL, /* Restricted during device attach */
>   };
>   
> 
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

RE: [PATCH] iommu/arm-smmu-v3: Add def_domain_type callback

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Robin Murphy [mailto:robin.murphy@arm.com]
> Sent: 29 October 2020 15:54
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi@huawei.com>;
> linux-arm-kernel@lists.infradead.org; iommu@lists.linux-foundation.org
> Cc: Linuxarm <linuxarm@huawei.com>; will@kernel.org; joro@8bytes.org;
> jean-philippe@linaro.org; ashok.raj@intel.com; John Garry
> <john.garry@huawei.com>; Song Bao Hua (Barry Song)
> <song.bao.hua@hisilicon.com>; Jonathan Cameron
> <jonathan.cameron@huawei.com>
> Subject: Re: [PATCH] iommu/arm-smmu-v3: Add def_domain_type callback
> 
> On 2020-10-29 15:41, Shameer Kolothum wrote:
> > With the introduction of def_domain_type in iommu_ops, vendor
> > drivers can now inform the iommu generic layer about any specific
> > default domain requirement for a device. Any pci dev marked as
> > untrusted is now prevented from having an IDENTITY mapping
> > domain.
> >
> > The callback is also required when the support for dynamically
> > changing the default domain of a group is available.
> 
> Yes, we want to allow the group type control to work for all drivers,
> ideally...
> 
> > Signed-off-by: Shameer Kolothum <shameerali.kolothum.thodi@huawei.com>
> > ---
> >   -Only devices downstream from externally exposed PCIe hierarchies
> >    (such as Thunderbolt outside the platform) are currently marked
> >    as "untrusted". Not aware of any ARM64 platforms that may use
> >    this type of device.
> >
> >    Nevertheless, the main motivation for this patch is to have the
> >    flexibility of changing the iommu default domain for a group based
> >    on the series[1] "iommu: Add support to change default domain of an
> >    iommu group" and that mandates vendor iommu driver to provide this
> >    callback.
> >
> >   -This is tested along with [1] and was able to change the default
> >    domain of an iommu group on an HiSilicon D06 hardware.
> >
> > 1.
> https://lore.kernel.org/linux-iommu/20200925190620.18732-1-ashok.raj@intel
> .com/
> > ---
> >   drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c | 26
> +++++++++++++++++++++
> >   1 file changed, 26 insertions(+)
> >
> > diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > index e634bbe60573..d5dbcee995db 100644
> > --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> > @@ -2567,6 +2567,31 @@ static int arm_smmu_dev_disable_feature(struct
> device *dev,
> >   	}
> >   }
> >
> > +/*
> > + * Return the required default domain type for a specific device.
> > + *
> > + * @dev: the device in query
> > + *
> > + * Returns:
> > + *  - IOMMU_DOMAIN_DMA: device requires a dynamic mapping domain
> > + *  - 0: both identity and dynamic domains work for this device
> > + */
> > +static int arm_smmu_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 Identity mapping domain.
> > +		 */
> > +		if (pdev->untrusted)
> > +			return IOMMU_DOMAIN_DMA;
> > +	}
> 
> This should be somewhere in core code - it has nothing to do with SMMUv3.

Agree.

> > +
> > +	return 0;
> 
> I don't strictly object to adding a stub callback for that bit, but why
> can't iommu_change_dev_def_domain() simply assume it from a NULL
> callback? That works for everyone, for no extra cost ;)

Right.

Hi Ashok,
Do you have any plan to respin your series and is it possible to include
the above suggestions into that? Please let me know.

Thanks,
Shameer
 
> Robin.
> 
> > +}
> > +
> >   static struct iommu_ops arm_smmu_ops = {
> >   	.capable		= arm_smmu_capable,
> >   	.domain_alloc		= arm_smmu_domain_alloc,
> > @@ -2589,6 +2614,7 @@ static struct iommu_ops arm_smmu_ops = {
> >   	.dev_feat_enabled	= arm_smmu_dev_feature_enabled,
> >   	.dev_enable_feat	= arm_smmu_dev_enable_feature,
> >   	.dev_disable_feat	= arm_smmu_dev_disable_feature,
> > +	.def_domain_type	= arm_smmu_def_domain_type,
> >   	.pgsize_bitmap		= -1UL, /* Restricted during device attach */
> >   };
> >
> >
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu