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

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

> --- 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?

> +     */
> +    __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?

Jan