Re: [PATCH v3 30/34] xen/riscv: add minimal stuff to processor.h to build full Xen

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Wed, 2024-01-24 at 09:19 +0100, Jan Beulich wrote:
> On 23.01.2024 18:08, Oleksii wrote:
> > On Tue, 2024-01-23 at 12:39 +0100, Jan Beulich wrote:
> > > On 22.12.2023 16:13, Oleksii Kurochko wrote:
> > > > Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
> > > > ---
> > > > Changes in V3:
> > > >  - Update the commit message
> > > 
> > > ??? (again)
> > The same as with previous. asm/processor.h was changed to
> > processor.h
> > 
> > > 
> > > > @@ -53,6 +56,18 @@ struct cpu_user_regs
> > > >      unsigned long pregs;
> > > >  };
> > > >  
> > > > +/* TODO: need to implement */
> > > > +#define cpu_to_core(cpu)   (0)
> > > > +#define cpu_to_socket(cpu) (0)
> > > > +
> > > > +static inline void cpu_relax(void)
> > > > +{
> > > > +    /* Encoding of the pause instruction */
> > > > +    __asm__ __volatile__ ( ".insn 0x100000F" );
> > > 
> > > binutils 2.40 knows "pause" - why use .insn then?
> > I thought that for this instruction it is needed to have extension
> > ZIHINTPAUSE ( according to Linux Kernel source code [1] ) and to
> > cover
> > older version.
> 
> Well, of course you'll need to enable the extension then for gas. But
> as long as you use the insn unconditionally, that's all fine and
> natural. Another thing would be if you meant to also run on systems
> not supporting the extension: Then the above use of .insn would need
> to become conditional anyway.
Then it makes sense to use "pause". 
Let's assume that for now we are running only on systems which support
the extension until we won't face compilation issue for some system.

> 
> > > > +    barrier();
> > > 
> > > Why?
> > Just to be aligned with Linux kernel implemetation from where this
> > function was taken.
> 
> Hmm, looking more closely we have an (open-coded) barrier even on
> x86.
> So I suppose it's really wanted (to keep the compiler from moving
> memory accesses around this construct), but then you may want to
> consider using
> 
>     __asm__ __volatile__ ( "pause" ::: "memory" );
> 
> here. First and foremost because at least in the general case two
> separate asm()s aren't the same as one combined one (volatile ones
> are more restricted, but I'd always err on the safe side, even if
> just to avoid giving bad examples which later on may be taken as a
> basis for deriving other code).
It makes sense, I'll update inline assembler line code.

Thanks.

~ Oleksii