[v3] Generic SMMU Bindings

[PATCH RESEND v3 3/3] arm,smmu: add support for generic DT bindings. Implement add_device and dt_xlate.

Posted by Stefano Stabellini 3 years, 7 months ago

From: Brian Woods <brian.woods@xilinx.com>

Now that all arm iommu drivers support generic bindings we can remove
the workaround from iommu_add_dt_device().

Note that if both legacy bindings and generic bindings are present in
device tree, the legacy bindings are the ones that are used.

Signed-off-by: Brian Woods <brian.woods@xilinx.com>
Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
Reviewed-by: Rahul Singh <rahul.singh@arm.com>
---
 xen/drivers/passthrough/arm/smmu.c    | 42 ++++++++++++++++++++++++++-
 xen/drivers/passthrough/device_tree.c | 17 +----------
 2 files changed, 42 insertions(+), 17 deletions(-)

diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index f949c110ad..b564851a56 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -254,6 +254,8 @@ struct iommu_group
 	atomic_t ref;
 };
 
+static struct arm_smmu_device *find_smmu(const struct device *dev);
+
 static struct iommu_group *iommu_group_alloc(void)
 {
 	struct iommu_group *group = xzalloc(struct iommu_group);
@@ -442,6 +444,8 @@ static struct iommu_group *iommu_group_get(struct device *dev)
 #define SMR_VALID			(1U << 31)
 #define SMR_MASK_SHIFT			16
 #define SMR_ID_SHIFT			0
+#define SMR_ID_MASK			0x7fff
+#define SMR_MASK_MASK			0x7fff
 
 #define ARM_SMMU_GR0_S2CR(n)		(0xc00 + ((n) << 2))
 #define S2CR_CBNDX_SHIFT		0
@@ -872,6 +876,40 @@ static int register_smmu_master(struct arm_smmu_device *smmu,
 					     fwspec);
 }
 
