Re: [PATCH v12 8/8] xen/README: add compiler and binutils versions for RISC-V64

["Oleksii K." <oleksii.kurochko@xxxxxxxxx>]

On Thu, 2024-05-30 at 20:52 +0100, Andrew Cooper wrote:
> On 29/05/2024 8:55 pm, Oleksii Kurochko wrote:
> > diff --git a/README b/README
> > index c8a108449e..30da5ff9c0 100644
> > --- a/README
> > +++ b/README
> > @@ -48,6 +48,10 @@ provided by your OS distributor:
> >        - For ARM 64-bit:
> >          - GCC 5.1 or later
> >          - GNU Binutils 2.24 or later
> > +      - For RISC-V 64-bit:
> > +        - GCC 12.2 or later
> > +        - GNU Binutils 2.39 or later
> 
> I would like to petition for GCC 10 and Binutils 2.35.
> 
> These are the versions provided as cross-compilers by Debian, and
> therefore are the versions I would prefer to do smoke testing with.
We can consider that, but I prefer to make a separate patch series for
that.

~ Oleksii

> 
> One issue is in cpu_relax(), needing this diff to fix:
> 
> diff --git a/xen/arch/riscv/include/asm/processor.h
> b/xen/arch/riscv/include/asm/processor.h
> index 6846151717f7..830a05dd8e3a 100644
> --- a/xen/arch/riscv/include/asm/processor.h
> +++ b/xen/arch/riscv/include/asm/processor.h
> @@ -67,7 +67,7 @@ static inline void cpu_relax(void)
>      __asm__ __volatile__ ( "pause" );
>  #else
>      /* Encoding of the pause instruction */
> -    __asm__ __volatile__ ( ".insn 0x0100000F" );
> +    __asm__ __volatile__ ( ".4byte 0x0100000F" );
>  #endif
>  
>      barrier();
> 
> The .insn directive appears to check that the byte pattern is a known
> extension, where .4byte doesn't.  AFAICT, this makes .insn pretty
> useless for what I'd expect is it's main purpose...
> 
> 
> The other problem is a real issue in cmpxchg.h, already committed to
> staging (51dabd6312c).
> 
> RISC-V does a conditional toolchain for the Zbb extension
> (xen/arch/riscv/rules.mk), but unconditionally uses the ANDN
> instruction
> in emulate_xchg_1_2().
> 
> Nevertheless, this is also quite easy to fix:
> 
> diff --git a/xen/arch/riscv/include/asm/cmpxchg.h
> b/xen/arch/riscv/include/asm/cmpxchg.h
> index d5e678c03678..12ecb0950701 100644
> --- a/xen/arch/riscv/include/asm/cmpxchg.h
> +++ b/xen/arch/riscv/include/asm/cmpxchg.h
> @@ -18,6 +18,20 @@
>          : "r" (new) \
>          : "memory" );
>  
> +/*
> + * Binutils < 2.37 doesn't understand ANDN.  If the toolchain is too
> old, form
> + * it of a NOT+AND pair
> + */
> +#ifdef __riscv_zbb
> +#define ANDN_INSN(rd, rs1, rs2)                 \
> +    "andn " rd ", " rs1 ", " rs2 "\n"
> +#else
> +#define ANDN_INSN(rd, rs1, rs2)                 \
> +    "not " rd ", " rs2 "\n"                     \
> +    "and " rd ", " rs1 ", " rd "\n"
> +#endif
> +
> +
>  /*
>   * For LR and SC, the A extension requires that the address held in
> rs1 be
>   * naturally aligned to the size of the operand (i.e., eight-byte
> aligned
> @@ -48,7 +62,7 @@
>      \
>      asm volatile ( \
>          "0: lr.w" lr_sfx " %[old], %[ptr_]\n" \
> -        "   andn  %[scratch], %[old], %[mask]\n" \
> +        ANDN_INSN("%[scratch]", "%[old]", "%[mask]") \
>          "   or   %[scratch], %[scratch], %z[new_]\n" \
>          "   sc.w" sc_sfx " %[scratch], %[scratch], %[ptr_]\n" \
>          "   bnez %[scratch], 0b\n" \
> 
> 
> 
> And with that, everything builds... but doesn't link.  I've got this:
> 
>   LDS     arch/riscv/xen.lds
> riscv64-linux-gnu-ld      -T arch/riscv/xen.lds -N prelink.o \
>     ./common/symbols-dummy.o -o ./.xen-syms.0
> riscv64-linux-gnu-ld: prelink.o: in function
> `keyhandler_crash_action':
> /local/xen.git/xen/common/keyhandler.c:552: undefined reference to
> `guest_physmap_remove_page'
> riscv64-linux-gnu-ld: ./.xen-syms.0: hidden symbol
> `guest_physmap_remove_page' isn't defined
> riscv64-linux-gnu-ld: final link failed: bad value
> 
> which is completely bizarre.
> 
> keyhandler_crash_action() has no actual reference to
> guest_physmap_remove_page(), and keyhandler.o has no such relocation.
> 
> I'm still investigating this one.
> 
> ~Andrew