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

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

[Will Deacon]

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.

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;
 
-- 
2.1.4

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

[Will Deacon]

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.

Reported-by: Eric Auger <eric.auger@linaro.org>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
 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;
 
-- 
2.1.4

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

[Eric Auger]

Hi Will,
On 04/01/2016 06:19 PM, 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.

This works fine for me, before and after PCIe assignment. However before
giving my T-b I would like to investigate another regression I observe
wrt SRIOV assignment. Assigning the 1st VF works fine. However assigning
a second VF does not work anymore (it used to in the past). I get
-ENOSPC from arm_smmu_master_configure_smrs

Best Regards

Eric
> 
> 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;
>  
> 

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

[Eric Auger]

Hi Will,
On 04/01/2016 06:19 PM, 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.

This works fine for me, before and after PCIe assignment. However before
giving my T-b I would like to investigate another regression I observe
wrt SRIOV assignment. Assigning the 1st VF works fine. However assigning
a second VF does not work anymore (it used to in the past). I get
-ENOSPC from arm_smmu_master_configure_smrs

Best Regards

Eric
> 
> Reported-by: Eric Auger <eric.auger@linaro.org>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
>  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;
>  
> 

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

[Robin Murphy]

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;
>
>

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

[Robin Murphy]

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@linaro.org>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
>   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;
>
>

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

[Eric Auger]

Hi Robin,
On 04/05/2016 12:48 PM, Robin Murphy wrote:
> 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.

Thanks for the notice.

Besides the fault I was experiencing before that fix I also face a
regression with respect to SRIOV 2d VF assignment.  It appears it is due
to a shortage of context banks and not a shortage of SMRs as I thought
at the beginning. The -ENOSPC is returned by
arm_smmu_init_domain_context (__arm_smmu_alloc_bitmap).
arm_smmu_init_domain_context is called even if handle a default iommu
domain and if my understanding is correct we shouldn't consume any
context bank in that case.

Best Regards

Eric
> 
> 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;
>>
>>
> 

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

[Eric Auger]

Hi Robin,
On 04/05/2016 12:48 PM, Robin Murphy wrote:
> 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.

Thanks for the notice.

Besides the fault I was experiencing before that fix I also face a
regression with respect to SRIOV 2d VF assignment.  It appears it is due
to a shortage of context banks and not a shortage of SMRs as I thought
at the beginning. The -ENOSPC is returned by
arm_smmu_init_domain_context (__arm_smmu_alloc_bitmap).
arm_smmu_init_domain_context is called even if handle a default iommu
domain and if my understanding is correct we shouldn't consume any
context bank in that case.

Best Regards

Eric
> 
> Robin.
> 
>> Reported-by: Eric Auger <eric.auger@linaro.org>
>> Signed-off-by: Will Deacon <will.deacon@arm.com>
>> ---
>>   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;
>>
>>
> 

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

[Will Deacon]

Hi Eric,

On Mon, Apr 04, 2016 at 11:40:29AM +0200, Eric Auger wrote:
> On 04/01/2016 06:19 PM, 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.
> 
> This works fine for me, before and after PCIe assignment. However before
> giving my T-b I would like to investigate another regression I observe
> wrt SRIOV assignment. Assigning the 1st VF works fine. However assigning
> a second VF does not work anymore (it used to in the past). I get
> -ENOSPC from arm_smmu_master_configure_smrs

Did you get anywhere debugging this?

Will

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

[Will Deacon]

Hi Eric,

On Mon, Apr 04, 2016 at 11:40:29AM +0200, Eric Auger wrote:
> On 04/01/2016 06:19 PM, 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.
> 
> This works fine for me, before and after PCIe assignment. However before
> giving my T-b I would like to investigate another regression I observe
> wrt SRIOV assignment. Assigning the 1st VF works fine. However assigning
> a second VF does not work anymore (it used to in the past). I get
> -ENOSPC from arm_smmu_master_configure_smrs

Did you get anywhere debugging this?

Will

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

[Eric Auger]

