| Message ID | 20201224164953.32357-1-julien@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/iommu: smmu: Rework how the SMMU version is detected | expand |
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
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 >
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
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,
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);