Re: [PATCH v2 33/39] xen/riscv: add minimal stuff to asm/processor.h to build full Xen

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Thu, 2023-12-14 at 17:04 +0100, Jan Beulich wrote:
> On 24.11.2023 11:30, Oleksii Kurochko wrote:
> > --- a/xen/arch/riscv/include/asm/processor.h
> > +++ b/xen/arch/riscv/include/asm/processor.h
> > @@ -12,6 +12,9 @@
> >  
> >  #ifndef __ASSEMBLY__
> >  
> > +/* TODO: need to be implemeted */
> > +#define get_processor_id() 0
> 
> Please don't re-introduce this - it was just recently dropped from
> the
> code base.
Thanks for remind me that. I'll drop it in the next version.

> 
> > @@ -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)
> 
> No need for leading underscores here.
Sure. Thanks.
> 
> > +static inline void cpu_relax(void)
> > +{
> > +   int dummy;
> > +   /* In lieu of a halt instruction, induce a long-latency
> > stall. */
> > +   __asm__ __volatile__ ("div %0, %0, zero" : "=r" (dummy));
> 
> Any reason for this, when Arm's is just barrier(), and apparently
> they got
> away with this quite fine? Also isn't this causing a division by
> zero,
> which I'd expect to cause some kind of exception? (Terminology-wise
> I'm of
> course biased by x86, where "halt instruction" wouldn't be suitable
> to use
> here. But if that terminology is fine on RISC-V, then obviously no
> objection.)
It was based on Linux kernel code:
https://elixir.bootlin.com/linux/latest/source/arch/riscv/include/asm/vdso/processor.h#L9

But looks I missed barrier()...
Probably it will be better update cpu_relax() to:

        /* Encoding of the pause instruction */
        __asm__ __volatile__ (".4byte 0x100000F");

        barrier();

~ Oleksii