[18/18] powerpc/64s: move power4 idle entirely to C

Christophe asked about doing this, most of the code is still in
asm but maybe it's slightly nicer? I don't know if it's worthwhile.
---
 arch/powerpc/kernel/idle.c        | 25 ++++++++++++++++++++-----
 arch/powerpc/kernel/idle_book3s.S | 22 ----------------------
 2 files changed, 20 insertions(+), 27 deletions(-)

Hi Nicholas,

I love your patch! Yet something to improve:

[auto build test ERROR on powerpc/next]
[also build test ERROR on v5.10-rc2 next-20201105]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Nicholas-Piggin/powerpc-interrupt-wrappers/20201105-231909
base:   https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next
config: powerpc-allyesconfig (attached as .config)
compiler: powerpc64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/561aae13dd3400b772b91d60681d55937e046cbc
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Nicholas-Piggin/powerpc-interrupt-wrappers/20201105-231909
        git checkout 561aae13dd3400b772b91d60681d55937e046cbc
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=powerpc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   {standard input}: Assembler messages:
>> {standard input}:662: Error: unsupported relocation against r13

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Le 05/11/2020 à 15:34, Nicholas Piggin a écrit :
> Christophe asked about doing this, most of the code is still in
> asm but maybe it's slightly nicer? I don't know if it's worthwhile.

Heu... I don't think I was asking for that, but why not, see later comments.

At first I was just asking to write the following in C:

+
+	.globl power4_idle_nap_return
+power4_idle_nap_return:
+	blr


In extenso, instead of the above do somewhere something like:

void power4_idle_nap_return(void)
{
}


> ---
>   arch/powerpc/kernel/idle.c        | 25 ++++++++++++++++++++-----
>   arch/powerpc/kernel/idle_book3s.S | 22 ----------------------
>   2 files changed, 20 insertions(+), 27 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/idle.c b/arch/powerpc/kernel/idle.c
> index ae0e2632393d..849e77a45915 100644
> --- a/arch/powerpc/kernel/idle.c
> +++ b/arch/powerpc/kernel/idle.c
> @@ -72,6 +72,9 @@ int powersave_nap;
>   #ifdef CONFIG_PPC_970_NAP
>   void power4_idle(void)
>   {
> +	unsigned long msr_idle = MSR_KERNEL|MSR_EE|MSR_POW;
> +	unsigned long tmp1, tmp2;
> +
>   	if (!cpu_has_feature(CPU_FTR_CAN_NAP))
>   		return;
>   
> @@ -84,13 +87,25 @@ void power4_idle(void)
>   	if (cpu_has_feature(CPU_FTR_ALTIVEC))
>   		asm volatile("DSSALL ; sync" ::: "memory");
>   
> -	power4_idle_nap();
> -
> +	asm volatile(
> +"	ld	%0,PACA_THREAD_INFO(r13)		\n"
> +"	ld	%1,TI_LOCAL_FLAGS(%0)			\n"
> +"	ori	%1,%1,_TLF_NAPPING			\n"
> +"	std	%1,TI_LOCAL_FLAGS(%0)			\n"

Can't this just be:

	current_thread_info()->local_flags |= _TLF_NAPPING;

>   	/*
> -	 * power4_idle_nap returns with interrupts enabled (soft and hard).
> -	 * to our caller with interrupts enabled (soft and hard). Our caller
> -	 * can cope with either interrupts disabled or enabled upon return.
> +	 * NAPPING bit is set, from this point onward nap_adjust_return()
> +	 * will cause interrupts to return to power4_idle_nap_return.
>   	 */
> +"1:	sync						\n"
> +"	isync						\n"
> +"	mtmsrd	%2					\n"
> +"	isync						\n"
> +"	b	1b					\n"

And this:

	for (;;) {
		mb();
		isync();
		mtmsr(MSR_KERNEL|MSR_EE|MSR_POW);
		isync();
	}


> +"	.globl power4_idle_nap_return			\n"
> +"power4_idle_nap_return:				\n"
> +	: "=r"(tmp1), "=r"(tmp2)
> +	: "r"(msr_idle)
> +	);
>   }
>   #endif
>   

