[Xen-devel,06/12] xen/arm64: sysreg: Implement the 32-bit helpers using the 64-bit helpers

Message ID 20190327184531.30986-7-julien.grall@arm.com
State New
Headers show
Series
  • xen/arm: Add support to build with clang
Related show

Commit Message

Julien Grall March 27, 2019, 6:45 p.m.
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(-)

Comments

Andrew Cooper March 27, 2019, 7:15 p.m. | #1
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
Julien Grall March 27, 2019, 8:21 p.m. | #2
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
Stefano Stabellini April 17, 2019, 8:26 p.m. | #3
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
>

Patch

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;                                    \