Re: [PATCH v4] xen/arm64: Add support Clang build on arm64

[Volodymyr Babchuk <Volodymyr_Babchuk@xxxxxxxx>]

Hi Saman,

Saman Dehghan <samaan.dehghan@xxxxxxxxx> writes:

> This patch enables building Xen for the arm64 using the Clang/LLVM compiler.
> Changes include:
> - Add explicit -march=armv8 flag for arm64 builds.
> - Introduce `READ_FP_SYSREG` and `WRITE_FP_SYSREG` to encapsulate the required
>   `.arch_extension fp` directive for system fp register access.
> - Add ".arch_extension fp" to the inline assembly for `save_state` and
>   `restore_state`.
>

It breaks GCC-based build (see below)

Also, are you going to address to other issues that I tried to
tackle in

https://patchew.org/Xen/20241129014850.2852844-1-volodymyr._5Fbabchuk@xxxxxxxx/

?

> Signed-off-by: Saman Dehghan <samaan.dehghan@xxxxxxxxx>
> ---
>  README                   |  2 ++
>  xen/arch/arm/arch.mk     |  1 +
>  xen/arch/arm/arm64/vfp.c | 34 ++++++++++++++++++++++++++--------
>  3 files changed, 29 insertions(+), 8 deletions(-)
>
> diff --git a/README b/README
> index 889a4ea906..67c1aa7fe6 100644
> --- a/README
> +++ b/README
> @@ -45,6 +45,8 @@ provided by your OS distributor:
>        - For ARM:
>          - GCC 5.1 or later
>          - GNU Binutils 2.25 or later
> +        or
> +        - Clang/LLVM 11 or later
>        - For RISC-V 64-bit:
>          - GCC 12.2 or later
>          - GNU Binutils 2.39 or later
> diff --git a/xen/arch/arm/arch.mk b/xen/arch/arm/arch.mk
> index 9c4bedfb3b..bcf548069b 100644
> --- a/xen/arch/arm/arch.mk
> +++ b/xen/arch/arm/arch.mk
> @@ -13,6 +13,7 @@ ifeq ($(CONFIG_MPU),y)
>  CFLAGS-$(CONFIG_ARM_64) += -march=armv8-r
>  else
>  CFLAGS-$(CONFIG_ARM_64) += -mcpu=generic
> +CFLAGS-$(CONFIG_ARM_64) += -march=armv8

This breaks build for me:

cc1: error: unknown value 'armv8' for '-march'
cc1: note: valid arguments are: armv8-a armv8.1-a armv8.2-a armv8.3-a armv8.4-a 
armv8.5-a armv8.6-a armv8.7-a armv8.8-a armv8.9-a armv8-r armv9-a armv9.1-a 
armv9.2-a armv9.3-a armv9.4-a armv9.5-a; did you mean 'armv8-a'?


