[PATCH] xen/arm: smmuv1: Revert associating the group pointer with the S2CR

[PATCH] xen/arm: smmuv1: Revert associating the group pointer with the S2CR

[Rahul Singh]

Revert the code that associates the group pointer with the S2CR as this
code causing an issue when the SMMU device has more than one master
device.

This code is merged with the commit  "xen/arm: smmuv1: Intelligent
SMR allocation”.

Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---
 xen/drivers/passthrough/arm/smmu.c | 44 +++---------------------------
 1 file changed, 4 insertions(+), 40 deletions(-)

diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index 24ac4f3a80..1a68c2ab3b 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -597,7 +597,6 @@ enum arm_smmu_arch_version {
 };
 
 struct arm_smmu_s2cr {
-	struct iommu_group		*group;
 	int				count;
 	enum arm_smmu_s2cr_type		type;
 	enum arm_smmu_s2cr_privcfg	privcfg;
@@ -1498,7 +1497,6 @@ static int arm_smmu_master_alloc_smes(struct device *dev)
 	struct arm_smmu_master_cfg *cfg = find_smmu_master_cfg(dev);
 	struct arm_smmu_device *smmu = cfg->smmu;
 	struct arm_smmu_smr *smrs = smmu->smrs;
-	struct iommu_group *group;
 	int i, idx, ret;
 
 	spin_lock(&smmu->stream_map_lock);
@@ -1523,19 +1521,9 @@ static int arm_smmu_master_alloc_smes(struct device *dev)
 		cfg->smendx[i] = (s16)idx;
 	}
 
-	group = iommu_group_get(dev);
-	if (!group)
-		group = ERR_PTR(-ENOMEM);
-	if (IS_ERR(group)) {
-		ret = PTR_ERR(group);
-		goto out_err;
-	}
-	iommu_group_put(group);
-
 	/* It worked! Now, poke the actual hardware */
 	for_each_cfg_sme(cfg, i, idx) {
 		arm_smmu_write_sme(smmu, idx);
-		smmu->s2crs[idx].group = group;
 	}
 
 	spin_unlock(&smmu->stream_map_lock);
@@ -1966,27 +1954,6 @@ static void __arm_smmu_release_pci_iommudata(void *data)
 	kfree(data);
 }
 
-static struct iommu_group *arm_smmu_device_group(struct
-						arm_smmu_master_cfg *cfg)
-{
-	struct arm_smmu_device *smmu = cfg->smmu;
-	struct iommu_group *group = NULL;
-	int i, idx;
-
-	for_each_cfg_sme(cfg, i, idx) {
-		if (group && smmu->s2crs[idx].group &&
-		    group != smmu->s2crs[idx].group)
-			return ERR_PTR(-EINVAL);
-
-		group = smmu->s2crs[idx].group;
-	}
-
-	if (group)
-		return group;
-
-	return NULL;
-}
-
 static int arm_smmu_add_device(struct device *dev)
 {
 	struct arm_smmu_device *smmu;
@@ -2027,13 +1994,10 @@ static int arm_smmu_add_device(struct device *dev)
 		cfg->smmu = smmu;
 	}
 
