Message-ID: <2859ccc3-49f6-47cb-aff9-62704c2356be@htecgroup.com>
Date: Wed, 9 Jul 2025 14:04:52 +0000
From: Aleksa Paunovic <aleksa.paunovic@...cgroup.com>
To: "ajones@...tanamicro.com" <ajones@...tanamicro.com>
CC: Aleksa Paunovic <aleksa.paunovic@...cgroup.com>, "alex@...ti.fr"
	<alex@...ti.fr>, "aou@...s.berkeley.edu" <aou@...s.berkeley.edu>,
	"conor+dt@...nel.org" <conor+dt@...nel.org>, "conor@...nel.org"
	<conor@...nel.org>, "corbet@....net" <corbet@....net>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"devnull+aleksa.paunovic.htecgroup.com@...nel.org"
	<devnull+aleksa.paunovic.htecgroup.com@...nel.org>, "krzk+dt@...nel.org"
	<krzk+dt@...nel.org>, "linux-doc@...r.kernel.org"
	<linux-doc@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "linux-riscv@...ts.infradead.org"
	<linux-riscv@...ts.infradead.org>, "palmer@...belt.com" <palmer@...belt.com>,
	"palmer@...ive.com" <palmer@...ive.com>, "paul.walmsley@...ive.com"
	<paul.walmsley@...ive.com>, "robh@...nel.org" <robh@...nel.org>,
	"charlie@...osinc.com" <charlie@...osinc.com>, Djordje Todorovic
	<Djordje.Todorovic@...cgroup.com>
Subject: Re: [PATCH v4 6/7] riscv: Add tools support for xmipsexectl

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@...cgroup.com>
>>>>>>
>>>>>> Use the hwprobe syscall to decide which PAUSE instruction to execute in
>>>>>> userspace code.
>>>>>>
>>>>>> Signed-off-by: Aleksa Paunovic <aleksa.paunovic@...cgroup.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

Powered by blists - more mailing lists