Re: [PATCH v5 18/23] xen/riscv: add minimal stuff to processor.h to build full Xen

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Tue, 2024-03-05 at 09:05 +0100, Jan Beulich wrote:
> On 26.02.2024 18:39, Oleksii Kurochko wrote:
> > --- /dev/null
> > +++ b/docs/misc/riscv/booting.txt
> > @@ -0,0 +1,8 @@
> > +System requirements
> > +===================
> > +
> > +The following extensions are expected to be supported by a system
> > on which
> > +Xen is run:
> > +- Zihintpause:
> > +  On a system that doesn't have this extension, cpu_relax() should
> > be
> > +  implemented properly. Otherwise, an illegal instruction
> > exception will arise.
> 
> This decision wants justifying in the (presently once again empty)
> description.
> 
> Furthermore - will there really be an illegal instruction exception
> otherwise?
> Isn't it the nature of hints that they are NOPs if not serving their
> designated
> purpose?
You are right, they are NOPs, so I will drop the part about an illegal
instruction exception.

> > --- a/xen/arch/riscv/arch.mk
> > +++ b/xen/arch/riscv/arch.mk
> > @@ -5,6 +5,12 @@ $(call cc-options-
> > add,CFLAGS,CC,$(EMBEDDED_EXTRA_CFLAGS))
> >  
> >  CFLAGS-$(CONFIG_RISCV_64) += -mabi=lp64
> >  
> > +ifeq ($(CONFIG_RISCV_64),y)
> > +has_zihintpause = $(call as-insn,$(CC) -mabi=lp64 -
> > march=rv64i_zihintpause, "pause",_zihintpause,)
> > +else
> > +has_zihintpause = $(call as-insn,$(CC) -mabi=ilp32 -
> > march=rv32i_zihintpause, "pause",_zihintpause,)
> > +endif
> 
> Considering that down the road likely more such tests will want
> adding, I think
> this wants further abstracting for the rv32/rv64 difference (ideally
> in a way
> that wouldn't make future RV128 wrongly and silently take the RV32
> branch).
> This would include eliminating the -mabi=lp64 redundancy with what's
> visible in
> context, perhaps by way of introducing a separate helper macro, e.g.
> 
> riscv-abi-$(CONFIG_RISCV_32) := -mabi=ilp32
> riscv-abi-$(CONFIG_RISCV_64) := -mabi=lp64
> 
> I further see nothing wrong with also using $(riscv-march-y) here.
> I.e.
> overall
> 
> _zihintpause := $(call as-insn,$(CC) $(riscv-abi-y) $(riscv-march-
> y)_zihintpause,"pause",_zihintpause)
> 
> (still with potential of abstracting further through another macro
> such
> that not every such construct would need to spell out the ABI and
> arch
> compiler options).
> 
> Plus a macro named has_* imo can be expected to expand to y or n. I
> would
> suggest to simply drop the "has", thus ...
> 
> > @@ -12,7 +18,7 @@ riscv-march-$(CONFIG_RISCV_ISA_C)       :=
> > $(riscv-march-y)c
> >  # into the upper half _or_ the lower half of the address space.
> >  # -mcmodel=medlow would force Xen into the lower half.
> >  
> > -CFLAGS += -march=$(riscv-march-y) -mstrict-align -mcmodel=medany
> > +CFLAGS += -march=$(riscv-march-y)$(has_zihintpause) -mstrict-align
> > -mcmodel=medany
> 
> ... also making the use site look 
> 
> > --- 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 smp_processor_id() 0
> > +
> >  /* On stack VCPU state */
> >  struct cpu_user_regs
> >  {
> > @@ -53,6 +56,26 @@ struct cpu_user_regs
> >      unsigned long pregs;
> >  };
> >  
> > +/* TODO: need to implement */
> > +#define cpu_to_core(cpu)   (0)
> > +#define cpu_to_socket(cpu) (0)
> 
> Nit: Like above in smp_processor_id() no need for parentheses here.
> 
> > +static inline void cpu_relax(void)
> > +{
> > +#ifdef __riscv_zihintpause
> > +    /*
> > +     * Reduce instruction retirement.
> > +     * This assumes the PC changes.
> 
> What is this 2nd sentence about?
cpu_relax() function was copied from Linux kernel and this comment
exists there, but I couldn't find in zihintpause spec how it affects PC
/IP, so it seems to me it can be dropped.

My guess that the 2nd sentece was added because of the following words
from the spec:
   The PAUSE instruction is a HINT that indicates the current hart’s
   rate of instruction retirement should be temporarily reduced or
   paused. The duration of its effect must be bounded and may be zero.
   
So it says reduced or pause, but still doesn't make sense as no matter
how long pause takes to complete, it will still advance PC.

> 
> > +     */
> > +    __asm__ __volatile__ ( "pause" );
> > +#else
> > +    /* Encoding of the pause instruction */
> > +    __asm__ __volatile__ ( ".insn 0x100000F" );
> 
> May I ask that you spell out the leading zero here, to make clear
> there
> aren't, by mistake, one to few zeroes in the middle?
I will add a leading zero. The encoding is correct, I've verified with
disassembler:
   c:   0100000f                pause    

~ Oleksii