-	group = arm_smmu_device_group(cfg);
-	if (!group) {
-		group = iommu_group_alloc();
-		if (IS_ERR(group)) {
-			dev_err(dev, "Failed to allocate IOMMU group\n");
-			return PTR_ERR(group);
-		}
+	group = iommu_group_alloc();
+	if (IS_ERR(group)) {
+		dev_err(dev, "Failed to allocate IOMMU group\n");
+		return PTR_ERR(group);
 	}
 
 	iommu_group_set_iommudata(group, cfg, releasefn);
-- 
2.17.1

Re: [PATCH] xen/arm: smmuv1: Revert associating the group pointer with the S2CR

[Bertrand Marquis]

Hi Rahul,

> On 16 Apr 2021, at 12:25, Rahul Singh <rahul.singh@arm.com> wrote:
> 
> Revert the code that associates the group pointer with the S2CR as this
> code causing an issue when the SMMU device has more than one master
> device.
> 
> This code is merged with the commit  "xen/arm: smmuv1: Intelligent
> SMR allocation”.
> 
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Cheers,
Bertrand

> ---
> xen/drivers/passthrough/arm/smmu.c | 44 +++---------------------------
> 1 file changed, 4 insertions(+), 40 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
> index 24ac4f3a80..1a68c2ab3b 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -597,7 +597,6 @@ enum arm_smmu_arch_version {
> };
> 
> struct arm_smmu_s2cr {
> -	struct iommu_group		*group;
> 	int				count;
> 	enum arm_smmu_s2cr_type		type;
> 	enum arm_smmu_s2cr_privcfg	privcfg;
> @@ -1498,7 +1497,6 @@ static int arm_smmu_master_alloc_smes(struct device *dev)
> 	struct arm_smmu_master_cfg *cfg = find_smmu_master_cfg(dev);
> 	struct arm_smmu_device *smmu = cfg->smmu;
> 	struct arm_smmu_smr *smrs = smmu->smrs;
> -	struct iommu_group *group;
> 	int i, idx, ret;
> 
> 	spin_lock(&smmu->stream_map_lock);
> @@ -1523,19 +1521,9 @@ static int arm_smmu_master_alloc_smes(struct device *dev)
> 		cfg->smendx[i] = (s16)idx;
> 	}
> 
> -	group = iommu_group_get(dev);
> -	if (!group)
> -		group = ERR_PTR(-ENOMEM);
> -	if (IS_ERR(group)) {
> -		ret = PTR_ERR(group);
> -		goto out_err;
> -	}
> -	iommu_group_put(group);
> -
> 	/* It worked! Now, poke the actual hardware */
> 	for_each_cfg_sme(cfg, i, idx) {
> 		arm_smmu_write_sme(smmu, idx);
> -		smmu->s2crs[idx].group = group;
> 	}
> 
> 	spin_unlock(&smmu->stream_map_lock);
> @@ -1966,27 +1954,6 @@ static void __arm_smmu_release_pci_iommudata(void *data)
> 	kfree(data);
> }
> 
> -static struct iommu_group *arm_smmu_device_group(struct
> -						arm_smmu_master_cfg *cfg)
> -{
> -	struct arm_smmu_device *smmu = cfg->smmu;
> -	struct iommu_group *group = NULL;
> -	int i, idx;
> -
> -	for_each_cfg_sme(cfg, i, idx) {
> -		if (group && smmu->s2crs[idx].group &&
> -		    group != smmu->s2crs[idx].group)
> -			return ERR_PTR(-EINVAL);
> -
> -		group = smmu->s2crs[idx].group;
> -	}
> -
> -	if (group)
> -		return group;
> -
> -	return NULL;
> -}
> -
> static int arm_smmu_add_device(struct device *dev)
> {
> 	struct arm_smmu_device *smmu;
> @@ -2027,13 +1994,10 @@ static int arm_smmu_add_device(struct device *dev)
> 		cfg->smmu = smmu;
> 	}
> 
> -	group = arm_smmu_device_group(cfg);
> -	if (!group) {
> -		group = iommu_group_alloc();
> -		if (IS_ERR(group)) {
> -			dev_err(dev, "Failed to allocate IOMMU group\n");
> -			return PTR_ERR(group);
> -		}
> +	group = iommu_group_alloc();
> +	if (IS_ERR(group)) {
> +		dev_err(dev, "Failed to allocate IOMMU group\n");
> +		return PTR_ERR(group);
> 	}
> 
> 	iommu_group_set_iommudata(group, cfg, releasefn);
> -- 
> 2.17.1
> 

Re: [PATCH] xen/arm: smmuv1: Revert associating the group pointer with the S2CR

[Julien Grall]

Hi,

On 16/04/2021 12:25, Rahul Singh wrote:
> Revert the code that associates the group pointer with the S2CR as this
> code causing an issue when the SMMU device has more than one master
> device.

It is not clear to me why this change was first added. Are we missing 
any feature when reverting it?

> This code is merged with the commit  "xen/arm: smmuv1: Intelligent
> SMR allocation”.
> 

This wants a fixes tag. Can be added on commit if there is nothing else 
to change.

Cheers,

-- 
Julien Grall

Re: [PATCH] xen/arm: smmuv1: Revert associating the group pointer with the S2CR

[Rahul Singh]

Hi Julien,

> On 16 Apr 2021, at 3:35 pm, Julien Grall <julien@xen.org> wrote:
> 
> Hi,
> 
> On 16/04/2021 12:25, Rahul Singh wrote:
>> Revert the code that associates the group pointer with the S2CR as this
>> code causing an issue when the SMMU device has more than one master
>> device.
> 
> It is not clear to me why this change was first added. Are we missing any feature when reverting it?

This feature was added when we backported the code from Linux to fix the stream match conflict issue
as part of commit "xen/arm: smmuv1: Intelligent SMR allocation”. 

This is an extra feature added to allocate IOMMU group based on stream-id. If two device has the
same stream-id then we assign those devices to the same group. This code was removed from Linux
later point in time when IOMMU group handling is done by Linux code not by a specific IOMMU driver.
Therefore I think it is ok revert the code.

> 
>> This code is merged with the commit  "xen/arm: smmuv1: Intelligent
>> SMR allocation”.
> 
> This wants a fixes tag. Can be added on commit if there is nothing else to change.

Yes. That will be great. This fixes the commit “0435784cc75dcfef3b5f59c29deb1dbb84265ddb:xen/arm: smmuv1: Intelligent SMR allocation”

Regards,
Rahul
> 
> Cheers,
> 
> -- 
> Julien Grall

Re: [PATCH] xen/arm: smmuv1: Revert associating the group pointer with the S2CR

[Julien Grall]

On 16/04/2021 16:01, Rahul Singh wrote:
> Hi Julien,

Hi Rahul,

> 
>> On 16 Apr 2021, at 3:35 pm, Julien Grall <julien@xen.org> wrote:
>>
>> Hi,
>>
>> On 16/04/2021 12:25, Rahul Singh wrote:
>>> Revert the code that associates the group pointer with the S2CR as this
>>> code causing an issue when the SMMU device has more than one master
>>> device.
>>
>> It is not clear to me why this change was first added. Are we missing any feature when reverting it?
> 
> This feature was added when we backported the code from Linux to fix the stream match conflict issue
> as part of commit "xen/arm: smmuv1: Intelligent SMR allocation”.
> 
> This is an extra feature added to allocate IOMMU group based on stream-id. If two device has the
> same stream-id then we assign those devices to the same group. 

If we revert the patch, then it would not be possible to use the SMMU if 
two devices use the same stream-id. Is that correct?

> This code was removed from Linux
> later point in time when IOMMU group handling is done by Linux code not by a specific IOMMU driver.

Right.... But Linux still support that option. Is that correct?

> Therefore I think it is ok revert the code.

I am ok with the principle of (partially) reverting patch to unblock the 
situation. But I have to admit, I don't quite understand why this is 
reverted rather than fixed.

Cheers,

-- 
Julien Grall

Re: [PATCH] xen/arm: smmuv1: Revert associating the group pointer with the S2CR

[Rahul Singh]

Hi Julein

> On 16 Apr 2021, at 4:23 pm, Julien Grall <julien@xen.org> wrote:
> 
> 
> 
> On 16/04/2021 16:01, Rahul Singh wrote:
>> Hi Julien,
> 
> Hi Rahul,
> 
>>> On 16 Apr 2021, at 3:35 pm, Julien Grall <julien@xen.org> wrote:
>>> 
>>> Hi,
>>> 
>>> On 16/04/2021 12:25, Rahul Singh wrote:
>>>> Revert the code that associates the group pointer with the S2CR as this
>>>> code causing an issue when the SMMU device has more than one master
>>>> device.
>>> 
>>> It is not clear to me why this change was first added. Are we missing any feature when reverting it?
>> This feature was added when we backported the code from Linux to fix the stream match conflict issue
>> as part of commit "xen/arm: smmuv1: Intelligent SMR allocation”.
>> This is an extra feature added to allocate IOMMU group based on stream-id. If two device has the
>> same stream-id then we assign those devices to the same group. 
> 
> If we revert the patch, then it would not be possible to use the SMMU if two devices use the same stream-id. Is that correct?

No. If we revert the patch we can use the SMMU if two devices use the same stream-id without any issue but each device will be in a separate group.This is same behaviour before the code is merged.
 
> 
>> This code was removed from Linux
>> later point in time when IOMMU group handling is done by Linux code not by a specific IOMMU driver.
> 
> Right.... But Linux still support that option. Is that correct?

Yes Linux support IOMMU groups handling at Linux code level and not by every IOMMU driver. 
That why I though when we add the support for IOMMU group at XEN level we can add this functionality.

> 
>> Therefore I think it is ok revert the code.
> 
> I am ok with the principle of (partially) reverting patch to unblock the situation. But I have to admit, I don't quite understand why this is reverted rather than fixed.

As I mention earlier I reverted this patch until we have proper support for IOMMU groups for ARM. Once we have IOMMU group support for ARM we can add this functionality.

Regards,
Rahul
> 
> Cheers,
> 
> -- 
> Julien Grall

Re: [PATCH] xen/arm: smmuv1: Revert associating the group pointer with the S2CR

[Julien Grall]

On 16/04/2021 17:05, Rahul Singh wrote:
>> On 16 Apr 2021, at 4:23 pm, Julien Grall <julien@xen.org> wrote:
>>
>>
>>
>> On 16/04/2021 16:01, Rahul Singh wrote:
>>> Hi Julien,
>>
>> Hi Rahul,
>>
>>>> On 16 Apr 2021, at 3:35 pm, Julien Grall <julien@xen.org> wrote:
>>>>
>>>> Hi,
>>>>
>>>> On 16/04/2021 12:25, Rahul Singh wrote:
>>>>> Revert the code that associates the group pointer with the S2CR as this
>>>>> code causing an issue when the SMMU device has more than one master
>>>>> device.
>>>>
>>>> It is not clear to me why this change was first added. Are we missing any feature when reverting it?
>>> This feature was added when we backported the code from Linux to fix the stream match conflict issue
>>> as part of commit "xen/arm: smmuv1: Intelligent SMR allocation”.
>>> This is an extra feature added to allocate IOMMU group based on stream-id. If two device has the
>>> same stream-id then we assign those devices to the same group.
>>
>> If we revert the patch, then it would not be possible to use the SMMU if two devices use the same stream-id. Is that correct?
> 
> No. If we revert the patch we can use the SMMU if two devices use the same stream-id without any issue but each device will be in a separate group.This is same behaviour before the code is merged.

Ok. So there is no change in behavior. Good. Can you propose a commit 
message clarifying that?

OOI why was the code added if it doesn't make any difference?

Anyway, thanks for the explanation!

Cheers,

-- 
Julien Grall

Re: [PATCH] xen/arm: smmuv1: Revert associating the group pointer with the S2CR

[Rahul Singh]

Hi Julien

> On 16 Apr 2021, at 5:08 pm, Julien Grall <julien@xen.org> wrote:
> 
> 
> 
> On 16/04/2021 17:05, Rahul Singh wrote:
>>> On 16 Apr 2021, at 4:23 pm, Julien Grall <julien@xen.org> wrote:
>>> 
>>> 
>>> 
>>> On 16/04/2021 16:01, Rahul Singh wrote:
>>>> Hi Julien,
>>> 
>>> Hi Rahul,
>>> 
>>>>> On 16 Apr 2021, at 3:35 pm, Julien Grall <julien@xen.org> wrote:
>>>>> 
>>>>> Hi,
>>>>> 
>>>>> On 16/04/2021 12:25, Rahul Singh wrote:
>>>>>> Revert the code that associates the group pointer with the S2CR as this
>>>>>> code causing an issue when the SMMU device has more than one master
>>>>>> device.
>>>>> 
>>>>> It is not clear to me why this change was first added. Are we missing any feature when reverting it?
>>>> This feature was added when we backported the code from Linux to fix the stream match conflict issue
>>>> as part of commit "xen/arm: smmuv1: Intelligent SMR allocation”.
>>>> This is an extra feature added to allocate IOMMU group based on stream-id. If two device has the
>>>> same stream-id then we assign those devices to the same group.
>>> 
>>> If we revert the patch, then it would not be possible to use the SMMU if two devices use the same stream-id. Is that correct?
>> No. If we revert the patch we can use the SMMU if two devices use the same stream-id without any issue but each device will be in a separate group.This is same behaviour before the code is merged.
> 
> Ok. So there is no change in behavior. Good. Can you propose a commit message clarifying that? 

Please have a look if it make sense.

xen/arm: smmuv1: Revert associating the group pointer with the S2CR

Revert the code that associates the group pointer with the S2CR as this
code causing an issue when the SMMU device has more than one master
device with same stream-id. This issue is introduced by the below commit:

“0435784cc75dcfef3b5f59c29deb1dbb84265ddb:xen/arm: smmuv1: Intelligent SMR allocation”
 
Reverting the code will not impact to use of SMMU if two devices use the
same stream-id but each device will be in a separate group. This is the same
behaviour before the code is merged.

> 
> OOI why was the code added if it doesn't make any difference?

This code is part of the code in Linux commit to fix stream match conflict issue when two devices have the same stream-id. See Linux commit message.
https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/drivers/iommu/arm-smmu.c?h=linux-5.8.y&id=588888a7399db352d2b1a41c9d5b3bf0fd482390

When I tested the code on Juno board I didn’t  observe any issue so I thought it is ok to merge this code to have clean backport.

Regards,
Rahul

> 
> Anyway, thanks for the explanation!
> 
> Cheers,
> 
> -- 
> Julien Grall

Re: [PATCH] xen/arm: smmuv1: Revert associating the group pointer with the S2CR

[Julien Grall]

On 16/04/2021 17:41, Rahul Singh wrote:
> Hi Julien

Hi Rahul,

>> On 16 Apr 2021, at 5:08 pm, Julien Grall <julien@xen.org> wrote:
>>
>>
>>
>> On 16/04/2021 17:05, Rahul Singh wrote:
>>>> On 16 Apr 2021, at 4:23 pm, Julien Grall <julien@xen.org> wrote:
>>>>
>>>>
>>>>
>>>> On 16/04/2021 16:01, Rahul Singh wrote:
>>>>> Hi Julien,
>>>>
>>>> Hi Rahul,
>>>>
>>>>>> On 16 Apr 2021, at 3:35 pm, Julien Grall <julien@xen.org> wrote:
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> On 16/04/2021 12:25, Rahul Singh wrote:
>>>>>>> Revert the code that associates the group pointer with the S2CR as this
>>>>>>> code causing an issue when the SMMU device has more than one master
>>>>>>> device.
>>>>>>
>>>>>> It is not clear to me why this change was first added. Are we missing any feature when reverting it?
>>>>> This feature was added when we backported the code from Linux to fix the stream match conflict issue
>>>>> as part of commit "xen/arm: smmuv1: Intelligent SMR allocation”.
>>>>> This is an extra feature added to allocate IOMMU group based on stream-id. If two device has the
>>>>> same stream-id then we assign those devices to the same group.
>>>>
>>>> If we revert the patch, then it would not be possible to use the SMMU if two devices use the same stream-id. Is that correct?
>>> No. If we revert the patch we can use the SMMU if two devices use the same stream-id without any issue but each device will be in a separate group.This is same behaviour before the code is merged.
>>
>> Ok. So there is no change in behavior. Good. Can you propose a commit message clarifying that?
> 
> Please have a look if it make sense.
> 
> xen/arm: smmuv1: Revert associating the group pointer with the S2CR
> 
> Revert the code that associates the group pointer with the S2CR as this
> code causing an issue when the SMMU device has more than one master
> device with same stream-id. This issue is introduced by the below commit:
> 
> “0435784cc75dcfef3b5f59c29deb1dbb84265ddb:xen/arm: smmuv1: Intelligent SMR allocation”
>   
> Reverting the code will not impact to use of SMMU if two devices use the
> same stream-id but each device will be in a separate group. This is the same
> behaviour before the code is merged.

Look good to me. Is this patch to be applied on top of Stefano's series? 
If not, is there going to be more clash?

Cheers,

-- 
Julien Grall

Re: [PATCH] xen/arm: smmuv1: Revert associating the group pointer with the S2CR

[Rahul Singh]

Hi Julien,

> On 18 Apr 2021, at 6:48 pm, Julien Grall <julien@xen.org> wrote:
> 
> 
> 
> On 16/04/2021 17:41, Rahul Singh wrote:
>> Hi Julien
> 
> Hi Rahul,
> 
>>> On 16 Apr 2021, at 5:08 pm, Julien Grall <julien@xen.org> wrote:
>>> 
>>> 
>>> 
>>> On 16/04/2021 17:05, Rahul Singh wrote:
>>>>> On 16 Apr 2021, at 4:23 pm, Julien Grall <julien@xen.org> wrote:
>>>>> 
>>>>> 
>>>>> 
>>>>> On 16/04/2021 16:01, Rahul Singh wrote:
>>>>>> Hi Julien,
>>>>> 
>>>>> Hi Rahul,
>>>>> 
>>>>>>> On 16 Apr 2021, at 3:35 pm, Julien Grall <julien@xen.org> wrote:
>>>>>>> 
>>>>>>> Hi,
>>>>>>> 
>>>>>>> On 16/04/2021 12:25, Rahul Singh wrote:
>>>>>>>> Revert the code that associates the group pointer with the S2CR as this
>>>>>>>> code causing an issue when the SMMU device has more than one master
>>>>>>>> device.
>>>>>>> 
>>>>>>> It is not clear to me why this change was first added. Are we missing any feature when reverting it?
>>>>>> This feature was added when we backported the code from Linux to fix the stream match conflict issue
>>>>>> as part of commit "xen/arm: smmuv1: Intelligent SMR allocation”.
>>>>>> This is an extra feature added to allocate IOMMU group based on stream-id. If two device has the
>>>>>> same stream-id then we assign those devices to the same group.
>>>>> 
>>>>> If we revert the patch, then it would not be possible to use the SMMU if two devices use the same stream-id. Is that correct?
>>>> No. If we revert the patch we can use the SMMU if two devices use the same stream-id without any issue but each device will be in a separate group.This is same behaviour before the code is merged.
>>> 
>>> Ok. So there is no change in behavior. Good. Can you propose a commit message clarifying that?
>> Please have a look if it make sense.
>> xen/arm: smmuv1: Revert associating the group pointer with the S2CR
>> Revert the code that associates the group pointer with the S2CR as this
>> code causing an issue when the SMMU device has more than one master
>> device with same stream-id. This issue is introduced by the below commit:
>> “0435784cc75dcfef3b5f59c29deb1dbb84265ddb:xen/arm: smmuv1: Intelligent SMR allocation”
>>  Reverting the code will not impact to use of SMMU if two devices use the
>> same stream-id but each device will be in a separate group. This is the same
>> behaviour before the code is merged.
> 
> Look good to me. Is this patch to be applied on top of Stefano's series? If not, is there going to be more clash?
> 

As per Stefano's mail he already tested his patch series on top of this patch. I think this patch has to merged before Stefano’s patch series 
Let Stefano also confirm that.

I think there will be no more clashes.


Regards,
Rahul

> Cheers,
> 
> -- 
> Julien Grall

Re: [PATCH] xen/arm: smmuv1: Revert associating the group pointer with the S2CR

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 2992 bytes --]

