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

[Shameerali Kolothum Thodi]

> -----Original Message-----
> From: Robin Murphy [mailto:robin.murphy at arm.com]
> Sent: 29 October 2020 15:54
> To: Shameerali Kolothum Thodi <shameerali.kolothum.thodi at huawei.com>;
> linux-arm-kernel at lists.infradead.org; iommu at lists.linux-foundation.org
> Cc: Linuxarm <linuxarm at huawei.com>; will at kernel.org; joro at 8bytes.org;
> jean-philippe at linaro.org; ashok.raj at intel.com; John Garry
> <john.garry at huawei.com>; Song Bao Hua (Barry Song)
> <song.bao.hua at hisilicon.com>; Jonathan Cameron
> <jonathan.cameron at 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 at 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 */
> >   };
> >
> >