Christophe

Excerpts from Christophe Leroy's message of November 7, 2020 7:43 pm:
> 
> 
> Le 05/11/2020 à 15:34, Nicholas Piggin a écrit :
>> Christophe asked about doing this, most of the code is still in
>> asm but maybe it's slightly nicer? I don't know if it's worthwhile.
> 
> Heu... I don't think I was asking for that, but why not, see later comments.
> 
> At first I was just asking to write the following in C:
> 
> +
> +	.globl power4_idle_nap_return
> +power4_idle_nap_return:
> +	blr
> 
> 
> In extenso, instead of the above do somewhere something like:
> 
> void power4_idle_nap_return(void)
> {
> }

Ah! Well either was a good question. I don't mind attempting it :)

>> ---
>>   arch/powerpc/kernel/idle.c        | 25 ++++++++++++++++++++-----
>>   arch/powerpc/kernel/idle_book3s.S | 22 ----------------------
>>   2 files changed, 20 insertions(+), 27 deletions(-)
>> 
>> diff --git a/arch/powerpc/kernel/idle.c b/arch/powerpc/kernel/idle.c
>> index ae0e2632393d..849e77a45915 100644
>> --- a/arch/powerpc/kernel/idle.c
>> +++ b/arch/powerpc/kernel/idle.c
>> @@ -72,6 +72,9 @@ int powersave_nap;
>>   #ifdef CONFIG_PPC_970_NAP
>>   void power4_idle(void)
>>   {
>> +	unsigned long msr_idle = MSR_KERNEL|MSR_EE|MSR_POW;
>> +	unsigned long tmp1, tmp2;
>> +
>>   	if (!cpu_has_feature(CPU_FTR_CAN_NAP))
>>   		return;
>>   
>> @@ -84,13 +87,25 @@ void power4_idle(void)
>>   	if (cpu_has_feature(CPU_FTR_ALTIVEC))
>>   		asm volatile("DSSALL ; sync" ::: "memory");
>>   
>> -	power4_idle_nap();
>> -
>> +	asm volatile(
>> +"	ld	%0,PACA_THREAD_INFO(r13)		\n"
>> +"	ld	%1,TI_LOCAL_FLAGS(%0)			\n"
>> +"	ori	%1,%1,_TLF_NAPPING			\n"
>> +"	std	%1,TI_LOCAL_FLAGS(%0)			\n"
> 
> Can't this just be:
> 
> 	current_thread_info()->local_flags |= _TLF_NAPPING;
> 
>>   	/*
>> -	 * power4_idle_nap returns with interrupts enabled (soft and hard).
>> -	 * to our caller with interrupts enabled (soft and hard). Our caller
>> -	 * can cope with either interrupts disabled or enabled upon return.
>> +	 * NAPPING bit is set, from this point onward nap_adjust_return()
>> +	 * will cause interrupts to return to power4_idle_nap_return.
>>   	 */
>> +"1:	sync						\n"
>> +"	isync						\n"
>> +"	mtmsrd	%2					\n"
>> +"	isync						\n"
>> +"	b	1b					\n"
> 
> And this:
> 
> 	for (;;) {
> 		mb();
> 		isync();
> 		mtmsr(MSR_KERNEL|MSR_EE|MSR_POW);
> 		isync();
> 	}

I was hoping something nicer like this but I think not because as soon 
as we set _TLF_NAPPING, we might take an interrupt which returns 
somewhere else, and you aren't allowed to do that in C code (mainly 
because the stack and register state would be unknown). Even going 
immediately to blr or end of function might miss restoring NVGPRs etc.

There might be some tricks we could play with soft-masking interrupts, 
using MSR[EE]=0, and then doing all this and returning to right after 
the mtmsr POW with a flag set...  But it's a bit of tricky churn for an 
old CPU that works okay.

Thanks,
Nick

> 
> 
>> +"	.globl power4_idle_nap_return			\n"
>> +"power4_idle_nap_return:				\n"
>> +	: "=r"(tmp1), "=r"(tmp2)
>> +	: "r"(msr_idle)
>> +	);
>>   }
>>   #endif
>>   
> 
> Christophe
>