Hi Will,
On 04/07/2016 04:46 PM, Will Deacon wrote:
> Hi Eric,
> 
> On Mon, Apr 04, 2016 at 11:40:29AM +0200, Eric Auger wrote:
>> On 04/01/2016 06:19 PM, 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.
>>
>> This works fine for me, before and after PCIe assignment. However before
>> giving my T-b I would like to investigate another regression I observe
>> wrt SRIOV assignment. Assigning the 1st VF works fine. However assigning
>> a second VF does not work anymore (it used to in the past). I get
>> -ENOSPC from arm_smmu_master_configure_smrs
> 
> Did you get anywhere debugging this?
Yes: http://www.spinics.net/lists/arm-kernel/msg495024.html

Eric
> 
> Will
> 

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

[Eric Auger]

Hi Will,
On 04/07/2016 04:46 PM, Will Deacon wrote:
> Hi Eric,
> 
> On Mon, Apr 04, 2016 at 11:40:29AM +0200, Eric Auger wrote:
>> On 04/01/2016 06:19 PM, 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.
>>
>> This works fine for me, before and after PCIe assignment. However before
>> giving my T-b I would like to investigate another regression I observe
>> wrt SRIOV assignment. Assigning the 1st VF works fine. However assigning
>> a second VF does not work anymore (it used to in the past). I get
>> -ENOSPC from arm_smmu_master_configure_smrs
> 
> Did you get anywhere debugging this?
Yes: http://www.spinics.net/lists/arm-kernel/msg495024.html

Eric
> 
> Will
> 

[PATCH] iommu/arm-smmu: Don't allocate resources for bypass domains

[Robin Murphy]

Until we get fully plumbed into of_iommu_configure, our default
IOMMU_DOMAIN_DMA domains just bypass translation. Since we achieve that
by leaving the stream table entries set to bypass instead of pointing at
a translation context, the context bank we allocate for the domain is
completely wasted. Context banks are typically a rather limited
resource, so don't hog ones we don't need.

Reported-by: Eric Auger <eric.auger-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
---
 drivers/iommu/arm-smmu.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 5158feb..7c39ac4 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -826,6 +826,12 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
 	if (smmu_domain->smmu)
 		goto out_unlock;
 
