[PATCH] xen/arm : smmuv3: Fix to handle multiple StreamIds per device.

Rahul Singh posted 1 patch 3 years, 1 month ago
Test gitlab-ci passed
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/43de5b58df37d8b8de037cb23c47ab8454caf37c.1613492577.git.rahul.singh@arm.com
xen/drivers/passthrough/arm/smmu-v3.c | 29 ++++++++++-----------------
1 file changed, 11 insertions(+), 18 deletions(-)
[PATCH] xen/arm : smmuv3: Fix to handle multiple StreamIds per device.
Posted by Rahul Singh 3 years, 1 month ago
SMMUv3 driver does not handle multiple StreamId if the master device
supports more than one StreamID.

This bug was introduced when the driver was ported from Linux to XEN.
dt_device_set_protected(..) should be called from add_device(..) not
from the dt_xlate(..).

Move dt_device_set_protected(..) from dt_xlate(..) to add_device().

Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---
This patch is a candidate for 4.15 as without this patch it is not possible to
assign multiple StreamIds to the same device when device is protected behind
SMMUv3.
---
 xen/drivers/passthrough/arm/smmu-v3.c | 29 ++++++++++-----------------
 1 file changed, 11 insertions(+), 18 deletions(-)

diff --git a/xen/drivers/passthrough/arm/smmu-v3.c b/xen/drivers/passthrough/arm/smmu-v3.c
index 914cdc1cf4..53d150cdb6 100644
--- a/xen/drivers/passthrough/arm/smmu-v3.c
+++ b/xen/drivers/passthrough/arm/smmu-v3.c
@@ -2207,24 +2207,6 @@ static int arm_smmu_add_device(u8 devfn, struct device *dev)
 	 */
 	arm_smmu_enable_pasid(master);
 
-	return 0;
-
-err_free_master:
-	xfree(master);
-	dev_iommu_priv_set(dev, NULL);
-	return ret;
-}
-
-static int arm_smmu_dt_xlate(struct device *dev,
-				const struct dt_phandle_args *args)
-{
-	int ret;
-	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
-
-	ret = iommu_fwspec_add_ids(dev, args->args, 1);
-	if (ret)
-		return ret;
-
 	if (dt_device_is_protected(dev_to_dt(dev))) {
 		dev_err(dev, "Already added to SMMUv3\n");
 		return -EEXIST;
@@ -2237,6 +2219,17 @@ static int arm_smmu_dt_xlate(struct device *dev,
 			dev_name(fwspec->iommu_dev), fwspec->num_ids);
 
 	return 0;
+
+err_free_master:
+	xfree(master);
+	dev_iommu_priv_set(dev, NULL);
+	return ret;
+}
+
+static int arm_smmu_dt_xlate(struct device *dev,
+				const struct dt_phandle_args *args)
+{
+	return iommu_fwspec_add_ids(dev, args->args, 1);
 }
 
 /* Probing and initialisation functions */
-- 
2.17.1


Re: [PATCH] xen/arm : smmuv3: Fix to handle multiple StreamIds per device.
Posted by Bertrand Marquis 3 years, 1 month ago
Hi Rahul,


> On 17 Feb 2021, at 10:05, Rahul Singh <Rahul.Singh@arm.com> wrote:
> 
> SMMUv3 driver does not handle multiple StreamId if the master device
> supports more than one StreamID.
> 
> This bug was introduced when the driver was ported from Linux to XEN.
> dt_device_set_protected(..) should be called from add_device(..) not
> from the dt_xlate(..).
> 
> Move dt_device_set_protected(..) from dt_xlate(..) to add_device().
> 
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>

Thanks a lot, this is fixing issues with multiple stream ids for one device :-)

Cheers
Bertrand

> ---
> This patch is a candidate for 4.15 as without this patch it is not possible to
> assign multiple StreamIds to the same device when device is protected behind
> SMMUv3.
> ---
> xen/drivers/passthrough/arm/smmu-v3.c | 29 ++++++++++-----------------
> 1 file changed, 11 insertions(+), 18 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/arm/smmu-v3.c b/xen/drivers/passthrough/arm/smmu-v3.c
> index 914cdc1cf4..53d150cdb6 100644
> --- a/xen/drivers/passthrough/arm/smmu-v3.c
> +++ b/xen/drivers/passthrough/arm/smmu-v3.c
> @@ -2207,24 +2207,6 @@ static int arm_smmu_add_device(u8 devfn, struct device *dev)
> 	 */
> 	arm_smmu_enable_pasid(master);
> 
> -	return 0;
> -
> -err_free_master:
> -	xfree(master);
> -	dev_iommu_priv_set(dev, NULL);
> -	return ret;
> -}
> -
> -static int arm_smmu_dt_xlate(struct device *dev,
> -				const struct dt_phandle_args *args)
> -{
> -	int ret;
> -	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> -
> -	ret = iommu_fwspec_add_ids(dev, args->args, 1);
> -	if (ret)
> -		return ret;
> -
> 	if (dt_device_is_protected(dev_to_dt(dev))) {
> 		dev_err(dev, "Already added to SMMUv3\n");
> 		return -EEXIST;
> @@ -2237,6 +2219,17 @@ static int arm_smmu_dt_xlate(struct device *dev,
> 			dev_name(fwspec->iommu_dev), fwspec->num_ids);
> 
> 	return 0;
> +
> +err_free_master:
> +	xfree(master);
> +	dev_iommu_priv_set(dev, NULL);
> +	return ret;
> +}
> +
> +static int arm_smmu_dt_xlate(struct device *dev,
> +				const struct dt_phandle_args *args)
> +{
> +	return iommu_fwspec_add_ids(dev, args->args, 1);
> }
> 
> /* Probing and initialisation functions */
> -- 
> 2.17.1
> 


Re: [PATCH] xen/arm : smmuv3: Fix to handle multiple StreamIds per device.
Posted by Stefano Stabellini 3 years, 1 month ago
+IanJ

On Wed, 17 Feb 2021, Bertrand Marquis wrote:
> Hi Rahul,
> 
> 
> > On 17 Feb 2021, at 10:05, Rahul Singh <Rahul.Singh@arm.com> wrote:
> > 
> > SMMUv3 driver does not handle multiple StreamId if the master device
> > supports more than one StreamID.
> > 
> > This bug was introduced when the driver was ported from Linux to XEN.
> > dt_device_set_protected(..) should be called from add_device(..) not
> > from the dt_xlate(..).
> > 
> > Move dt_device_set_protected(..) from dt_xlate(..) to add_device().
> > 
> > Signed-off-by: Rahul Singh <rahul.singh@arm.com>
> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
> 
> Thanks a lot, this is fixing issues with multiple stream ids for one device :-)

Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>


