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(-)

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();
-- 
2.45.2

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