+static int arm_smmu_dt_add_device_generic(u8 devfn, struct device *dev)
+{
+	struct arm_smmu_device *smmu;
+	struct iommu_fwspec *fwspec;
+
+	fwspec = dev_iommu_fwspec_get(dev);
+	if (fwspec == NULL)
+		return -ENXIO;
+
+	smmu = find_smmu(fwspec->iommu_dev);
+	if (smmu == NULL)
+		return -ENXIO;
+
+	return arm_smmu_dt_add_device_legacy(smmu, dev, fwspec);
+}
+
+static int arm_smmu_dt_xlate_generic(struct device *dev,
+				    const struct dt_phandle_args *spec)
+{
+	uint32_t mask, fwid = 0;
+
+	if (spec->args_count > 0)
+		fwid |= (SMR_ID_MASK & spec->args[0]) << SMR_ID_SHIFT;
+
+	if (spec->args_count > 1)
+		fwid |= (SMR_MASK_MASK & spec->args[1]) << SMR_MASK_SHIFT;
+	else if (!of_property_read_u32(spec->np, "stream-match-mask", &mask))
+		fwid |= (SMR_MASK_MASK & mask) << SMR_MASK_SHIFT;
+
+	return iommu_fwspec_add_ids(dev,
+				    &fwid,
+				    1);
+}
+
 static struct arm_smmu_device *find_smmu_for_device(struct device *dev)
 {
 	struct arm_smmu_device *smmu;
@@ -2836,6 +2874,7 @@ static void arm_smmu_iommu_domain_teardown(struct domain *d)
 static const struct iommu_ops arm_smmu_iommu_ops = {
     .init = arm_smmu_iommu_domain_init,
     .hwdom_init = arm_smmu_iommu_hwdom_init,
+    .add_device = arm_smmu_dt_add_device_generic,
     .teardown = arm_smmu_iommu_domain_teardown,
     .iotlb_flush = arm_smmu_iotlb_flush,
     .iotlb_flush_all = arm_smmu_iotlb_flush_all,
@@ -2843,9 +2882,10 @@ static const struct iommu_ops arm_smmu_iommu_ops = {
     .reassign_device = arm_smmu_reassign_dev,
     .map_page = arm_iommu_map_page,
     .unmap_page = arm_iommu_unmap_page,
+    .dt_xlate = arm_smmu_dt_xlate_generic,
 };
 
-static __init const struct arm_smmu_device *find_smmu(const struct device *dev)
+static struct arm_smmu_device *find_smmu(const struct device *dev)
 {
 	struct arm_smmu_device *smmu;
 	bool found = false;
diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
index a51ae3c9c3..ae07f272e1 100644
--- a/xen/drivers/passthrough/device_tree.c
+++ b/xen/drivers/passthrough/device_tree.c
@@ -162,22 +162,7 @@ int iommu_add_dt_device(struct dt_device_node *np)
          * these callback implemented.
          */
         if ( !ops->add_device || !ops->dt_xlate )
-        {
-            /*
-             * Some Device Trees may expose both legacy SMMU and generic
-             * IOMMU bindings together. However, the SMMU driver is only
-             * supporting the former and will protect them during the
-             * initialization. So we need to skip them and not return
-             * error here.
-             *
-             * XXX: This can be dropped when the SMMU is able to deal
-             * with generic bindings.
-             */
-            if ( dt_device_is_protected(np) )
-                return 0;
-            else
-                return -EINVAL;
-        }
+            return -EINVAL;
 
         if ( !dt_device_is_available(iommu_spec.np) )
             break;
-- 
2.17.1

Re: [PATCH RESEND v3 3/3] arm,smmu: add support for generic DT bindings. Implement add_device and dt_xlate.

Posted by Julien Grall 3 years, 7 months ago


On 13/04/2021 18:59, Stefano Stabellini wrote:
> From: Brian Woods <brian.woods@xilinx.com>
> 
> Now that all arm iommu drivers support generic bindings we can remove
> the workaround from iommu_add_dt_device().

Well, it was just added in a different place in patch #1. ;) I have 
commented about it in patch #1.

> 
> Note that if both legacy bindings and generic bindings are present in
> device tree, the legacy bindings are the ones that are used
Can you oultine what guarantee that? Also what happen if some of devices 
are using the generic bindings while other are using the legacy one?
> 
> Signed-off-by: Brian Woods <brian.woods@xilinx.com>
> Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> Reviewed-by: Rahul Singh <rahul.singh@arm.com>
> ---
>   xen/drivers/passthrough/arm/smmu.c    | 42 ++++++++++++++++++++++++++-
>   xen/drivers/passthrough/device_tree.c | 17 +----------
>   2 files changed, 42 insertions(+), 17 deletions(-)
> 
> diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
> index f949c110ad..b564851a56 100644
> --- a/xen/drivers/passthrough/arm/smmu.c
> +++ b/xen/drivers/passthrough/arm/smmu.c
> @@ -254,6 +254,8 @@ struct iommu_group
>   	atomic_t ref;
>   };
>   
> +static struct arm_smmu_device *find_smmu(const struct device *dev);
> +
>   static struct iommu_group *iommu_group_alloc(void)
>   {
>   	struct iommu_group *group = xzalloc(struct iommu_group);
> @@ -442,6 +444,8 @@ static struct iommu_group *iommu_group_get(struct device *dev)
>   #define SMR_VALID			(1U << 31)
>   #define SMR_MASK_SHIFT			16
>   #define SMR_ID_SHIFT			0
> +#define SMR_ID_MASK			0x7fff
> +#define SMR_MASK_MASK			0x7fff
>   
>   #define ARM_SMMU_GR0_S2CR(n)		(0xc00 + ((n) << 2))
>   #define S2CR_CBNDX_SHIFT		0
> @@ -872,6 +876,40 @@ static int register_smmu_master(struct arm_smmu_device *smmu,
>   					     fwspec);
>   }
>   
> +static int arm_smmu_dt_add_device_generic(u8 devfn, struct device *dev)
> +{
> +	struct arm_smmu_device *smmu;
> +	struct iommu_fwspec *fwspec;
> +
> +	fwspec = dev_iommu_fwspec_get(dev);
> +	if (fwspec == NULL)
> +		return -ENXIO;
> +
> +	smmu = find_smmu(fwspec->iommu_dev);
> +	if (smmu == NULL)
> +		return -ENXIO;
> +
> +	return arm_smmu_dt_add_device_legacy(smmu, dev, fwspec);

Patch #2 seems to imply the code was reworked to have distinct path 
between legacy and generic. But now, you are calling the legacy code 
from the generic helper. This is pretty confusing, can you explain 
what's going on?

> +}
> +
> +static int arm_smmu_dt_xlate_generic(struct device *dev,
> +				    const struct dt_phandle_args *spec)

This seems to be a verbatim copy from Linux. It would be good to mention 
it in the commit message. This would make easier to track any change.

> +{
> +	uint32_t mask, fwid = 0;
> +
> +	if (spec->args_count > 0)
> +		fwid |= (SMR_ID_MASK & spec->args[0]) << SMR_ID_SHIFT;
> +
> +	if (spec->args_count > 1)
> +		fwid |= (SMR_MASK_MASK & spec->args[1]) << SMR_MASK_SHIFT;
> +	else if (!of_property_read_u32(spec->np, "stream-match-mask", &mask))
> +		fwid |= (SMR_MASK_MASK & mask) << SMR_MASK_SHIFT;
> +
> +	return iommu_fwspec_add_ids(dev,
> +				    &fwid,
> +				    1);

NIT: This feels a bit odd to read. Can't they be defined on the same line?

> +}
> +
>   static struct arm_smmu_device *find_smmu_for_device(struct device *dev)
>   {
>   	struct arm_smmu_device *smmu;
> @@ -2836,6 +2874,7 @@ static void arm_smmu_iommu_domain_teardown(struct domain *d)
>   static const struct iommu_ops arm_smmu_iommu_ops = {
>       .init = arm_smmu_iommu_domain_init,
>       .hwdom_init = arm_smmu_iommu_hwdom_init,
> +    .add_device = arm_smmu_dt_add_device_generic,
>       .teardown = arm_smmu_iommu_domain_teardown,
>       .iotlb_flush = arm_smmu_iotlb_flush,
>       .iotlb_flush_all = arm_smmu_iotlb_flush_all,
> @@ -2843,9 +2882,10 @@ static const struct iommu_ops arm_smmu_iommu_ops = {
>       .reassign_device = arm_smmu_reassign_dev,
>       .map_page = arm_iommu_map_page,
>       .unmap_page = arm_iommu_unmap_page,
> +    .dt_xlate = arm_smmu_dt_xlate_generic,
>   };
>   
> -static __init const struct arm_smmu_device *find_smmu(const struct device *dev)
> +static struct arm_smmu_device *find_smmu(const struct device *dev)
>   {
>   	struct arm_smmu_device *smmu;
>   	bool found = false;
> diff --git a/xen/drivers/passthrough/device_tree.c b/xen/drivers/passthrough/device_tree.c
> index a51ae3c9c3..ae07f272e1 100644
> --- a/xen/drivers/passthrough/device_tree.c
> +++ b/xen/drivers/passthrough/device_tree.c
> @@ -162,22 +162,7 @@ int iommu_add_dt_device(struct dt_device_node *np)
>            * these callback implemented.
>            */
>           if ( !ops->add_device || !ops->dt_xlate )
> -        {
> -            /*
> -             * Some Device Trees may expose both legacy SMMU and generic
> -             * IOMMU bindings together. However, the SMMU driver is only
> -             * supporting the former and will protect them during the
> -             * initialization. So we need to skip them and not return
> -             * error here.
> -             *
> -             * XXX: This can be dropped when the SMMU is able to deal
> -             * with generic bindings.
> -             */
> -            if ( dt_device_is_protected(np) )
> -                return 0;
> -            else
> -                return -EINVAL;
> -        }
> +            return -EINVAL;
>   
>           if ( !dt_device_is_available(iommu_spec.np) )
>               break;
> 

-- 
Julien Grall

Re: [PATCH RESEND v3 3/3] arm,smmu: add support for generic DT bindings. Implement add_device and dt_xlate.

Posted by Stefano Stabellini 3 years, 4 months ago

On Wed, 28 Apr 2021, Julien Grall wrote:
> On 13/04/2021 18:59, Stefano Stabellini wrote:
> > From: Brian Woods <brian.woods@xilinx.com>
> > 
> > Now that all arm iommu drivers support generic bindings we can remove
> > the workaround from iommu_add_dt_device().
> 
> Well, it was just added in a different place in patch #1. ;) I have commented
> about it in patch #1.

That is a different workaround. This is removing the one introduced by
cf4af9d6d6c (xen/arm: boot with device trees with "mmu-masters" and
"iommus").

I'll add a note to the commit message.

> 
> > Note that if both legacy bindings and generic bindings are present in
> > device tree, the legacy bindings are the ones that are used
> Can you oultine what guarantee that? Also what happen if some of devices are
> using the generic bindings while other are using the legacy one?

If both legacy bindings and generic bindings are present in device tree,
the legacy bindings are the ones that are used because mmu-masters is
parsed by xen/drivers/passthrough/arm/smmu.c:arm_smmu_device_dt_probe
which is called by arm_smmu_dt_init. It happens very early. iommus is
parsed by xen/drivers/passthrough/device_tree.c:iommu_add_dt_device
which is called by xen/arch/arm/domain_build.c:handle_device and happens
afterwards.

I'll add a note to the commit message.


> > 
> > Signed-off-by: Brian Woods <brian.woods@xilinx.com>
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xilinx.com>
> > Reviewed-by: Rahul Singh <rahul.singh@arm.com>
> > ---
> >   xen/drivers/passthrough/arm/smmu.c    | 42 ++++++++++++++++++++++++++-
> >   xen/drivers/passthrough/device_tree.c | 17 +----------
> >   2 files changed, 42 insertions(+), 17 deletions(-)
> > 
> > diff --git a/xen/drivers/passthrough/arm/smmu.c
> > b/xen/drivers/passthrough/arm/smmu.c
> > index f949c110ad..b564851a56 100644
> > --- a/xen/drivers/passthrough/arm/smmu.c
> > +++ b/xen/drivers/passthrough/arm/smmu.c
> > @@ -254,6 +254,8 @@ struct iommu_group
> >   	atomic_t ref;
> >   };
> >   +static struct arm_smmu_device *find_smmu(const struct device *dev);
> > +
> >   static struct iommu_group *iommu_group_alloc(void)
> >   {
> >   	struct iommu_group *group = xzalloc(struct iommu_group);
> > @@ -442,6 +444,8 @@ static struct iommu_group *iommu_group_get(struct device
> > *dev)
> >   #define SMR_VALID			(1U << 31)
> >   #define SMR_MASK_SHIFT			16
> >   #define SMR_ID_SHIFT			0
> > +#define SMR_ID_MASK			0x7fff
> > +#define SMR_MASK_MASK			0x7fff
> >     #define ARM_SMMU_GR0_S2CR(n)		(0xc00 + ((n) << 2))
> >   #define S2CR_CBNDX_SHIFT		0
> > @@ -872,6 +876,40 @@ static int register_smmu_master(struct arm_smmu_device
> > *smmu,
> >   					     fwspec);
> >   }
> >   +static int arm_smmu_dt_add_device_generic(u8 devfn, struct device *dev)
> > +{
> > +	struct arm_smmu_device *smmu;
> > +	struct iommu_fwspec *fwspec;
> > +
> > +	fwspec = dev_iommu_fwspec_get(dev);
> > +	if (fwspec == NULL)
> > +		return -ENXIO;
> > +
> > +	smmu = find_smmu(fwspec->iommu_dev);
> > +	if (smmu == NULL)
> > +		return -ENXIO;
> > +
> > +	return arm_smmu_dt_add_device_legacy(smmu, dev, fwspec);
> 
> Patch #2 seems to imply the code was reworked to have distinct path between
> legacy and generic. But now, you are calling the legacy code from the generic
> helper. This is pretty confusing, can you explain what's going on?

For the legacy path, arm_smmu_dt_add_device_legacy is called by
register_smmu_master scanning mmu-masters (a fwspec entry is also
created.) For the generic path, arm_smmu_dt_add_device_generic gets
called instead. Then, arm_smmu_dt_add_device_generic calls
arm_smmu_dt_add_device_legacy afterwards, shared with the legacy path.
This way most of the low level implementation is shared between the two
paths although the way they are called is distinct.

I'll add a note to the commit message.


> > +}
> > +
> > +static int arm_smmu_dt_xlate_generic(struct device *dev,
> > +				    const struct dt_phandle_args *spec)
> 
> This seems to be a verbatim copy from Linux. It would be good to mention it in
> the commit message. This would make easier to track any change.

Yes, I'll add a note to the commit message.


> > +{
> > +	uint32_t mask, fwid = 0;
> > +
> > +	if (spec->args_count > 0)
> > +		fwid |= (SMR_ID_MASK & spec->args[0]) << SMR_ID_SHIFT;
> > +
> > +	if (spec->args_count > 1)
> > +		fwid |= (SMR_MASK_MASK & spec->args[1]) << SMR_MASK_SHIFT;
> > +	else if (!of_property_read_u32(spec->np, "stream-match-mask", &mask))
> > +		fwid |= (SMR_MASK_MASK & mask) << SMR_MASK_SHIFT;
> > +
> > +	return iommu_fwspec_add_ids(dev,
> > +				    &fwid,
> > +				    1);
> 
> NIT: This feels a bit odd to read. Can't they be defined on the same line?

Fixed


> > +}
> > +
> >   static struct arm_smmu_device *find_smmu_for_device(struct device *dev)
> >   {
> >   	struct arm_smmu_device *smmu;
> > @@ -2836,6 +2874,7 @@ static void arm_smmu_iommu_domain_teardown(struct
> > domain *d)
> >   static const struct iommu_ops arm_smmu_iommu_ops = {
> >       .init = arm_smmu_iommu_domain_init,
> >       .hwdom_init = arm_smmu_iommu_hwdom_init,
> > +    .add_device = arm_smmu_dt_add_device_generic,
> >       .teardown = arm_smmu_iommu_domain_teardown,
> >       .iotlb_flush = arm_smmu_iotlb_flush,
> >       .iotlb_flush_all = arm_smmu_iotlb_flush_all,
> > @@ -2843,9 +2882,10 @@ static const struct iommu_ops arm_smmu_iommu_ops = {
> >       .reassign_device = arm_smmu_reassign_dev,
> >       .map_page = arm_iommu_map_page,
> >       .unmap_page = arm_iommu_unmap_page,
> > +    .dt_xlate = arm_smmu_dt_xlate_generic,
> >   };
> >   -static __init const struct arm_smmu_device *find_smmu(const struct device
> > *dev)
> > +static struct arm_smmu_device *find_smmu(const struct device *dev)
> >   {
> >   	struct arm_smmu_device *smmu;
> >   	bool found = false;
> > diff --git a/xen/drivers/passthrough/device_tree.c
> > b/xen/drivers/passthrough/device_tree.c
> > index a51ae3c9c3..ae07f272e1 100644
> > --- a/xen/drivers/passthrough/device_tree.c
> > +++ b/xen/drivers/passthrough/device_tree.c
> > @@ -162,22 +162,7 @@ int iommu_add_dt_device(struct dt_device_node *np)
> >            * these callback implemented.
> >            */
> >           if ( !ops->add_device || !ops->dt_xlate )
> > -        {
> > -            /*
> > -             * Some Device Trees may expose both legacy SMMU and generic
> > -             * IOMMU bindings together. However, the SMMU driver is only
> > -             * supporting the former and will protect them during the
> > -             * initialization. So we need to skip them and not return
> > -             * error here.
> > -             *
> > -             * XXX: This can be dropped when the SMMU is able to deal
> > -             * with generic bindings.
> > -             */
> > -            if ( dt_device_is_protected(np) )
> > -                return 0;
> > -            else
> > -                return -EINVAL;
> > -        }
> > +            return -EINVAL;
> >             if ( !dt_device_is_available(iommu_spec.np) )
> >               break;

[PATCH RESEND v3 1/3] arm,smmu: switch to using iommu_fwspec functions
[PATCH RESEND v3 2/3] arm,smmu: restructure code in preparation to new bindings support
[PATCH RESEND v3 3/3] arm,smmu: add support for generic DT bindings. Implement add_device and dt_xlate.