| Message ID | c6ff49af9a107965f8121862e6b32c24548956e6.1717008161.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | *Enable build of full Xen for RISC-V | expand |
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 > + Older GCC and GNU Binutils would work, but this is not a guarantee. This sentence isn't appropriate to live here. The commit message saying "this is what we run in CI" is perfectly good enough. With this dropped, Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>. Can fix on commit.
On Thu, 2024-05-30 at 17:47 +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 > > + Older GCC and GNU Binutils would work, but this is not a > > guarantee. > > This sentence isn't appropriate to live here. > > The commit message saying "this is what we run in CI" is perfectly > good > enough. > > With this dropped, Reviewed-by: Andrew Cooper > <andrew.cooper3@citrix.com>. Can fix on commt. I am okay with dropping this sentence, but someone ( unfortunately I don't remember who Jan? Julien? ) requested it, and I think it would be nice to hear their opinion before doing so. ~ Oleksii
On 30/05/2024 6:16 pm, Oleksii K. wrote: > On Thu, 2024-05-30 at 17:47 +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 >>> + Older GCC and GNU Binutils would work, but this is not a >>> guarantee. >> This sentence isn't appropriate to live here. >> >> The commit message saying "this is what we run in CI" is perfectly >> good >> enough. >> >> With this dropped, Reviewed-by: Andrew Cooper >> <andrew.cooper3@citrix.com>. Can fix on commt. > I am okay with dropping this sentence, but someone ( unfortunately I > don't remember who Jan? Julien? ) requested it, and I think it would be > nice to hear their opinion before doing so. It's line noise, and literally a redundant statement. The same is true of every other line in the file, and we don't write it because that would be incredibly stupid. Anyone who wants to see why this was chosen can read the commit message. ~Andrew
Hi, On 30/05/2024 18:22, Andrew Cooper wrote: > On 30/05/2024 6:16 pm, Oleksii K. wrote: >> On Thu, 2024-05-30 at 17:47 +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 >>>> + Older GCC and GNU Binutils would work, but this is not a >>>> guarantee. >>> This sentence isn't appropriate to live here. >>> >>> The commit message saying "this is what we run in CI" is perfectly >>> good >>> enough. >>> >>> With this dropped, Reviewed-by: Andrew Cooper >>> <andrew.cooper3@citrix.com>. Can fix on commt. >> I am okay with dropping this sentence, but someone ( unfortunately I >> don't remember who Jan? Julien? ) requested it, and I think it would be >> nice to hear their opinion before doing so. I don't think it was me :). I have no issue with dropping the line. > > It's line noise, and literally a redundant statement. The same is true > of every other line in the file, +1. If anyone wanted extra clarify, we could spell out at the beginning of the file that this is the versions are confident with. Cheers,
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
On 30.05.2024 21:52, 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. See why I asked to amend the specified versions by a softening sentence that you (only now) said you dislike? The "this is what we use in CI" makes it a very random choice, entirely unrelated to the compiler's abilities. Jan
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
On 31/05/2024 7:18 am, Jan Beulich wrote: > On 30.05.2024 21:52, 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. > See why I asked to amend the specified versions by a softening sentence that > you (only now) said you dislike? The "this is what we use in CI" makes it a > very random choice, entirely unrelated to the compiler's abilities. "what's in CI" is an arbitrary choice, and that's *explicitly* fine and the right choice for Oleksii to have made. He's got the hard job of making the damn thing work in the first place. Requiring him to also go and get some old compilers to backdate the support statement is unreasonable for you to demand. In this case, I'm saying that it would be convenient for *me* if the numbers were older, because that's what *I* have and what *I'm* wanting to testing with. This means that I'm the one taking on the responsibility of playing backwards-compatibility-roulette. Now, for other reasons I no longer have those versions, but one of the 3 bugs I raises is still a real bug needing fixing. ~Andrew
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 + Older GCC and GNU Binutils would work, but this is not a guarantee. * POSIX compatible awk * Development install of zlib (e.g., zlib-dev) * Development install of Python 2.7 or later (e.g., python-dev)
This patch doesn't represent a strict lower bound for GCC and GNU Binutils; rather, these versions are specifically employed by the Xen RISC-V container and are anticipated to undergo continuous testing. Older GCC and GNU Binutils would work, but this is not a guarantee. While it is feasible to utilize Clang, it's important to note that, currently, there is no Xen RISC-V CI job in place to verify the seamless functioning of the build with Clang. Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> -- Changes in V5-V12: - Nothing changed. Only rebase. --- Changes in V6: - update the message in README. --- Changes in V5: - update the commit message and README file with additional explanation about GCC and GNU Binutils version. Additionally, it was added information about Clang. --- Changes in V4: - Update version of GCC (12.2) and GNU Binutils (2.39) to the version which are in Xen's contrainter for RISC-V --- Changes in V3: - new patch --- README | 4 ++++ 1 file changed, 4 insertions(+)