On Mon, 19 Apr 2021, Rahul Singh wrote:
> Hi Julien,
> 
> > On 18 Apr 2021, at 6:48 pm, Julien Grall <julien@xen.org> wrote:
> > 
> > 
> > 
> > On 16/04/2021 17:41, Rahul Singh wrote:
> >> Hi Julien
> > 
> > Hi Rahul,
> > 
> >>> On 16 Apr 2021, at 5:08 pm, Julien Grall <julien@xen.org> wrote:
> >>> 
> >>> 
> >>> 
> >>> On 16/04/2021 17:05, Rahul Singh wrote:
> >>>>> On 16 Apr 2021, at 4:23 pm, Julien Grall <julien@xen.org> wrote:
> >>>>> 
> >>>>> 
> >>>>> 
> >>>>> On 16/04/2021 16:01, Rahul Singh wrote:
> >>>>>> Hi Julien,
> >>>>> 
> >>>>> Hi Rahul,
> >>>>> 
> >>>>>>> On 16 Apr 2021, at 3:35 pm, Julien Grall <julien@xen.org> wrote:
> >>>>>>> 
> >>>>>>> Hi,
> >>>>>>> 
> >>>>>>> On 16/04/2021 12:25, Rahul Singh wrote:
> >>>>>>>> Revert the code that associates the group pointer with the S2CR as this
> >>>>>>>> code causing an issue when the SMMU device has more than one master
> >>>>>>>> device.
> >>>>>>> 
> >>>>>>> It is not clear to me why this change was first added. Are we missing any feature when reverting it?
> >>>>>> This feature was added when we backported the code from Linux to fix the stream match conflict issue
> >>>>>> as part of commit "xen/arm: smmuv1: Intelligent SMR allocation”.
> >>>>>> This is an extra feature added to allocate IOMMU group based on stream-id. If two device has the
> >>>>>> same stream-id then we assign those devices to the same group.
> >>>>> 
> >>>>> If we revert the patch, then it would not be possible to use the SMMU if two devices use the same stream-id. Is that correct?
> >>>> No. If we revert the patch we can use the SMMU if two devices use the same stream-id without any issue but each device will be in a separate group.This is same behaviour before the code is merged.
> >>> 
> >>> Ok. So there is no change in behavior. Good. Can you propose a commit message clarifying that?
> >> Please have a look if it make sense.
> >> xen/arm: smmuv1: Revert associating the group pointer with the S2CR
> >> Revert the code that associates the group pointer with the S2CR as this
> >> code causing an issue when the SMMU device has more than one master
> >> device with same stream-id. This issue is introduced by the below commit:
> >> “0435784cc75dcfef3b5f59c29deb1dbb84265ddb:xen/arm: smmuv1: Intelligent SMR allocation”
> >>  Reverting the code will not impact to use of SMMU if two devices use the
> >> same stream-id but each device will be in a separate group. This is the same
> >> behaviour before the code is merged.
> > 
> > Look good to me. Is this patch to be applied on top of Stefano's series? If not, is there going to be more clash?
> > 
> 
> As per Stefano's mail he already tested his patch series on top of this patch. I think this patch has to merged before Stefano’s patch series 
> Let Stefano also confirm that.
> 
> I think there will be no more clashes.

