From: Aleksa Paunovic <aleksa.paunovic@htecgroup.com>
Use the hwprobe syscall to decide which PAUSE instruction to execute in
userspace code.
Signed-off-by: Aleksa Paunovic <aleksa.paunovic@htecgroup.com>
---
tools/arch/riscv/include/asm/vdso/processor.h | 27 +++++++++++++++++----------
1 file changed, 17 insertions(+), 10 deletions(-)
diff --git a/tools/arch/riscv/include/asm/vdso/processor.h b/tools/arch/riscv/include/asm/vdso/processor.h
index 662aca03984817f9c69186658b19e9dad9e4771c..027219a486b7b93814888190f8224af29498707c 100644
--- a/tools/arch/riscv/include/asm/vdso/processor.h
+++ b/tools/arch/riscv/include/asm/vdso/processor.h
@@ -4,26 +4,33 @@
#ifndef __ASSEMBLY__
+#include <asm/hwprobe.h>
+#include <sys/hwprobe.h>
+#include <asm/vendor/mips.h>
#include <asm-generic/barrier.h>
static inline void cpu_relax(void)
{
+ struct riscv_hwprobe pair;
+ bool has_mipspause;
#ifdef __riscv_muldiv
int dummy;
/* In lieu of a halt instruction, induce a long-latency stall. */
__asm__ __volatile__ ("div %0, %0, zero" : "=r" (dummy));
#endif
-#ifdef CONFIG_TOOLCHAIN_HAS_ZIHINTPAUSE
- /*
- * Reduce instruction retirement.
- * This assumes the PC changes.
- */
- __asm__ __volatile__ ("pause");
-#else
- /* Encoding of the pause instruction */
- __asm__ __volatile__ (".4byte 0x100000F");
-#endif
+ pair.key = RISCV_HWPROBE_KEY_VENDOR_EXT_MIPS_0;
+ __riscv_hwprobe(&pair, 1, 0, NULL, 0);
+ has_mipspause = pair.value & RISCV_HWPROBE_VENDOR_EXT_XMIPSEXECTL;
+
+ if (has_mipspause) {
+ /* Encoding of the mips pause instruction */
+ __asm__ __volatile__(".4byte 0x00501013");
+ } else {
+ /* Encoding of the pause instruction */
+ __asm__ __volatile__(".4byte 0x100000F");
+ }
+
barrier();
}
--
2.34.1On 6/25/25 16:21, Aleksa Paunovic via B4 Relay wrote:
> From: Aleksa Paunovic <aleksa.paunovic@htecgroup.com>
>
> Use the hwprobe syscall to decide which PAUSE instruction to execute in
> userspace code.
>
> Signed-off-by: Aleksa Paunovic <aleksa.paunovic@htecgroup.com>
> ---
> tools/arch/riscv/include/asm/vdso/processor.h | 27 +++++++++++++++++----------
> 1 file changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/tools/arch/riscv/include/asm/vdso/processor.h b/tools/arch/riscv/include/asm/vdso/processor.h
> index 662aca03984817f9c69186658b19e9dad9e4771c..027219a486b7b93814888190f8224af29498707c 100644
> --- a/tools/arch/riscv/include/asm/vdso/processor.h
> +++ b/tools/arch/riscv/include/asm/vdso/processor.h
> @@ -4,26 +4,33 @@
>
> #ifndef __ASSEMBLY__
>
> +#include <asm/hwprobe.h>
> +#include <sys/hwprobe.h>
> +#include <asm/vendor/mips.h>
> #include <asm-generic/barrier.h>
>
> static inline void cpu_relax(void)
> {
> + struct riscv_hwprobe pair;
> + bool has_mipspause;
> #ifdef __riscv_muldiv
> int dummy;
> /* In lieu of a halt instruction, induce a long-latency stall. */
> __asm__ __volatile__ ("div %0, %0, zero" : "=r" (dummy));
> #endif
>
> -#ifdef CONFIG_TOOLCHAIN_HAS_ZIHINTPAUSE
> - /*
> - * Reduce instruction retirement.
> - * This assumes the PC changes.
> - */
> - __asm__ __volatile__ ("pause");
> -#else
> - /* Encoding of the pause instruction */
> - __asm__ __volatile__ (".4byte 0x100000F");
> -#endif
> + pair.key = RISCV_HWPROBE_KEY_VENDOR_EXT_MIPS_0;
> + __riscv_hwprobe(&pair, 1, 0, NULL, 0);
So this should not trigger a syscall, so even if it's weird, I guess
that's ok.
Another solution that was already suggested for CFI would be to
implement VDSO alternatives, we could easily parse the VDSO elf and
patch it at boot time, I'm pretty sure that will be useful at some point.
> + has_mipspause = pair.value & RISCV_HWPROBE_VENDOR_EXT_XMIPSEXECTL;
> +
> + if (has_mipspause) {
> + /* Encoding of the mips pause instruction */
> + __asm__ __volatile__(".4byte 0x00501013");
Here you could have used the MIPS_PAUSE introduced earlier.
> + } else {
> + /* Encoding of the pause instruction */
> + __asm__ __volatile__(".4byte 0x100000F");
> + }
> +
> barrier();
> }
>
>
Anyway, let's merge this for now:
Reviewed-by: Alexandre Ghiti <alexghiti@rivosinc.com>
Thanks,
Alex
On Wed, Jun 25, 2025 at 04:21:01PM +0200, Aleksa Paunovic via B4 Relay wrote:
> From: Aleksa Paunovic <aleksa.paunovic@htecgroup.com>
>
> Use the hwprobe syscall to decide which PAUSE instruction to execute in
> userspace code.
>
> Signed-off-by: Aleksa Paunovic <aleksa.paunovic@htecgroup.com>
> ---
> tools/arch/riscv/include/asm/vdso/processor.h | 27 +++++++++++++++++----------
> 1 file changed, 17 insertions(+), 10 deletions(-)
>
> diff --git a/tools/arch/riscv/include/asm/vdso/processor.h b/tools/arch/riscv/include/asm/vdso/processor.h
> index 662aca03984817f9c69186658b19e9dad9e4771c..027219a486b7b93814888190f8224af29498707c 100644
> --- a/tools/arch/riscv/include/asm/vdso/processor.h
> +++ b/tools/arch/riscv/include/asm/vdso/processor.h
> @@ -4,26 +4,33 @@
>
> #ifndef __ASSEMBLY__
>
> +#include <asm/hwprobe.h>
> +#include <sys/hwprobe.h>
> +#include <asm/vendor/mips.h>
> #include <asm-generic/barrier.h>
>
> static inline void cpu_relax(void)
> {
> + struct riscv_hwprobe pair;
> + bool has_mipspause;
> #ifdef __riscv_muldiv
> int dummy;
> /* In lieu of a halt instruction, induce a long-latency stall. */
> __asm__ __volatile__ ("div %0, %0, zero" : "=r" (dummy));
> #endif
>
> -#ifdef CONFIG_TOOLCHAIN_HAS_ZIHINTPAUSE
> - /*
> - * Reduce instruction retirement.
> - * This assumes the PC changes.
> - */
> - __asm__ __volatile__ ("pause");
> -#else
> - /* Encoding of the pause instruction */
> - __asm__ __volatile__ (".4byte 0x100000F");
> -#endif
> + pair.key = RISCV_HWPROBE_KEY_VENDOR_EXT_MIPS_0;
> + __riscv_hwprobe(&pair, 1, 0, NULL, 0);
> + has_mipspause = pair.value & RISCV_HWPROBE_VENDOR_EXT_XMIPSEXECTL;
> +
> + if (has_mipspause) {
> + /* Encoding of the mips pause instruction */
> + __asm__ __volatile__(".4byte 0x00501013");
> + } else {
> + /* Encoding of the pause instruction */
> + __asm__ __volatile__(".4byte 0x100000F");
> + }
> +
cpu_relax() is used in places where we cannot afford the overhead nor call
arbitrary functions which may take locks, etc. We've even had trouble
using a static key here in the past since this is inlined and it bloated
the size too much. You'll need to use ALTERNATIVE().
Thanks,
drew
> barrier();
> }
>
>
> --
> 2.34.1
>
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
On Thu, Jun 26, 2025 at 11:21:10AM +0200, Andrew Jones wrote:
> On Wed, Jun 25, 2025 at 04:21:01PM +0200, Aleksa Paunovic via B4 Relay wrote:
> > From: Aleksa Paunovic <aleksa.paunovic@htecgroup.com>
> >
> > Use the hwprobe syscall to decide which PAUSE instruction to execute in
> > userspace code.
> >
> > Signed-off-by: Aleksa Paunovic <aleksa.paunovic@htecgroup.com>
> > ---
> > tools/arch/riscv/include/asm/vdso/processor.h | 27 +++++++++++++++++----------
> > 1 file changed, 17 insertions(+), 10 deletions(-)
> >
> > diff --git a/tools/arch/riscv/include/asm/vdso/processor.h b/tools/arch/riscv/include/asm/vdso/processor.h
> > index 662aca03984817f9c69186658b19e9dad9e4771c..027219a486b7b93814888190f8224af29498707c 100644
> > --- a/tools/arch/riscv/include/asm/vdso/processor.h
> > +++ b/tools/arch/riscv/include/asm/vdso/processor.h
> > @@ -4,26 +4,33 @@
> >
> > #ifndef __ASSEMBLY__
> >
> > +#include <asm/hwprobe.h>
> > +#include <sys/hwprobe.h>
> > +#include <asm/vendor/mips.h>
> > #include <asm-generic/barrier.h>
> >
> > static inline void cpu_relax(void)
> > {
> > + struct riscv_hwprobe pair;
> > + bool has_mipspause;
> > #ifdef __riscv_muldiv
> > int dummy;
> > /* In lieu of a halt instruction, induce a long-latency stall. */
> > __asm__ __volatile__ ("div %0, %0, zero" : "=r" (dummy));
> > #endif
> >
> > -#ifdef CONFIG_TOOLCHAIN_HAS_ZIHINTPAUSE
> > - /*
> > - * Reduce instruction retirement.
> > - * This assumes the PC changes.
> > - */
> > - __asm__ __volatile__ ("pause");
> > -#else
> > - /* Encoding of the pause instruction */
> > - __asm__ __volatile__ (".4byte 0x100000F");
> > -#endif
> > + pair.key = RISCV_HWPROBE_KEY_VENDOR_EXT_MIPS_0;
> > + __riscv_hwprobe(&pair, 1, 0, NULL, 0);
> > + has_mipspause = pair.value & RISCV_HWPROBE_VENDOR_EXT_XMIPSEXECTL;
> > +
> > + if (has_mipspause) {
> > + /* Encoding of the mips pause instruction */
> > + __asm__ __volatile__(".4byte 0x00501013");
> > + } else {
> > + /* Encoding of the pause instruction */
> > + __asm__ __volatile__(".4byte 0x100000F");
> > + }
> > +
>
> cpu_relax() is used in places where we cannot afford the overhead nor call
> arbitrary functions which may take locks, etc. We've even had trouble
> using a static key here in the past since this is inlined and it bloated
> the size too much. You'll need to use ALTERNATIVE().
Oh, I see now that the next patch is handling the kernel cpu_relax with
ALTERNATIVE and this was just the tools cpu_relax. We don't want to make
a syscall inside cpu_relax though either, since it gets called in loops.
It'd be better to just call the standard pause (0x100000F) even if it
does nothing. Or maybe there's some define that can be added/used to
select the correct instruction?
Thanks,
drew
On Thu, Jun 26, 2025 at 11:34:21AM +0200, Andrew Jones wrote:
> On Thu, Jun 26, 2025 at 11:21:10AM +0200, Andrew Jones wrote:
> > On Wed, Jun 25, 2025 at 04:21:01PM +0200, Aleksa Paunovic via B4 Relay wrote:
> > > From: Aleksa Paunovic <aleksa.paunovic@htecgroup.com>
> > >
> > > Use the hwprobe syscall to decide which PAUSE instruction to execute in
> > > userspace code.
> > >
> > > Signed-off-by: Aleksa Paunovic <aleksa.paunovic@htecgroup.com>
> > > ---
> > > tools/arch/riscv/include/asm/vdso/processor.h | 27 +++++++++++++++++----------
> > > 1 file changed, 17 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/tools/arch/riscv/include/asm/vdso/processor.h b/tools/arch/riscv/include/asm/vdso/processor.h
> > > index 662aca03984817f9c69186658b19e9dad9e4771c..027219a486b7b93814888190f8224af29498707c 100644
> > > --- a/tools/arch/riscv/include/asm/vdso/processor.h
> > > +++ b/tools/arch/riscv/include/asm/vdso/processor.h
> > > @@ -4,26 +4,33 @@
> > >
> > > #ifndef __ASSEMBLY__
> > >
> > > +#include <asm/hwprobe.h>
> > > +#include <sys/hwprobe.h>
> > > +#include <asm/vendor/mips.h>
> > > #include <asm-generic/barrier.h>
> > >
> > > static inline void cpu_relax(void)
> > > {
> > > + struct riscv_hwprobe pair;
> > > + bool has_mipspause;
> > > #ifdef __riscv_muldiv
> > > int dummy;
> > > /* In lieu of a halt instruction, induce a long-latency stall. */
> > > __asm__ __volatile__ ("div %0, %0, zero" : "=r" (dummy));
> > > #endif
> > >
> > > -#ifdef CONFIG_TOOLCHAIN_HAS_ZIHINTPAUSE
> > > - /*
> > > - * Reduce instruction retirement.
> > > - * This assumes the PC changes.
> > > - */
> > > - __asm__ __volatile__ ("pause");
> > > -#else
> > > - /* Encoding of the pause instruction */
> > > - __asm__ __volatile__ (".4byte 0x100000F");
> > > -#endif
> > > + pair.key = RISCV_HWPROBE_KEY_VENDOR_EXT_MIPS_0;
> > > + __riscv_hwprobe(&pair, 1, 0, NULL, 0);
> > > + has_mipspause = pair.value & RISCV_HWPROBE_VENDOR_EXT_XMIPSEXECTL;
> > > +
> > > + if (has_mipspause) {
> > > + /* Encoding of the mips pause instruction */
> > > + __asm__ __volatile__(".4byte 0x00501013");
> > > + } else {
> > > + /* Encoding of the pause instruction */
> > > + __asm__ __volatile__(".4byte 0x100000F");
> > > + }
> > > +
> >
> > cpu_relax() is used in places where we cannot afford the overhead nor call
> > arbitrary functions which may take locks, etc. We've even had trouble
> > using a static key here in the past since this is inlined and it bloated
> > the size too much. You'll need to use ALTERNATIVE().
>
> Oh, I see now that the next patch is handling the kernel cpu_relax with
> ALTERNATIVE and this was just the tools cpu_relax. We don't want to make
> a syscall inside cpu_relax though either, since it gets called in loops.
(Another follow up to myself...)
I guess with the vdso cached result it should only be a handful of
instructions, but it still seems odd to embed a call in cpu_relax.
Thanks,
drew
> It'd be better to just call the standard pause (0x100000F) even if it
> does nothing. Or maybe there's some define that can be added/used to
> select the correct instruction?
>
> Thanks,
> drew
On 26. 6. 25. 12:49, Andrew Jones wrote:> On Thu, Jun 26, 2025 at 11:34:21AM +0200, Andrew Jones wrote:
>> On Thu, Jun 26, 2025 at 11:21:10AM +0200, Andrew Jones wrote:
>>> On Wed, Jun 25, 2025 at 04:21:01PM +0200, Aleksa Paunovic via B4 Relay wrote:
>>>> From: Aleksa Paunovic <aleksa.paunovic@htecgroup.com>
>>>>
>>>> Use the hwprobe syscall to decide which PAUSE instruction to execute in
>>>> userspace code.
>>>>
>>>> Signed-off-by: Aleksa Paunovic <aleksa.paunovic@htecgroup.com>
>>>> ---
>>>> tools/arch/riscv/include/asm/vdso/processor.h | 27 +++++++++++++++++----------
>>>> 1 file changed, 17 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/tools/arch/riscv/include/asm/vdso/processor.h b/tools/arch/riscv/include/asm/vdso/processor.h
>>>> index 662aca03984817f9c69186658b19e9dad9e4771c..027219a486b7b93814888190f8224af29498707c 100644
>>>> --- a/tools/arch/riscv/include/asm/vdso/processor.h
>>>> +++ b/tools/arch/riscv/include/asm/vdso/processor.h
>>>> @@ -4,26 +4,33 @@
>>>>
>>>> #ifndef __ASSEMBLY__
>>>>
>>>> +#include <asm/hwprobe.h>
>>>> +#include <sys/hwprobe.h>
>>>> +#include <asm/vendor/mips.h>
>>>> #include <asm-generic/barrier.h>
>>>>
>>>> static inline void cpu_relax(void)
>>>> {
>>>> + struct riscv_hwprobe pair;
>>>> + bool has_mipspause;
>>>> #ifdef __riscv_muldiv
>>>> int dummy;
>>>> /* In lieu of a halt instruction, induce a long-latency stall. */
>>>> __asm__ __volatile__ ("div %0, %0, zero" : "=r" (dummy));
>>>> #endif
>>>>
>>>> -#ifdef CONFIG_TOOLCHAIN_HAS_ZIHINTPAUSE
>>>> - /*
>>>> - * Reduce instruction retirement.
>>>> - * This assumes the PC changes.
>>>> - */
>>>> - __asm__ __volatile__ ("pause");
>>>> -#else
>>>> - /* Encoding of the pause instruction */
>>>> - __asm__ __volatile__ (".4byte 0x100000F");
>>>> -#endif
>>>> + pair.key = RISCV_HWPROBE_KEY_VENDOR_EXT_MIPS_0;
>>>> + __riscv_hwprobe(&pair, 1, 0, NULL, 0);
>>>> + has_mipspause = pair.value & RISCV_HWPROBE_VENDOR_EXT_XMIPSEXECTL;
>>>> +
>>>> + if (has_mipspause) {
>>>> + /* Encoding of the mips pause instruction */
>>>> + __asm__ __volatile__(".4byte 0x00501013");
>>>> + } else {
>>>> + /* Encoding of the pause instruction */
>>>> + __asm__ __volatile__(".4byte 0x100000F");
>>>> + }
>>>> +
>>>
>>> cpu_relax() is used in places where we cannot afford the overhead nor call
>>> arbitrary functions which may take locks, etc. We've even had trouble
>>> using a static key here in the past since this is inlined and it bloated
>>> the size too much. You'll need to use ALTERNATIVE().
>>
>> Oh, I see now that the next patch is handling the kernel cpu_relax with
>> ALTERNATIVE and this was just the tools cpu_relax. We don't want to make
>> a syscall inside cpu_relax though either, since it gets called in loops.
>
> (Another follow up to myself...)
>
> I guess with the vdso cached result it should only be a handful of
> instructions, but it still seems odd to embed a call in cpu_relax.
>
Hi Andrew,
Thank you for your comments!
> Thanks,
> drew
>
>> It'd be better to just call the standard pause (0x100000F) even if it
>> does nothing. Or maybe there's some define that can be added/used to
>> select the correct instruction?
>>
We did try using an ifdef/else in v3, but since that would have to be marked
non-portable, we decided to go with a hwprobe call.
Since the MIPS pause should behave as a nop on other CPUs,
would leaving both the standard pause and the MIPS pause calls be an acceptable solution?
That said, I am not sure how this would behave on future MIPS CPUs in case they support both encodings.
Best regards,
Aleksa
>> Thanks,
>> drew
On Fri, Jun 27, 2025 at 08:40:53AM +0000, Aleksa Paunovic wrote:
> On 26. 6. 25. 12:49, Andrew Jones wrote:> On Thu, Jun 26, 2025 at 11:34:21AM +0200, Andrew Jones wrote:
> >> On Thu, Jun 26, 2025 at 11:21:10AM +0200, Andrew Jones wrote:
> >>> On Wed, Jun 25, 2025 at 04:21:01PM +0200, Aleksa Paunovic via B4 Relay wrote:
> >>>> From: Aleksa Paunovic <aleksa.paunovic@htecgroup.com>
> >>>>
> >>>> Use the hwprobe syscall to decide which PAUSE instruction to execute in
> >>>> userspace code.
> >>>>
> >>>> Signed-off-by: Aleksa Paunovic <aleksa.paunovic@htecgroup.com>
> >>>> ---
> >>>> tools/arch/riscv/include/asm/vdso/processor.h | 27 +++++++++++++++++----------
> >>>> 1 file changed, 17 insertions(+), 10 deletions(-)
> >>>>
> >>>> diff --git a/tools/arch/riscv/include/asm/vdso/processor.h b/tools/arch/riscv/include/asm/vdso/processor.h
> >>>> index 662aca03984817f9c69186658b19e9dad9e4771c..027219a486b7b93814888190f8224af29498707c 100644
> >>>> --- a/tools/arch/riscv/include/asm/vdso/processor.h
> >>>> +++ b/tools/arch/riscv/include/asm/vdso/processor.h
> >>>> @@ -4,26 +4,33 @@
> >>>>
> >>>> #ifndef __ASSEMBLY__
> >>>>
> >>>> +#include <asm/hwprobe.h>
> >>>> +#include <sys/hwprobe.h>
> >>>> +#include <asm/vendor/mips.h>
> >>>> #include <asm-generic/barrier.h>
> >>>>
> >>>> static inline void cpu_relax(void)
> >>>> {
> >>>> + struct riscv_hwprobe pair;
> >>>> + bool has_mipspause;
> >>>> #ifdef __riscv_muldiv
> >>>> int dummy;
> >>>> /* In lieu of a halt instruction, induce a long-latency stall. */
> >>>> __asm__ __volatile__ ("div %0, %0, zero" : "=r" (dummy));
> >>>> #endif
> >>>>
> >>>> -#ifdef CONFIG_TOOLCHAIN_HAS_ZIHINTPAUSE
> >>>> - /*
> >>>> - * Reduce instruction retirement.
> >>>> - * This assumes the PC changes.
> >>>> - */
> >>>> - __asm__ __volatile__ ("pause");
> >>>> -#else
> >>>> - /* Encoding of the pause instruction */
> >>>> - __asm__ __volatile__ (".4byte 0x100000F");
> >>>> -#endif
> >>>> + pair.key = RISCV_HWPROBE_KEY_VENDOR_EXT_MIPS_0;
> >>>> + __riscv_hwprobe(&pair, 1, 0, NULL, 0);
> >>>> + has_mipspause = pair.value & RISCV_HWPROBE_VENDOR_EXT_XMIPSEXECTL;
> >>>> +
> >>>> + if (has_mipspause) {
> >>>> + /* Encoding of the mips pause instruction */
> >>>> + __asm__ __volatile__(".4byte 0x00501013");
> >>>> + } else {
> >>>> + /* Encoding of the pause instruction */
> >>>> + __asm__ __volatile__(".4byte 0x100000F");
> >>>> + }
> >>>> +
> >>>
> >>> cpu_relax() is used in places where we cannot afford the overhead nor call
> >>> arbitrary functions which may take locks, etc. We've even had trouble
> >>> using a static key here in the past since this is inlined and it bloated
> >>> the size too much. You'll need to use ALTERNATIVE().
> >>
> >> Oh, I see now that the next patch is handling the kernel cpu_relax with
> >> ALTERNATIVE and this was just the tools cpu_relax. We don't want to make
> >> a syscall inside cpu_relax though either, since it gets called in loops.
> >
> > (Another follow up to myself...)
> >
> > I guess with the vdso cached result it should only be a handful of
> > instructions, but it still seems odd to embed a call in cpu_relax.
> >
>
> Hi Andrew,
>
> Thank you for your comments!
>
> > Thanks,
> > drew
> >
> >> It'd be better to just call the standard pause (0x100000F) even if it
> >> does nothing. Or maybe there's some define that can be added/used to
> >> select the correct instruction?
> >>
>
> We did try using an ifdef/else in v3, but since that would have to be marked
> non-portable, we decided to go with a hwprobe call.
> Since the MIPS pause should behave as a nop on other CPUs,
> would leaving both the standard pause and the MIPS pause calls be an acceptable solution?
>
> That said, I am not sure how this would behave on future MIPS CPUs in case they support both encodings.
We should probably avoid assuming that undefined custom instructions will
behave as nops everywhere, meaning it should remain guarded. This seems
like a problem we should try to solve before we get even more pause
instructions or whatever that need dynamic selection in userspace, but I
can't think of anything reasonable atm. For now, we may need to live with
vdso hwprobe calls in places like cpu_relax. I'll stop complaining about
this patch as I can't think of anything better.
Thanks,
drew
>
> Best regards,
> Aleksa
>
> >> Thanks,
> >> drew
On 27. 6. 25. 13:08, Andrew Jones wrote:> On Fri, Jun 27, 2025 at 08:40:53AM +0000, Aleksa Paunovic wrote:
>> On 26. 6. 25. 12:49, Andrew Jones wrote:> On Thu, Jun 26, 2025 at 11:34:21AM +0200, Andrew Jones wrote:
>>>> On Thu, Jun 26, 2025 at 11:21:10AM +0200, Andrew Jones wrote:
>>>>> On Wed, Jun 25, 2025 at 04:21:01PM +0200, Aleksa Paunovic via B4 Relay wrote:
>>>>>> From: Aleksa Paunovic <aleksa.paunovic@htecgroup.com>
>>>>>>
>>>>>> Use the hwprobe syscall to decide which PAUSE instruction to execute in
>>>>>> userspace code.
>>>>>>
>>>>>> Signed-off-by: Aleksa Paunovic <aleksa.paunovic@htecgroup.com>
>>>>>> ---
>>>>>> tools/arch/riscv/include/asm/vdso/processor.h | 27 +++++++++++++++++----------
>>>>>> 1 file changed, 17 insertions(+), 10 deletions(-)
>>>>>>
>>>>>> diff --git a/tools/arch/riscv/include/asm/vdso/processor.h b/tools/arch/riscv/include/asm/vdso/processor.h
>>>>>> index 662aca03984817f9c69186658b19e9dad9e4771c..027219a486b7b93814888190f8224af29498707c 100644
>>>>>> --- a/tools/arch/riscv/include/asm/vdso/processor.h
>>>>>> +++ b/tools/arch/riscv/include/asm/vdso/processor.h
>>>>>> @@ -4,26 +4,33 @@
>>>>>>
>>>>>> #ifndef __ASSEMBLY__
>>>>>>
>>>>>> +#include <asm/hwprobe.h>
>>>>>> +#include <sys/hwprobe.h>
>>>>>> +#include <asm/vendor/mips.h>
>>>>>> #include <asm-generic/barrier.h>
>>>>>>
>>>>>> static inline void cpu_relax(void)
>>>>>> {
>>>>>> + struct riscv_hwprobe pair;
>>>>>> + bool has_mipspause;
>>>>>> #ifdef __riscv_muldiv
>>>>>> int dummy;
>>>>>> /* In lieu of a halt instruction, induce a long-latency stall. */
>>>>>> __asm__ __volatile__ ("div %0, %0, zero" : "=r" (dummy));
>>>>>> #endif
>>>>>>
>>>>>> -#ifdef CONFIG_TOOLCHAIN_HAS_ZIHINTPAUSE
>>>>>> - /*
>>>>>> - * Reduce instruction retirement.
>>>>>> - * This assumes the PC changes.
>>>>>> - */
>>>>>> - __asm__ __volatile__ ("pause");
>>>>>> -#else
>>>>>> - /* Encoding of the pause instruction */
>>>>>> - __asm__ __volatile__ (".4byte 0x100000F");
>>>>>> -#endif
>>>>>> + pair.key = RISCV_HWPROBE_KEY_VENDOR_EXT_MIPS_0;
>>>>>> + __riscv_hwprobe(&pair, 1, 0, NULL, 0);
>>>>>> + has_mipspause = pair.value & RISCV_HWPROBE_VENDOR_EXT_XMIPSEXECTL;
>>>>>> +
>>>>>> + if (has_mipspause) {
>>>>>> + /* Encoding of the mips pause instruction */
>>>>>> + __asm__ __volatile__(".4byte 0x00501013");
>>>>>> + } else {
>>>>>> + /* Encoding of the pause instruction */
>>>>>> + __asm__ __volatile__(".4byte 0x100000F");
>>>>>> + }
>>>>>> +
>>>>>
>>>>> cpu_relax() is used in places where we cannot afford the overhead nor call
>>>>> arbitrary functions which may take locks, etc. We've even had trouble
>>>>> using a static key here in the past since this is inlined and it bloated
>>>>> the size too much. You'll need to use ALTERNATIVE().
>>>>
>>>> Oh, I see now that the next patch is handling the kernel cpu_relax with
>>>> ALTERNATIVE and this was just the tools cpu_relax. We don't want to make
>>>> a syscall inside cpu_relax though either, since it gets called in loops.
>>>
>>> (Another follow up to myself...)
>>>
>>> I guess with the vdso cached result it should only be a handful of
>>> instructions, but it still seems odd to embed a call in cpu_relax.
>>>
>>
>> Hi Andrew,
>>
>> Thank you for your comments!
>>
>>> Thanks,
>>> drew
>>>
>>>> It'd be better to just call the standard pause (0x100000F) even if it
>>>> does nothing. Or maybe there's some define that can be added/used to
>>>> select the correct instruction?
>>>>
>>
>> We did try using an ifdef/else in v3, but since that would have to be marked
>> non-portable, we decided to go with a hwprobe call.
>> Since the MIPS pause should behave as a nop on other CPUs,
>> would leaving both the standard pause and the MIPS pause calls be an acceptable solution?
>>
>> That said, I am not sure how this would behave on future MIPS CPUs in case they support both encodings.
>
> We should probably avoid assuming that undefined custom instructions will
> behave as nops everywhere, meaning it should remain guarded. This seems
> like a problem we should try to solve before we get even more pause
> instructions or whatever that need dynamic selection in userspace, but I
> can't think of anything reasonable atm. For now, we may need to live with
> vdso hwprobe calls in places like cpu_relax. I'll stop complaining about
> this patch as I can't think of anything better.
>
Hi Andrew,
Thank you again for your response.
We talked in an internal meeting and we would be fine with not touching the userspace
code with this series (just deleting this particular patch),
if that's a more acceptable solution. I am not sure how else we could proceed?
I am CCing the others who participated in the earlier conversations.
Best regards,
Aleksa
> Thanks,
> drew
>
>>
>> Best regards,
>> Aleksa
>>
>>>> Thanks,
>>>> drew
© 2016 - 2025 Red Hat, Inc.