[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



On Thu, 24 Dec 2020 at 17:39, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> 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@xxxxxxxxxx>
> >>>
> >>> 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@xxxxxxxxxx>
> >>
> >> 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,



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.