>  endif
>  CFLAGS-$(CONFIG_ARM_64) += -mgeneral-regs-only # No fp registers etc
>  $(call cc-option-add,CFLAGS-$(CONFIG_ARM_64),CC,-mno-outline-atomics)
> diff --git a/xen/arch/arm/arm64/vfp.c b/xen/arch/arm/arm64/vfp.c
> index c4f89c7b0e..cd5c97cfd0 100644
> --- a/xen/arch/arm/arm64/vfp.c
> +++ b/xen/arch/arm/arm64/vfp.c
> @@ -6,7 +6,8 @@
>  
>  static inline void save_state(uint64_t *fpregs)
>  {
> -    asm volatile("stp q0, q1, [%1, #16 * 0]\n\t"
> +    asm volatile(".arch_extension fp\n\t"
> +                 "stp q0, q1, [%1, #16 * 0]\n\t"
>                   "stp q2, q3, [%1, #16 * 2]\n\t"
>                   "stp q4, q5, [%1, #16 * 4]\n\t"
>                   "stp q6, q7, [%1, #16 * 6]\n\t"
> @@ -22,12 +23,14 @@ static inline void save_state(uint64_t *fpregs)
>                   "stp q26, q27, [%1, #16 * 26]\n\t"
>                   "stp q28, q29, [%1, #16 * 28]\n\t"
>                   "stp q30, q31, [%1, #16 * 30]\n\t"
> +                 ".arch_extension nofp\n\t"
>                   : "=Q" (*fpregs) : "r" (fpregs));
>  }
>  
>  static inline void restore_state(const uint64_t *fpregs)
>  {
> -    asm volatile("ldp q0, q1, [%1, #16 * 0]\n\t"
> +    asm volatile(".arch_extension fp\n\t"
> +                 "ldp q0, q1, [%1, #16 * 0]\n\t"
>                   "ldp q2, q3, [%1, #16 * 2]\n\t"
>                   "ldp q4, q5, [%1, #16 * 4]\n\t"
>                   "ldp q6, q7, [%1, #16 * 6]\n\t"
> @@ -43,9 +46,24 @@ static inline void restore_state(const uint64_t *fpregs)
>                   "ldp q26, q27, [%1, #16 * 26]\n\t"
>                   "ldp q28, q29, [%1, #16 * 28]\n\t"
>                   "ldp q30, q31, [%1, #16 * 30]\n\t"
> +                 ".arch_extension nofp\n\t"
>                   : : "Q" (*fpregs), "r" (fpregs));
>  }
>  
> +#define WRITE_FP_SYSREG(v, name) do {                   \
> +     uint64_t _r = (v);                                 \
> +     asm volatile(".arch_extension fp\n\t"              \
> +                  "msr "__stringify(name)", %0\n\t"     \
> +                  ".arch_extension nofp" : : "r" (_r)); \
> +} while (0)
> +
> +#define READ_FP_SYSREG(name) ({                         \
> +     uint64_t _r;                                       \
> +     asm volatile(".arch_extension fp\n\t"              \
> +                  "mrs  %0, "__stringify(name)"\n\t"    \
> +                  ".arch_extension nofp" : "=r" (_r));  \
> +_r; })
> +
>  void vfp_save_state(struct vcpu *v)
>  {
>      if ( !cpu_has_fp )
> @@ -56,10 +74,10 @@ void vfp_save_state(struct vcpu *v)
>      else
>          save_state(v->arch.vfp.fpregs);
>  
> -    v->arch.vfp.fpsr = READ_SYSREG(FPSR);
> -    v->arch.vfp.fpcr = READ_SYSREG(FPCR);
> +    v->arch.vfp.fpsr = READ_FP_SYSREG(FPSR);
> +    v->arch.vfp.fpcr = READ_FP_SYSREG(FPCR);
>      if ( is_32bit_domain(v->domain) )
> -        v->arch.vfp.fpexc32_el2 = READ_SYSREG(FPEXC32_EL2);
> +        v->arch.vfp.fpexc32_el2 = READ_FP_SYSREG(FPEXC32_EL2);
>  }
>  
>  void vfp_restore_state(struct vcpu *v)
> @@ -72,8 +90,8 @@ void vfp_restore_state(struct vcpu *v)
>      else
>          restore_state(v->arch.vfp.fpregs);
>  
> -    WRITE_SYSREG(v->arch.vfp.fpsr, FPSR);
> -    WRITE_SYSREG(v->arch.vfp.fpcr, FPCR);
> +    WRITE_FP_SYSREG(v->arch.vfp.fpsr, FPSR);
> +    WRITE_FP_SYSREG(v->arch.vfp.fpcr, FPCR);
>      if ( is_32bit_domain(v->domain) )
> -        WRITE_SYSREG(v->arch.vfp.fpexc32_el2, FPEXC32_EL2);
> +        WRITE_FP_SYSREG(v->arch.vfp.fpexc32_el2, FPEXC32_EL2);
>  }

-- 
WBR, Volodymyr