+	/* We're bypassing these SIDs, so don't allocate an actual context */
+	if (domain->type == IOMMU_DOMAIN_DMA) {
+		smmu_domain->smmu = smmu;
+		goto out_unlock;
+	}
+
 	/*
 	 * Mapping the requested stage onto what we support is surprisingly
 	 * complicated, mainly because the spec allows S1+S2 SMMUs without
@@ -948,7 +954,7 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
 	void __iomem *cb_base;
 	int irq;
 
-	if (!smmu)
+	if (!smmu || domain->type == IOMMU_DOMAIN_DMA)
 		return;
 
 	/*
-- 
2.7.3.dirty

[PATCH] iommu/arm-smmu: Don't allocate resources for bypass domains

[Robin Murphy]

Until we get fully plumbed into of_iommu_configure, our default
IOMMU_DOMAIN_DMA domains just bypass translation. Since we achieve that
by leaving the stream table entries set to bypass instead of pointing at
a translation context, the context bank we allocate for the domain is
completely wasted. Context banks are typically a rather limited
resource, so don't hog ones we don't need.

Reported-by: Eric Auger <eric.auger@linaro.org>
Signed-off-by: Robin Murphy <robin.murphy@arm.com>
---
 drivers/iommu/arm-smmu.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
index 5158feb..7c39ac4 100644
--- a/drivers/iommu/arm-smmu.c
+++ b/drivers/iommu/arm-smmu.c
@@ -826,6 +826,12 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
 	if (smmu_domain->smmu)
 		goto out_unlock;
 
+	/* We're bypassing these SIDs, so don't allocate an actual context */
+	if (domain->type == IOMMU_DOMAIN_DMA) {
+		smmu_domain->smmu = smmu;
+		goto out_unlock;
+	}
+
 	/*
 	 * Mapping the requested stage onto what we support is surprisingly
 	 * complicated, mainly because the spec allows S1+S2 SMMUs without
@@ -948,7 +954,7 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
 	void __iomem *cb_base;
 	int irq;
 
-	if (!smmu)
+	if (!smmu || domain->type == IOMMU_DOMAIN_DMA)
 		return;
 
 	/*
-- 
2.7.3.dirty

Re: [PATCH] iommu/arm-smmu: Don't allocate resources for bypass domains

[Will Deacon]

On Wed, Apr 13, 2016 at 03:46:57PM +0100, Robin Murphy wrote:
> Until we get fully plumbed into of_iommu_configure, our default
> IOMMU_DOMAIN_DMA domains just bypass translation. Since we achieve that
> by leaving the stream table entries set to bypass instead of pointing at
> a translation context, the context bank we allocate for the domain is
> completely wasted. Context banks are typically a rather limited
> resource, so don't hog ones we don't need.

Eric, does this resolve your remaining passthrough regressions when applied
on top of my patch?

Will

> Reported-by: Eric Auger <eric.auger-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> ---
>  drivers/iommu/arm-smmu.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 5158feb..7c39ac4 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -826,6 +826,12 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
>  	if (smmu_domain->smmu)
>  		goto out_unlock;
>  
> +	/* We're bypassing these SIDs, so don't allocate an actual context */
> +	if (domain->type == IOMMU_DOMAIN_DMA) {
> +		smmu_domain->smmu = smmu;
> +		goto out_unlock;
> +	}
> +
>  	/*
>  	 * Mapping the requested stage onto what we support is surprisingly
>  	 * complicated, mainly because the spec allows S1+S2 SMMUs without
> @@ -948,7 +954,7 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
>  	void __iomem *cb_base;
>  	int irq;
>  
> -	if (!smmu)
> +	if (!smmu || domain->type == IOMMU_DOMAIN_DMA)
>  		return;
>  
>  	/*
> -- 
> 2.7.3.dirty
> 

[PATCH] iommu/arm-smmu: Don't allocate resources for bypass domains

[Will Deacon]

On Wed, Apr 13, 2016 at 03:46:57PM +0100, Robin Murphy wrote:
> Until we get fully plumbed into of_iommu_configure, our default
> IOMMU_DOMAIN_DMA domains just bypass translation. Since we achieve that
> by leaving the stream table entries set to bypass instead of pointing at
> a translation context, the context bank we allocate for the domain is
> completely wasted. Context banks are typically a rather limited
> resource, so don't hog ones we don't need.

Eric, does this resolve your remaining passthrough regressions when applied
on top of my patch?

Will

> Reported-by: Eric Auger <eric.auger@linaro.org>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  drivers/iommu/arm-smmu.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 5158feb..7c39ac4 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -826,6 +826,12 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
>  	if (smmu_domain->smmu)
>  		goto out_unlock;
>  
> +	/* We're bypassing these SIDs, so don't allocate an actual context */
> +	if (domain->type == IOMMU_DOMAIN_DMA) {
> +		smmu_domain->smmu = smmu;
> +		goto out_unlock;
> +	}
> +
>  	/*
>  	 * Mapping the requested stage onto what we support is surprisingly
>  	 * complicated, mainly because the spec allows S1+S2 SMMUs without
> @@ -948,7 +954,7 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
>  	void __iomem *cb_base;
>  	int irq;
>  
> -	if (!smmu)
> +	if (!smmu || domain->type == IOMMU_DOMAIN_DMA)
>  		return;
>  
>  	/*
> -- 
> 2.7.3.dirty
> 

Re: [PATCH] iommu/arm-smmu: Don't allocate resources for bypass domains

[Eric Auger]

Hi Robin,
On 04/13/2016 04:46 PM, Robin Murphy wrote:
> Until we get fully plumbed into of_iommu_configure, our default
> IOMMU_DOMAIN_DMA domains just bypass translation. Since we achieve that
> by leaving the stream table entries set to bypass instead of pointing at
> a translation context, the context bank we allocate for the domain is
> completely wasted. Context banks are typically a rather limited
> resource, so don't hog ones we don't need.
> 
Thanks for this fix. I recovered the ability to assign more than VFs on
Overdrive HW.

Tested-by: Eric Auger <eric.auger-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Best Regards

Eric
> Reported-by: Eric Auger <eric.auger-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
> ---
>  drivers/iommu/arm-smmu.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 5158feb..7c39ac4 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -826,6 +826,12 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
>  	if (smmu_domain->smmu)
>  		goto out_unlock;
>  
> +	/* We're bypassing these SIDs, so don't allocate an actual context */
> +	if (domain->type == IOMMU_DOMAIN_DMA) {
> +		smmu_domain->smmu = smmu;
> +		goto out_unlock;
> +	}
> +
>  	/*
>  	 * Mapping the requested stage onto what we support is surprisingly
>  	 * complicated, mainly because the spec allows S1+S2 SMMUs without
> @@ -948,7 +954,7 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
>  	void __iomem *cb_base;
>  	int irq;
>  
> -	if (!smmu)
> +	if (!smmu || domain->type == IOMMU_DOMAIN_DMA)
>  		return;
>  
>  	/*
> 

[PATCH] iommu/arm-smmu: Don't allocate resources for bypass domains

[Eric Auger]

Hi Robin,
On 04/13/2016 04:46 PM, Robin Murphy wrote:
> Until we get fully plumbed into of_iommu_configure, our default
> IOMMU_DOMAIN_DMA domains just bypass translation. Since we achieve that
> by leaving the stream table entries set to bypass instead of pointing at
> a translation context, the context bank we allocate for the domain is
> completely wasted. Context banks are typically a rather limited
> resource, so don't hog ones we don't need.
> 
Thanks for this fix. I recovered the ability to assign more than VFs on
Overdrive HW.

Tested-by: Eric Auger <eric.auger@linaro.org>

Best Regards

Eric
> Reported-by: Eric Auger <eric.auger@linaro.org>
> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
> ---
>  drivers/iommu/arm-smmu.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 5158feb..7c39ac4 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -826,6 +826,12 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
>  	if (smmu_domain->smmu)
>  		goto out_unlock;
>  
> +	/* We're bypassing these SIDs, so don't allocate an actual context */
> +	if (domain->type == IOMMU_DOMAIN_DMA) {
> +		smmu_domain->smmu = smmu;
> +		goto out_unlock;
> +	}
> +
>  	/*
>  	 * Mapping the requested stage onto what we support is surprisingly
>  	 * complicated, mainly because the spec allows S1+S2 SMMUs without
> @@ -948,7 +954,7 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
>  	void __iomem *cb_base;
>  	int irq;
>  
> -	if (!smmu)
> +	if (!smmu || domain->type == IOMMU_DOMAIN_DMA)
>  		return;
>  
>  	/*
> 

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

[Eric Auger]

Hi Will,
On 04/01/2016 06:19 PM, 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.
> 
Tested-by: Eric Auger <eric.auger-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>

Best Regards

Eric
> 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;
>  
> 

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

[Eric Auger]

Hi Will,
On 04/01/2016 06:19 PM, 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.
> 
Tested-by: Eric Auger <eric.auger@linaro.org>

Best Regards

Eric
> Reported-by: Eric Auger <eric.auger@linaro.org>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
> ---
>  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;
>  
> 

Re: [PATCH] iommu/arm-smmu: Don't allocate resources for bypass domains

[Eric Auger]

Hi Will,
On 04/15/2016 03:05 PM, Will Deacon wrote:
> On Wed, Apr 13, 2016 at 03:46:57PM +0100, Robin Murphy wrote:
>> Until we get fully plumbed into of_iommu_configure, our default
>> IOMMU_DOMAIN_DMA domains just bypass translation. Since we achieve that
>> by leaving the stream table entries set to bypass instead of pointing at
>> a translation context, the context bank we allocate for the domain is
>> completely wasted. Context banks are typically a rather limited
>> resource, so don't hog ones we don't need.
> 
> Eric, does this resolve your remaining passthrough regressions when applied
> on top of my patch?

Yes it does. Thank you for the fixes.

Best Regards

Eric
> 
> Will
> 
>> Reported-by: Eric Auger <eric.auger-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> Signed-off-by: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>
>> ---
>>  drivers/iommu/arm-smmu.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>> index 5158feb..7c39ac4 100644
>> --- a/drivers/iommu/arm-smmu.c
>> +++ b/drivers/iommu/arm-smmu.c
>> @@ -826,6 +826,12 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
>>  	if (smmu_domain->smmu)
>>  		goto out_unlock;
>>  
>> +	/* We're bypassing these SIDs, so don't allocate an actual context */
>> +	if (domain->type == IOMMU_DOMAIN_DMA) {
>> +		smmu_domain->smmu = smmu;
>> +		goto out_unlock;
>> +	}
>> +
>>  	/*
>>  	 * Mapping the requested stage onto what we support is surprisingly
>>  	 * complicated, mainly because the spec allows S1+S2 SMMUs without
>> @@ -948,7 +954,7 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
>>  	void __iomem *cb_base;
>>  	int irq;
>>  
>> -	if (!smmu)
>> +	if (!smmu || domain->type == IOMMU_DOMAIN_DMA)
>>  		return;
>>  
>>  	/*
>> -- 
>> 2.7.3.dirty
>>

[PATCH] iommu/arm-smmu: Don't allocate resources for bypass domains

[Eric Auger]

Hi Will,
On 04/15/2016 03:05 PM, Will Deacon wrote:
> On Wed, Apr 13, 2016 at 03:46:57PM +0100, Robin Murphy wrote:
>> Until we get fully plumbed into of_iommu_configure, our default
>> IOMMU_DOMAIN_DMA domains just bypass translation. Since we achieve that
>> by leaving the stream table entries set to bypass instead of pointing at
>> a translation context, the context bank we allocate for the domain is
>> completely wasted. Context banks are typically a rather limited
>> resource, so don't hog ones we don't need.
> 
> Eric, does this resolve your remaining passthrough regressions when applied
> on top of my patch?

Yes it does. Thank you for the fixes.

Best Regards

Eric
> 
> Will
> 
>> Reported-by: Eric Auger <eric.auger@linaro.org>
>> Signed-off-by: Robin Murphy <robin.murphy@arm.com>
>> ---
>>  drivers/iommu/arm-smmu.c | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>> index 5158feb..7c39ac4 100644
>> --- a/drivers/iommu/arm-smmu.c
>> +++ b/drivers/iommu/arm-smmu.c
>> @@ -826,6 +826,12 @@ static int arm_smmu_init_domain_context(struct iommu_domain *domain,
>>  	if (smmu_domain->smmu)
>>  		goto out_unlock;
>>  
>> +	/* We're bypassing these SIDs, so don't allocate an actual context */
>> +	if (domain->type == IOMMU_DOMAIN_DMA) {
>> +		smmu_domain->smmu = smmu;
>> +		goto out_unlock;
>> +	}
>> +
>>  	/*
>>  	 * Mapping the requested stage onto what we support is surprisingly
>>  	 * complicated, mainly because the spec allows S1+S2 SMMUs without
>> @@ -948,7 +954,7 @@ static void arm_smmu_destroy_domain_context(struct iommu_domain *domain)
>>  	void __iomem *cb_base;
>>  	int irq;
>>  
>> -	if (!smmu)
>> +	if (!smmu || domain->type == IOMMU_DOMAIN_DMA)
>>  		return;
>>  
>>  	/*
>> -- 
>> 2.7.3.dirty
>>

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

[Shi, Yang]

On 4/15/2016 8:09 AM, Eric Auger wrote:
> Hi Will,
> On 04/01/2016 06:19 PM, 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.
>>
> Tested-by: Eric Auger <eric.auger@linaro.org>

Tested on my LS2085A board too.

Tested-by: Yang Shi <yang.shi@linaro.org>

Regards,
Yang

>
> Best Regards
>
> Eric
>> Reported-by: Eric Auger <eric.auger@linaro.org>
>> Signed-off-by: Will Deacon <will.deacon@arm.com>
>> ---
>>   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;
>>
>>
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

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

[Shi, Yang]

On 4/15/2016 8:09 AM, Eric Auger wrote:
> Hi Will,
> On 04/01/2016 06:19 PM, 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.
>>
> Tested-by: Eric Auger <eric.auger@linaro.org>

Tested on my LS2085A board too.

Tested-by: Yang Shi <yang.shi@linaro.org>

Regards,
Yang

>
> Best Regards
>
> Eric
>> Reported-by: Eric Auger <eric.auger@linaro.org>
>> Signed-off-by: Will Deacon <will.deacon@arm.com>
>> ---
>>   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;
>>
>>
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>