| Message ID | 20201029154114.20364-1-shameerali.kolothum.thodi@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | iommu/arm-smmu-v3: Add def_domain_type callback | expand |
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 */ > }; > >
> -----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 */ > > }; > > > >
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 */ };
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(+)