[PATCH] xen/iommu: smmu: Rework how the SMMU version is detected

Julien Grall posted 1 patch 3 years, 3 months ago
Test gitlab-ci passed
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20201224164953.32357-1-julien@xen.org
xen/drivers/passthrough/arm/smmu.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
[PATCH] xen/iommu: smmu: Rework how the SMMU version is detected
Posted by Julien Grall 3 years, 3 months ago
From: Julien Grall <jgrall@amazon.com>

Clang 11 will throw the following error:

smmu.c:2284:18: error: cast to smaller integer type 'enum arm_smmu_arch_version' from 'const void *' [-Werror,-Wvoid-pointer-to-enum-cast]
        smmu->version = (enum arm_smmu_arch_version)of_id->data;
                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

The error can be prevented by introduce static variable for each SMMU
version and store a pointer for each of them.

Signed-off-by: Julien Grall <jgrall@amazon.com>

---

Only build tested
---
 xen/drivers/passthrough/arm/smmu.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/xen/drivers/passthrough/arm/smmu.c b/xen/drivers/passthrough/arm/smmu.c
index ed04d85e05e9..4a507d7aff64 100644
--- a/xen/drivers/passthrough/arm/smmu.c
+++ b/xen/drivers/passthrough/arm/smmu.c
@@ -2249,12 +2249,15 @@ static int arm_smmu_device_cfg_probe(struct arm_smmu_device *smmu)
 	return 0;
 }
 
+static const enum arm_smmu_arch_version smmu_v1_version = ARM_SMMU_V1;
+static const enum arm_smmu_arch_version smmu_v2_version = ARM_SMMU_V2;
+
 static const struct of_device_id arm_smmu_of_match[] = {
-	{ .compatible = "arm,smmu-v1", .data = (void *)ARM_SMMU_V1 },
-	{ .compatible = "arm,smmu-v2", .data = (void *)ARM_SMMU_V2 },
-	{ .compatible = "arm,mmu-400", .data = (void *)ARM_SMMU_V1 },
-	{ .compatible = "arm,mmu-401", .data = (void *)ARM_SMMU_V1 },
-	{ .compatible = "arm,mmu-500", .data = (void *)ARM_SMMU_V2 },
+	{ .compatible = "arm,smmu-v1", .data = &smmu_v1_version },
+	{ .compatible = "arm,smmu-v2", .data = &smmu_v2_version },
+	{ .compatible = "arm,mmu-400", .data = &smmu_v1_version },
+	{ .compatible = "arm,mmu-401", .data = &smmu_v1_version },
+	{ .compatible = "arm,mmu-500", .data = &smmu_v2_version },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, arm_smmu_of_match);
@@ -2281,7 +2284,7 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)
 	smmu->dev = dev;
 
 	of_id = of_match_node(arm_smmu_of_match, dev->of_node);
-	smmu->version = (enum arm_smmu_arch_version)of_id->data;
+	smmu->version = *(enum arm_smmu_arch_version *)of_id->data;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	smmu->base = devm_ioremap_resource(dev, res);
-- 
2.17.1


Re: [PATCH] xen/iommu: smmu: Rework how the SMMU version is detected
Posted by Andrew Cooper 3 years, 3 months ago
On 24/12/2020 16:49, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
>
> Clang 11 will throw the following error:
>
> smmu.c:2284:18: error: cast to smaller integer type 'enum arm_smmu_arch_version' from 'const void *' [-Werror,-Wvoid-pointer-to-enum-cast]
>         smmu->version = (enum arm_smmu_arch_version)of_id->data;
>                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>
> The error can be prevented by introduce static variable for each SMMU
> version and store a pointer for each of them.
>
> Signed-off-by: Julien Grall <jgrall@amazon.com>

You can also fix this by casting through (uintptr_t) instead of (enum
arm_smmu_arch_version), which wouldn't involve an extra indirection.

Alternatively, you could modify dt_device_match to union void *data with
uintptr_t val for when you want to actually pass non-pointer data.

~Andrew

Re: [PATCH] xen/iommu: smmu: Rework how the SMMU version is detected
Posted by Julien Grall 3 years, 3 months ago