Yes, this patch is to be committed *before* my series and I have already
tested this patch alone and with my series on top. Both cases work fine.

Re: [PATCH] xen/arm: smmuv1: Revert associating the group pointer with the S2CR

[Julien Grall]

Hi,

On 19/04/2021 17:21, Stefano Stabellini wrote:
> On Mon, 19 Apr 2021, Rahul Singh wrote:
>> Hi Julien,
>>
>>> On 18 Apr 2021, at 6:48 pm, Julien Grall <julien@xen.org> wrote:
>>>
>>>
>>>
>>> On 16/04/2021 17:41, Rahul Singh wrote:
>>>> Hi Julien
>>>
>>> Hi Rahul,
>>>
>>>>> On 16 Apr 2021, at 5:08 pm, Julien Grall <julien@xen.org> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On 16/04/2021 17:05, Rahul Singh wrote:
>>>>>>> On 16 Apr 2021, at 4:23 pm, Julien Grall <julien@xen.org> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 16/04/2021 16:01, Rahul Singh wrote:
>>>>>>>> Hi Julien,
>>>>>>>
>>>>>>> Hi Rahul,
>>>>>>>
>>>>>>>>> On 16 Apr 2021, at 3:35 pm, Julien Grall <julien@xen.org> wrote:
>>>>>>>>>
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> On 16/04/2021 12:25, Rahul Singh wrote:
>>>>>>>>>> Revert the code that associates the group pointer with the S2CR as this
>>>>>>>>>> code causing an issue when the SMMU device has more than one master
>>>>>>>>>> device.
>>>>>>>>>
>>>>>>>>> It is not clear to me why this change was first added. Are we missing any feature when reverting it?
>>>>>>>> This feature was added when we backported the code from Linux to fix the stream match conflict issue
>>>>>>>> as part of commit "xen/arm: smmuv1: Intelligent SMR allocation”.
>>>>>>>> This is an extra feature added to allocate IOMMU group based on stream-id. If two device has the
>>>>>>>> same stream-id then we assign those devices to the same group.
>>>>>>>
>>>>>>> If we revert the patch, then it would not be possible to use the SMMU if two devices use the same stream-id. Is that correct?
>>>>>> No. If we revert the patch we can use the SMMU if two devices use the same stream-id without any issue but each device will be in a separate group.This is same behaviour before the code is merged.
>>>>>
>>>>> Ok. So there is no change in behavior. Good. Can you propose a commit message clarifying that?
>>>> Please have a look if it make sense.
>>>> xen/arm: smmuv1: Revert associating the group pointer with the S2CR
>>>> Revert the code that associates the group pointer with the S2CR as this
>>>> code causing an issue when the SMMU device has more than one master
>>>> device with same stream-id. This issue is introduced by the below commit:
>>>> “0435784cc75dcfef3b5f59c29deb1dbb84265ddb:xen/arm: smmuv1: Intelligent SMR allocation”
>>>>   Reverting the code will not impact to use of SMMU if two devices use the
>>>> same stream-id but each device will be in a separate group. This is the same
>>>> behaviour before the code is merged.
>>>
>>> Look good to me. Is this patch to be applied on top of Stefano's series? If not, is there going to be more clash?
>>>
>>
>> As per Stefano's mail he already tested his patch series on top of this patch. I think this patch has to merged before Stefano’s patch series
>> Let Stefano also confirm that.
>>
>> I think there will be no more clashes.
> 
> Yes, this patch is to be committed *before* my series and I have already
> tested this patch alone and with my series on top. Both cases work fine.

