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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

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