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
