Signed-off-by: Oleksii Kurochko <oleksii.kurochko@gmail.com>
---
Changes in V4:
 - Change message -> subject in "Changes in V3"
 - Documentation about system requirement was added. In the future, it can be checked if the extension is supported
   by system __riscv_isa_extension_available() ( https://gitlab.com/xen-project/people/olkur/xen/-/commit/737998e89ed305eb92059300c374dfa53d2143fa )
 - update cpu_relax() function to check if __riscv_zihintpause is supported by a toolchain
 - add conditional _zihintpause to -march if it is supported by a toolchain
Changes in V3:
 - update the commit subject
 - rename get_processor_id to smp_processor_id
 - code style fixes
 - update the cpu_relax instruction: use pause instruction instead of div %0, %0, zero
---
Changes in V2:
 - Nothing changed. Only rebase.
---
 docs/misc/riscv/booting.txt            |  8 ++++++++
 xen/arch/riscv/Kconfig                 |  7 +++++++
 xen/arch/riscv/arch.mk                 |  1 +
 xen/arch/riscv/include/asm/processor.h | 23 +++++++++++++++++++++++
 4 files changed, 39 insertions(+)
 create mode 100644 docs/misc/riscv/booting.txt

diff --git a/docs/misc/riscv/booting.txt b/docs/misc/riscv/booting.txt
new file mode 100644
index 0000000000..38fad74956
--- /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.
diff --git a/xen/arch/riscv/Kconfig b/xen/arch/riscv/Kconfig
index f382b36f6c..383ce06771 100644
--- 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
+	depends on !64BIT || $(cc-option,-mabi=lp64 -march=rv64ima_zihintpause)
+	depends on !32BIT || $(cc-option,-mabi=ilp32 -march=rv32ima_zihintpause)
+	depends on LLD_VERSION >= 150000 || LD_VERSION >= 23600
+
 endmenu
 
 source "common/Kconfig"
diff --git a/xen/arch/riscv/arch.mk b/xen/arch/riscv/arch.mk
index 8403f96b6f..a4b53adaf7 100644
--- a/xen/arch/riscv/arch.mk
+++ b/xen/arch/riscv/arch.mk
@@ -7,6 +7,7 @@ CFLAGS-$(CONFIG_RISCV_64) += -mabi=lp64
 
 riscv-march-$(CONFIG_RISCV_ISA_RV64G) := rv64g
 riscv-march-$(CONFIG_RISCV_ISA_C)       := $(riscv-march-y)c
+riscv-march-$(CONFIG_TOOLCHAIN_HAS_ZIHINTPAUSE) := $(riscv-march-y)_zihintpause
 
 # Note that -mcmodel=medany is used so that Xen can be mapped
 # into the upper half _or_ the lower half of the address space.
diff --git a/xen/arch/riscv/include/asm/processor.h b/xen/arch/riscv/include/asm/processor.h
index 6db681d805..289dc35ea0 100644
--- 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
+
+    barrier();
+}
+
 static inline void wfi(void)
 {
     __asm__ __volatile__ ("wfi");
-- 
2.43.0

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

On Tue, 2024-02-13 at 14:33 +0100, Jan Beulich wrote:
> 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?
Agree, that M and A could be dropped.

Regarding -mabi my guess is because D extension can be emulated by
compiler, doesn't matter if D is set in -march.  If it is set then
hardware instruction will be used, otherwise emulated instruction will
be used.
And if D extenstion is always present it is need to know which ABI
should be used. If D extenstion has h/w support then -mabi should be
also update to lp64d instead of lp64.

> 
> > +	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_.
I missed to introduce {L}LLD_VERSION config. It should output from the
command:
  riscv64-linux-gnu-ld --version
> 
> 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.
Could I ask what is wrong with introduction of such things on KConfig?

Would it be better to put everything in riscv/arch.mk?

~ Oleksii

On 15.02.2024 17:38, Oleksii wrote:
> On Tue, 2024-02-13 at 14:33 +0100, Jan Beulich wrote:
>> On 05.02.2024 16:32, Oleksii Kurochko wrote:
>>> +	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_.
> I missed to introduce {L}LLD_VERSION config. It should output from the
> command:
>   riscv64-linux-gnu-ld --version

Doesn't answer my question though where the linker version matters
here.

>> 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.
> Could I ask what is wrong with introduction of such things on KConfig?

Just one of several possible pointers:
https://lists.xen.org/archives/html/xen-devel/2022-09/msg01793.html

> Would it be better to put everything in riscv/arch.mk?

Or a mix of both, as per the proposal. Just to be clear, if I say "yes"
to your question, someone else may come along and tell you to turn
around again.

Jan

On Thu, 2024-02-15 at 17:43 +0100, Jan Beulich wrote:
> On 15.02.2024 17:38, Oleksii wrote:
> > On Tue, 2024-02-13 at 14:33 +0100, Jan Beulich wrote:
> > > On 05.02.2024 16:32, Oleksii Kurochko wrote:
> > > > +	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_.
> > I missed to introduce {L}LLD_VERSION config. It should output from
> > the
> > command:
> >   riscv64-linux-gnu-ld --version
> 
> Doesn't answer my question though where the linker version matters
> here.
Then I misinterpreted your initial question.
Could you please provide further clarification or rephrase it for
better understanding?

Probably, your question was about why linker dependency is needed here,
then
it is not sufficient to check if a toolchain supports a particular  
extension without checking if the linker supports that extension   
too.
For example, Clang 15 supports Zihintpause but GNU bintutils
2.35.2 does not, leading build errors like so:
    
   riscv64-linux-gnu-ld: -march=rv64i_zihintpause2p0: Invalid or
   unknown z ISA extension: 'zihintpause'


~ Oleksii

On 16.02.2024 12:16, Oleksii wrote:
> On Thu, 2024-02-15 at 17:43 +0100, Jan Beulich wrote:
>> On 15.02.2024 17:38, Oleksii wrote:
>>> On Tue, 2024-02-13 at 14:33 +0100, Jan Beulich wrote:
>>>> On 05.02.2024 16:32, Oleksii Kurochko wrote:
>>>>> +	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_.
>>> I missed to introduce {L}LLD_VERSION config. It should output from
>>> the
>>> command:
>>>   riscv64-linux-gnu-ld --version
>>
>> Doesn't answer my question though where the linker version matters
>> here.
> Then I misinterpreted your initial question.
> Could you please provide further clarification or rephrase it for
> better understanding?
> 
> Probably, your question was about why linker dependency is needed here,
> then
> it is not sufficient to check if a toolchain supports a particular  
> extension without checking if the linker supports that extension   
> too.
> For example, Clang 15 supports Zihintpause but GNU bintutils
> 2.35.2 does not, leading build errors like so:
>     
>    riscv64-linux-gnu-ld: -march=rv64i_zihintpause2p0: Invalid or
>    unknown z ISA extension: 'zihintpause'

Hmm, that's certainly "interesting" behavior of the RISC-V linker. Yet
isn't the linker capability expected to be tied to that of gas? I would
find it far more natural if a gas dependency existed here. If such a
connection cannot be taken for granted, I'm pretty sure you'd need to
probe both then anyway.

Jan

On Mon, 2024-02-19 at 09:06 +0100, Jan Beulich wrote:
> On 16.02.2024 12:16, Oleksii wrote:
> > On Thu, 2024-02-15 at 17:43 +0100, Jan Beulich wrote:
> > > On 15.02.2024 17:38, Oleksii wrote:
> > > > On Tue, 2024-02-13 at 14:33 +0100, Jan Beulich wrote:
> > > > > On 05.02.2024 16:32, Oleksii Kurochko wrote:
> > > > > > +	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_.
> > > > I missed to introduce {L}LLD_VERSION config. It should output
> > > > from
> > > > the
> > > > command:
> > > >   riscv64-linux-gnu-ld --version
> > > 
> > > Doesn't answer my question though where the linker version
> > > matters
> > > here.
> > Then I misinterpreted your initial question.
> > Could you please provide further clarification or rephrase it for
> > better understanding?
> > 
> > Probably, your question was about why linker dependency is needed
> > here,
> > then
> > it is not sufficient to check if a toolchain supports a particular 
> > extension without checking if the linker supports that extension   
> > too.
> > For example, Clang 15 supports Zihintpause but GNU bintutils
> > 2.35.2 does not, leading build errors like so:
> >     
> >    riscv64-linux-gnu-ld: -march=rv64i_zihintpause2p0: Invalid or
> >    unknown z ISA extension: 'zihintpause'
> 
> Hmm, that's certainly "interesting" behavior of the RISC-V linker.
> Yet
> isn't the linker capability expected to be tied to that of gas? I
> would
> find it far more natural if a gas dependency existed here. If such a
> connection cannot be taken for granted, I'm pretty sure you'd need to
> probe both then anyway.

Wouldn't it be enough in this case instead of introducing of new
configs and etc, just to do the following:
   +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
   +
    riscv-march-$(CONFIG_RISCV_ISA_RV64G) := rv64g
    riscv-march-$(CONFIG_RISCV_ISA_C)       := $(riscv-march-y)c
   -riscv-march-$(CONFIG_TOOLCHAIN_HAS_ZIHINTPAUSE) := $(riscv-march-
   y)_zihintpause
    
    # Note that -mcmodel=medany is used so that Xen can be mapped
    # 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

Probably, it would be better:
   ...
   +CFLAGS += -march=$(riscv-march-y)$(call or,$(has_zihintpause)) -
   mstrict-align -
   mcmodel=medany


~ Oleksii

On 23.02.2024 18:00, Oleksii wrote:
> On Mon, 2024-02-19 at 09:06 +0100, Jan Beulich wrote:
>> On 16.02.2024 12:16, Oleksii wrote:
>>> On Thu, 2024-02-15 at 17:43 +0100, Jan Beulich wrote:
>>>> On 15.02.2024 17:38, Oleksii wrote:
>>>>> On Tue, 2024-02-13 at 14:33 +0100, Jan Beulich wrote:
>>>>>> On 05.02.2024 16:32, Oleksii Kurochko wrote:
>>>>>>> +	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_.
>>>>> I missed to introduce {L}LLD_VERSION config. It should output
>>>>> from
>>>>> the
>>>>> command:
>>>>>   riscv64-linux-gnu-ld --version
>>>>
>>>> Doesn't answer my question though where the linker version
>>>> matters
>>>> here.
>>> Then I misinterpreted your initial question.
>>> Could you please provide further clarification or rephrase it for
>>> better understanding?
>>>
>>> Probably, your question was about why linker dependency is needed
>>> here,
>>> then
>>> it is not sufficient to check if a toolchain supports a particular 
>>> extension without checking if the linker supports that extension   
>>> too.
>>> For example, Clang 15 supports Zihintpause but GNU bintutils
>>> 2.35.2 does not, leading build errors like so:
>>>     
>>>    riscv64-linux-gnu-ld: -march=rv64i_zihintpause2p0: Invalid or
>>>    unknown z ISA extension: 'zihintpause'
>>
>> Hmm, that's certainly "interesting" behavior of the RISC-V linker.
>> Yet
>> isn't the linker capability expected to be tied to that of gas? I
>> would
>> find it far more natural if a gas dependency existed here. If such a
>> connection cannot be taken for granted, I'm pretty sure you'd need to
>> probe both then anyway.
> 
> Wouldn't it be enough in this case instead of introducing of new
> configs and etc, just to do the following:
>    +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
>    +
>     riscv-march-$(CONFIG_RISCV_ISA_RV64G) := rv64g
>     riscv-march-$(CONFIG_RISCV_ISA_C)       := $(riscv-march-y)c
>    -riscv-march-$(CONFIG_TOOLCHAIN_HAS_ZIHINTPAUSE) := $(riscv-march-
>    y)_zihintpause
>     
>     # Note that -mcmodel=medany is used so that Xen can be mapped
>     # 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

Yes, this is kind of what I'd expect.

> Probably, it would be better:
>    ...
>    +CFLAGS += -march=$(riscv-march-y)$(call or,$(has_zihintpause)) -
>    mstrict-align -
>    mcmodel=medany

Why the use of "or"? IOW right now I don't see what's "better" here.

Jan