| Message ID | 20190327184531.30986-7-julien.grall@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | xen/arm: Add support to build with clang | expand |
On 27/03/2019 18:45, Julien Grall wrote: > Clang is pickier than GCC for the register size in asm statement. It > expects the register size to match the value size. > > The instructions msr/mrs are expecting a 64-bit register. This means the > implementation of the 32-bit helpers is not correct. The easiest > solution is to implement the 32-bit helpers using the 64-bit helpers. > > Signed-off-by: Julien Grall <julien.grall@arm.com> (I don't have an opinion on how to fix the issue, but) Are SYSREGS actually always 64 bits even on arm32, and these helpers just a shorthand for the lower 32 bits, or is this an effectively-unnecessary constraint imposed by Clang? ~Andrew
Hi, On 27/03/2019 19:15, Andrew Cooper wrote: > On 27/03/2019 18:45, Julien Grall wrote: >> Clang is pickier than GCC for the register size in asm statement. It >> expects the register size to match the value size. >> >> The instructions msr/mrs are expecting a 64-bit register. This means the >> implementation of the 32-bit helpers is not correct. The easiest >> solution is to implement the 32-bit helpers using the 64-bit helpers. >> >> Signed-off-by: Julien Grall <julien.grall@arm.com> > > (I don't have an opinion on how to fix the issue, but) > > Are SYSREGS actually always 64 bits even on arm32, and these helpers > just a shorthand for the lower 32 bits, or is this an > effectively-unnecessary constraint imposed by Clang? This code is Arm64 specific. On Arm64, system registers are always 64-bits, and therefore the instructions msr/mrs require a 64-bit register. So the complain from Clang is valid here. It happens that some of the registers have the top 32-bit RES0 in current architecture. One could argue that this is not very future-proof and we should get rid of {READ, WRITE)_SYSREG32. Unfortunately, the callers are expecting a 32-bit value. I need to investigate all the callers to ensure no one is transforming the value to 64-bit again. I don't really want to block clang support on that, so I have added an action for fixing this later on. Cheers, -- Julien Grall
On Wed, 27 Mar 2019, Julien Grall wrote: > Clang is pickier than GCC for the register size in asm statement. It > expects the register size to match the value size. > > The instructions msr/mrs are expecting a 64-bit register. This means the > implementation of the 32-bit helpers is not correct. The easiest > solution is to implement the 32-bit helpers using the 64-bit helpers. > > Signed-off-by: Julien Grall <julien.grall@arm.com> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > --- > xen/include/asm-arm/arm64/sysregs.h | 11 +++-------- > 1 file changed, 3 insertions(+), 8 deletions(-) > > diff --git a/xen/include/asm-arm/arm64/sysregs.h b/xen/include/asm-arm/arm64/sysregs.h > index 08585a969e..c60029d38f 100644 > --- a/xen/include/asm-arm/arm64/sysregs.h > +++ b/xen/include/asm-arm/arm64/sysregs.h > @@ -59,14 +59,9 @@ > > /* Access to system registers */ > > -#define READ_SYSREG32(name) ({ \ > - uint32_t _r; \ > - asm volatile("mrs %0, "__stringify(name) : "=r" (_r)); \ > - _r; }) > -#define WRITE_SYSREG32(v, name) do { \ > - uint32_t _r = v; \ > - asm volatile("msr "__stringify(name)", %0" : : "r" (_r)); \ > -} while (0) > +#define READ_SYSREG32(name) ((uint32_t)READ_SYSREG64(name)) > + > +#define WRITE_SYSREG32(v, name) WRITE_SYSREG64((uint64_t)v, name) > > #define WRITE_SYSREG64(v, name) do { \ > uint64_t _r = v; \ > -- > 2.11.0 >
diff --git a/xen/include/asm-arm/arm64/sysregs.h b/xen/include/asm-arm/arm64/sysregs.h index 08585a969e..c60029d38f 100644 --- a/xen/include/asm-arm/arm64/sysregs.h +++ b/xen/include/asm-arm/arm64/sysregs.h @@ -59,14 +59,9 @@ /* Access to system registers */ -#define READ_SYSREG32(name) ({ \ - uint32_t _r; \ - asm volatile("mrs %0, "__stringify(name) : "=r" (_r)); \ - _r; }) -#define WRITE_SYSREG32(v, name) do { \ - uint32_t _r = v; \ - asm volatile("msr "__stringify(name)", %0" : : "r" (_r)); \ -} while (0) +#define READ_SYSREG32(name) ((uint32_t)READ_SYSREG64(name)) + +#define WRITE_SYSREG32(v, name) WRITE_SYSREG64((uint64_t)v, name) #define WRITE_SYSREG64(v, name) do { \ uint64_t _r = v; \
Clang is pickier than GCC for the register size in asm statement. It expects the register size to match the value size. The instructions msr/mrs are expecting a 64-bit register. This means the implementation of the 32-bit helpers is not correct. The easiest solution is to implement the 32-bit helpers using the 64-bit helpers. Signed-off-by: Julien Grall <julien.grall@arm.com> --- xen/include/asm-arm/arm64/sysregs.h | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)