| Message ID | b5ccb3850cbfc0c84d2feea35a971351395fa974.1719319093.git.oleksii.kurochko@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Enable build of full Xen for RISC-V | expand |
On 25.06.2024 15:51, Oleksii Kurochko wrote: > The .insn directive appears to check that the byte pattern is a known > extension, where .4byte doesn't. Support for specifying "raw" insns was added only in 2.38. Moving away from .insn has other downsides (which may or may not matter here, but that would want discussing). But: Do we really need to move away? Can't you use .insn with operands that the older gas supports, e.g. .insn r MISC_MEM, 0, 0, x0, x0, x16 ? I'm sorry, the oldest RISC-V gas I have to hand is 2.39, so I couldn't double check that 2.35 would grok this. From checking sources it should, though. > The following compilation error occurs: > ./arch/riscv/include/asm/processor.h: Assembler messages: > ./arch/riscv/include/asm/processor.h:70: Error: unrecognized opcode `0x0100000F' > In case of the following Binutils: > $ riscv64-linux-gnu-as --version > GNU assembler (GNU Binutils for Debian) 2.35.2 In patch 6 you say 2.39. Why is 2.35.2 suddenly becoming of interest? > --- 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__ ( ".byte 0x0100000F" ); > #endif In the description you (correctly) say .4byte; why is it .byte here? Does this build at all? Jan
On Tue, 2024-06-25 at 16:45 +0200, Jan Beulich wrote: > On 25.06.2024 15:51, Oleksii Kurochko wrote: > > The .insn directive appears to check that the byte pattern is a > > known > > extension, where .4byte doesn't. > > Support for specifying "raw" insns was added only in 2.38. Moving > away > from .insn has other downsides (which may or may not matter here, but > that would want discussing). But: Do we really need to move away? > Can't > you use .insn with operands that the older gas supports, e.g. > > .insn r MISC_MEM, 0, 0, x0, x0, x16 > > ? I'm sorry, the oldest RISC-V gas I have to hand is 2.39, so I > couldn't > double check that 2.35 would grok this. From checking sources it > should, > though. We can do in this way too. I checked on https://godbolt.org/ even with RISC-V ( 64 bits ) gcc 8.2.0 the suggested option with .insn compiles well. > > > The following compilation error occurs: > > ./arch/riscv/include/asm/processor.h: Assembler messages: > > ./arch/riscv/include/asm/processor.h:70: Error: unrecognized > > opcode `0x0100000F' > > In case of the following Binutils: > > $ riscv64-linux-gnu-as --version > > GNU assembler (GNU Binutils for Debian) 2.35.2 > > In patch 6 you say 2.39. Why is 2.35.2 suddenly becoming of interest? Andrew has (or had) an older version of binutils and started to face the issues mentioned in this patch and the next one. As a result, some changes were suggested. The version in the README wasn't changed because, in my opinion, this requires a separate CI job with a prebuilt or fixed toolchain version. At the moment, it is supported only the version mentioned in README and that one I have on my machine. > > > --- 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__ ( ".byte 0x0100000F" ); > > #endif > > In the description you (correctly) say .4byte; why is it .byte here? > Does this build at all? Yes, it is a typo. Should be .4byte and it is built, but with warning: value 0x100000f truncated to 0xf ~ Oleksii
On 25.06.2024 20:09, Oleksii wrote: > On Tue, 2024-06-25 at 16:45 +0200, Jan Beulich wrote: >> On 25.06.2024 15:51, Oleksii Kurochko wrote: >>> The following compilation error occurs: >>> ./arch/riscv/include/asm/processor.h: Assembler messages: >>> ./arch/riscv/include/asm/processor.h:70: Error: unrecognized >>> opcode `0x0100000F' >>> In case of the following Binutils: >>> $ riscv64-linux-gnu-as --version >>> GNU assembler (GNU Binutils for Debian) 2.35.2 >> >> In patch 6 you say 2.39. Why is 2.35.2 suddenly becoming of interest? > Andrew has (or had) an older version of binutils and started to face > the issues mentioned in this patch and the next one. As a result, some > changes were suggested. > > The version in the README wasn't changed because, in my opinion, this > requires a separate CI job with a prebuilt or fixed toolchain version. > At the moment, it is supported only the version mentioned in README and > that one I have on my machine. So from my perspective, if you go to the lengths of making changes to support anything older than what you put into README, you will want to at least briefly mention why this is needed / wanted. Plus, as to "separate CI job": That makes little sense to me, or else we'd need to have separate jobs for each and every compiler version out in the world (and within range of what README says). Not just for RISC-V, but also for other architectures. This imo simply wouldn't scale. Jan
diff --git a/xen/arch/riscv/include/asm/processor.h b/xen/arch/riscv/include/asm/processor.h index 6846151717..0e75122efb 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__ ( ".byte 0x0100000F" ); #endif barrier();
The .insn directive appears to check that the byte pattern is a known extension, where .4byte doesn't. The following compilation error occurs: ./arch/riscv/include/asm/processor.h: Assembler messages: ./arch/riscv/include/asm/processor.h:70: Error: unrecognized opcode `0x0100000F' In case of the following Binutils: $ riscv64-linux-gnu-as --version GNU assembler (GNU Binutils for Debian) 2.35.2 Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com> --- Changes in V13: - new patch --- xen/arch/riscv/include/asm/processor.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)