Cool. Thanks for the confirmation. I have committed the patch with the 
new commit message (although, I tweaked a little bit to use the 
abbreviated version of the commit ID).

Cheers,

-- 
Julien Grall

Re: [PATCH] xen/arm: smmuv1: Revert associating the group pointer with the S2CR

[Rahul Singh]

Hi Julien,

> On 20 Apr 2021, at 1:39 pm, Julien Grall <julien@xen.org> wrote:
> 
> Hi,
> 
> On 19/04/2021 17:21, Stefano Stabellini wrote:
>> On Mon, 19 Apr 2021, Rahul Singh wrote:
>>> Hi Julien,
>>> 
>>>> On 18 Apr 2021, at 6:48 pm, Julien Grall <julien@xen.org> wrote:
>>>> 
>>>> 
>>>> 
>>>> On 16/04/2021 17:41, Rahul Singh wrote:
>>>>> Hi Julien
>>>> 
>>>> Hi Rahul,
>>>> 
>>>>>> On 16 Apr 2021, at 5:08 pm, Julien Grall <julien@xen.org> wrote:
>>>>>> 
>>>>>> 
>>>>>> 
>>>>>> On 16/04/2021 17:05, Rahul Singh wrote:
>>>>>>>> On 16 Apr 2021, at 4:23 pm, Julien Grall <julien@xen.org> wrote:
>>>>>>>> 
>>>>>>>> 
>>>>>>>> 
>>>>>>>> On 16/04/2021 16:01, Rahul Singh wrote:
>>>>>>>>> Hi Julien,
>>>>>>>> 
>>>>>>>> Hi Rahul,
>>>>>>>> 
>>>>>>>>>> On 16 Apr 2021, at 3:35 pm, Julien Grall <julien@xen.org> wrote:
>>>>>>>>>> 
>>>>>>>>>> Hi,
>>>>>>>>>> 
>>>>>>>>>> On 16/04/2021 12:25, Rahul Singh wrote:
>>>>>>>>>>> Revert the code that associates the group pointer with the S2CR as this
>>>>>>>>>>> code causing an issue when the SMMU device has more than one master
>>>>>>>>>>> device.
>>>>>>>>>> 
>>>>>>>>>> It is not clear to me why this change was first added. Are we missing any feature when reverting it?
>>>>>>>>> This feature was added when we backported the code from Linux to fix the stream match conflict issue
>>>>>>>>> as part of commit "xen/arm: smmuv1: Intelligent SMR allocation”.
>>>>>>>>> This is an extra feature added to allocate IOMMU group based on stream-id. If two device has the
>>>>>>>>> same stream-id then we assign those devices to the same group.
>>>>>>>> 
>>>>>>>> If we revert the patch, then it would not be possible to use the SMMU if two devices use the same stream-id. Is that correct?
>>>>>>> No. If we revert the patch we can use the SMMU if two devices use the same stream-id without any issue but each device will be in a separate group.This is same behaviour before the code is merged.
>>>>>> 
>>>>>> Ok. So there is no change in behavior. Good. Can you propose a commit message clarifying that?
>>>>> Please have a look if it make sense.
>>>>> xen/arm: smmuv1: Revert associating the group pointer with the S2CR
>>>>> Revert the code that associates the group pointer with the S2CR as this
>>>>> code causing an issue when the SMMU device has more than one master
>>>>> device with same stream-id. This issue is introduced by the below commit:
>>>>> “0435784cc75dcfef3b5f59c29deb1dbb84265ddb:xen/arm: smmuv1: Intelligent SMR allocation”
>>>>>  Reverting the code will not impact to use of SMMU if two devices use the
>>>>> same stream-id but each device will be in a separate group. This is the same
>>>>> behaviour before the code is merged.
>>>> 
>>>> Look good to me. Is this patch to be applied on top of Stefano's series? If not, is there going to be more clash?
>>>> 
>>> 
>>> As per Stefano's mail he already tested his patch series on top of this patch. I think this patch has to merged before Stefano’s patch series
>>> Let Stefano also confirm that.
>>> 
>>> I think there will be no more clashes.
>> Yes, this patch is to be committed *before* my series and I have already
>> tested this patch alone and with my series on top. Both cases work fine.
> 
> Cool. Thanks for the confirmation. I have committed the patch with the new commit message (although, I tweaked a little bit to use the abbreviated version of the commit ID).
> 

Thanks! 

Regards,
Rahul
> Cheers,
> 
> -- 
> Julien Grall