On 24/12/2020 17:00, Andrew Cooper wrote:
> On 24/12/2020 16:49, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> Clang 11 will throw the following error:
>>
>> smmu.c:2284:18: error: cast to smaller integer type 'enum arm_smmu_arch_version' from 'const void *' [-Werror,-Wvoid-pointer-to-enum-cast]
>>          smmu->version = (enum arm_smmu_arch_version)of_id->data;
>>                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>
>> The error can be prevented by introduce static variable for each SMMU
>> version and store a pointer for each of them.
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> You can also fix this by casting through (uintptr_t) instead of (enum
> arm_smmu_arch_version), which wouldn't involve an extra indirection.

I thought about using a different cast, but it feels just papering over 
the issue.

But I don't see what's the problem with the extra indirection... It is 
self-contained and only used during the IOMMU initialization.

> 
> Alternatively, you could modify dt_device_match to union void *data with
> uintptr_t val for when you want to actually pass non-pointer data.
>
> ~Andrew
> 

-- 
Julien Grall

Re: [PATCH] xen/iommu: smmu: Rework how the SMMU version is detected
Posted by Andrew Cooper 3 years, 3 months ago
On 24/12/2020 17:10, Julien Grall wrote:
>
>
> On 24/12/2020 17:00, Andrew Cooper wrote:
>> On 24/12/2020 16:49, Julien Grall wrote:
>>> From: Julien Grall <jgrall@amazon.com>
>>>
>>> Clang 11 will throw the following error:
>>>
>>> smmu.c:2284:18: error: cast to smaller integer type 'enum
>>> arm_smmu_arch_version' from 'const void *'
>>> [-Werror,-Wvoid-pointer-to-enum-cast]
>>>          smmu->version = (enum arm_smmu_arch_version)of_id->data;
>>>                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>
>>> The error can be prevented by introduce static variable for each SMMU
>>> version and store a pointer for each of them.
>>>
>>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>>
>> You can also fix this by casting through (uintptr_t) instead of (enum
>> arm_smmu_arch_version), which wouldn't involve an extra indirection.
>
> I thought about using a different cast, but it feels just papering
> over the issue.
>
> But I don't see what's the problem with the extra indirection... It is
> self-contained and only used during the IOMMU initialization.

Well - its extra .rodata for the sake of satisfying the compiler in a
specific way.

Irrespective, all of this looks like it ought to be initdata rather than
runtime data, which is probably a bigger deal than worrying about one
extra pointer access.

~Andrew

Re: [PATCH] xen/iommu: smmu: Rework how the SMMU version is detected
Posted by Julien Grall 3 years, 3 months ago
On Thu, 24 Dec 2020 at 17:39, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>
> On 24/12/2020 17:10, Julien Grall wrote:
> >
> >
> > On 24/12/2020 17:00, Andrew Cooper wrote:
> >> On 24/12/2020 16:49, Julien Grall wrote:
> >>> From: Julien Grall <jgrall@amazon.com>
> >>>
> >>> Clang 11 will throw the following error:
> >>>
> >>> smmu.c:2284:18: error: cast to smaller integer type 'enum
> >>> arm_smmu_arch_version' from 'const void *'
> >>> [-Werror,-Wvoid-pointer-to-enum-cast]
> >>>          smmu->version = (enum arm_smmu_arch_version)of_id->data;
> >>>                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >>>
> >>> The error can be prevented by introduce static variable for each SMMU
> >>> version and store a pointer for each of them.
> >>>
> >>> Signed-off-by: Julien Grall <jgrall@amazon.com>
> >>
> >> You can also fix this by casting through (uintptr_t) instead of (enum
> >> arm_smmu_arch_version), which wouldn't involve an extra indirection.
> >
> > I thought about using a different cast, but it feels just papering
> > over the issue.
> >
> > But I don't see what's the problem with the extra indirection... It is
> > self-contained and only used during the IOMMU initialization.
>
> Well - its extra .rodata for the sake of satisfying the compiler in a
> specific way.

You are making the assumption that I wrote this way only to make the compiler
happy. :)

While the patch was originally written because of the compiler, we will need to
introduce some workaround. So the enum is going to be transformed to a
structure.

I thought about introducing the structure now, but I felt it was more
controversial
than this approach.

>
> Irrespective, all of this looks like it ought to be initdata rather than
> runtime data, which is probably a bigger deal than worrying about one
> extra pointer access.

I thought about that. However, not all the users are in __init yet. So this
would technically be a layer violation (runtime function should not
use init variable).

In practice, I think all the users could be in init. I will check and send a
patch.

Cheers,