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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 05.02.2024 16:32, Oleksii Kurochko wrote:
> --- a/xen/arch/riscv/Kconfig
> +++ b/xen/arch/riscv/Kconfig
> @@ -45,6 +45,13 @@ config RISCV_ISA_C
>  
>         If unsure, say Y.
>  
> +config TOOLCHAIN_HAS_ZIHINTPAUSE
> +     bool
> +     default y

Shorter as "def_bool y".

> +     depends on !64BIT || $(cc-option,-mabi=lp64 -march=rv64ima_zihintpause)
> +     depends on !32BIT || $(cc-option,-mabi=ilp32 -march=rv32ima_zihintpause)

So for a reason I cannot really see -mabi= is indeed required here,
or else the compiler sees an issue with the D extension. But enabling
both M and A shouldn't really be needed in this check, as being
unrelated?

> +     depends on LLD_VERSION >= 150000 || LD_VERSION >= 23600

What's the linker dependency here? Depending on the answer I might further
ask why "TOOLCHAIN" when elsewhere we use CC_HAS_ or HAS_CC_ or HAS_AS_.

That said, you may or may not be aware that personally I'm against
encoding such in Kconfig, and my repeated attempts to get the respective
discussion unstuck have not led anywhere. Therefore if you keep this, I'll
be in trouble whether to actually ack the change as a whole.

> --- 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)
> +
> +static inline void cpu_relax(void)
> +{
> +#ifdef __riscv_zihintpause
> +    /*
> +     * Reduce instruction retirement.
> +     * This assumes the PC changes.
> +     */
> +    __asm__ __volatile__ ("pause");
> +#else
> +    /* Encoding of the pause instruction */
> +    __asm__ __volatile__ (".insn 0x100000F");
> +#endif

Like elsewhere, nit: Missing blanks immediately inside the parentheses.

> +    barrier();

It's probably okay to be separate, but I'd suggest folding this right
into the asm()-s.

Jan