Re: [PATCH] iommu/arm-smmu: Fix stream-match conflict with IOMMU_DOMAIN_DMA

[Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>]

On 01/04/16 17:19, Will Deacon wrote:
> Commit cbf8277ef456 ("iommu/arm-smmu: Treat IOMMU_DOMAIN_DMA as bypass
> for now") ignores requests to attach a device to the default domain
> since, without IOMMU-basked DMA ops available everywhere, the default
> domain will just lead to unexpected transaction faults being reported.
>
> Unfortunately, the way this was implemented on SMMUv2 causes a
> regression with VFIO PCI device passthrough under KVM on AMD Seattle.
> On this system, the host controller device is associated with both a
> pci_dev *and* a platform_device, and can therefore end up with duplicate
> SMR entries, resulting in a stream-match conflict at runtime.
>
> This patch amends the original fix so that attaching to IOMMU_DOMAIN_DMA
> is rejected even before configuring the SMRs. This restores the old
> behaviour for now, but we'll need to look at handing host controllers
> specially when we come to supporting the default domain fully.

FWIW this is already solved with the generic bindings, as in most cases 
the host controller won't need an "iommus" property - for those that do 
actually have their own IDs for the platform device side, any aliasing 
with the PCI-derived IDs from "iommu-map" is magically taken care of in 
the group allocation code. That cross-bus-grouping should also apply to 
legacy masters too, since they look the same by the time we get to that 
point, so there should be no more stream match conflicts either way. 
I'll be working on a v2 of that series this week, so I'll rebase on top 
of this fix.

Robin.

> Reported-by: Eric Auger <eric.auger-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Signed-off-by: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
> ---
>   drivers/iommu/arm-smmu.c | 14 ++++++++------
>   1 file changed, 8 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index e933679a3266..2f186d22477f 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -1098,18 +1098,20 @@ static int arm_smmu_domain_add_master(struct arm_smmu_domain *smmu_domain,
>   	struct arm_smmu_device *smmu = smmu_domain->smmu;
>   	void __iomem *gr0_base = ARM_SMMU_GR0(smmu);
>
> -	/* Devices in an IOMMU group may already be configured */
> -	ret = arm_smmu_master_configure_smrs(smmu, cfg);
> -	if (ret)
> -		return ret == -EEXIST ? 0 : ret;
> -
>   	/*
>   	 * FIXME: This won't be needed once we have IOMMU-backed DMA ops
> -	 * for all devices behind the SMMU.
> +	 * for all devices behind the SMMU. Note that we need to take
> +	 * care configuring SMRs for devices both a platform_device and
> +	 * and a PCI device (i.e. a PCI host controller)
>   	 */
>   	if (smmu_domain->domain.type == IOMMU_DOMAIN_DMA)
>   		return 0;
>
> +	/* Devices in an IOMMU group may already be configured */
> +	ret = arm_smmu_master_configure_smrs(smmu, cfg);
> +	if (ret)
> +		return ret == -EEXIST ? 0 : ret;
> +
>   	for (i = 0; i < cfg->num_streamids; ++i) {
>   		u32 idx, s2cr;
>
>