> > ---
> > This patch is a candidate for 4.15 as without this patch it is not possible to
> > assign multiple StreamIds to the same device when device is protected behind
> > SMMUv3.

I agree especially considering that the impact is limited to smmu-v3.c.


> > ---
> > xen/drivers/passthrough/arm/smmu-v3.c | 29 ++++++++++-----------------
> > 1 file changed, 11 insertions(+), 18 deletions(-)
> > 
> > diff --git a/xen/drivers/passthrough/arm/smmu-v3.c b/xen/drivers/passthrough/arm/smmu-v3.c
> > index 914cdc1cf4..53d150cdb6 100644
> > --- a/xen/drivers/passthrough/arm/smmu-v3.c
> > +++ b/xen/drivers/passthrough/arm/smmu-v3.c
> > @@ -2207,24 +2207,6 @@ static int arm_smmu_add_device(u8 devfn, struct device *dev)
> > 	 */
> > 	arm_smmu_enable_pasid(master);
> > 
> > -	return 0;
> > -
> > -err_free_master:
> > -	xfree(master);
> > -	dev_iommu_priv_set(dev, NULL);
> > -	return ret;
> > -}
> > -
> > -static int arm_smmu_dt_xlate(struct device *dev,
> > -				const struct dt_phandle_args *args)
> > -{
> > -	int ret;
> > -	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> > -
> > -	ret = iommu_fwspec_add_ids(dev, args->args, 1);
> > -	if (ret)
> > -		return ret;
> > -
> > 	if (dt_device_is_protected(dev_to_dt(dev))) {
> > 		dev_err(dev, "Already added to SMMUv3\n");
> > 		return -EEXIST;
> > @@ -2237,6 +2219,17 @@ static int arm_smmu_dt_xlate(struct device *dev,
> > 			dev_name(fwspec->iommu_dev), fwspec->num_ids);
> > 
> > 	return 0;
> > +
> > +err_free_master:
> > +	xfree(master);
> > +	dev_iommu_priv_set(dev, NULL);
> > +	return ret;
> > +}
> > +
> > +static int arm_smmu_dt_xlate(struct device *dev,
> > +				const struct dt_phandle_args *args)
> > +{
> > +	return iommu_fwspec_add_ids(dev, args->args, 1);
> > }
> > 
> > /* Probing and initialisation functions */
> > -- 
> > 2.17.1
> > 
> 

Re: [PATCH] xen/arm : smmuv3: Fix to handle multiple StreamIds per device.
Posted by Julien Grall 3 years, 1 month ago
Hi,

On 17/02/2021 23:23, Stefano Stabellini wrote:
> +IanJ
> 
> On Wed, 17 Feb 2021, Bertrand Marquis wrote:
>> Hi Rahul,
>>
>>
>>> On 17 Feb 2021, at 10:05, Rahul Singh <Rahul.Singh@arm.com> wrote:
>>>
>>> SMMUv3 driver does not handle multiple StreamId if the master device
>>> supports more than one StreamID.
>>>
>>> This bug was introduced when the driver was ported from Linux to XEN.
>>> dt_device_set_protected(..) should be called from add_device(..) not
>>> from the dt_xlate(..).
>>>
>>> Move dt_device_set_protected(..) from dt_xlate(..) to add_device().
>>>
>>> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
>> Reviewed-by: Bertrand Marquis <bertrand.marquis@arm.com>
>>
>> Thanks a lot, this is fixing issues with multiple stream ids for one device :-)
> 
> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
> 
> 
>>> ---
>>> This patch is a candidate for 4.15 as without this patch it is not possible to
>>> assign multiple StreamIds to the same device when device is protected behind
>>> SMMUv3.
> 
> I agree especially considering that the impact is limited to smmu-v3.c.

SMMUv3 is in tech preview, so the risk is pretty low as we don't support 
it :).

But, a release ack from Ian is not necessary until tonight (19th 
February) for bug fixes. So I have committed the patch.

Cheers,

-- 
Julien Grall