Re: [PATCH v13 08/10] xen/riscv: change .insn to .byte in cpu_relax()

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

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