Re: [PATCH 07/14] powerpc: Add support for restartable sequences

[Mathieu Desnoyers]

----- On May 24, 2018, at 3:03 AM, Michael Ellerman mpe@xxxxxxxxxxxxxx wrote:

> Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxxxx> writes:
>> ----- On May 23, 2018, at 4:14 PM, Mathieu Desnoyers
>> mathieu.desnoyers@xxxxxxxxxxxx wrote:
> ...
>>> 
>>> Hi Boqun,
>>> 
>>> I tried your patch in a ppc64 le environment, and it does not survive boot
>>> with CONFIG_DEBUG_RSEQ=y. init gets killed right away.
> 
> 
> Sorry this code is super gross and hard to deal with.
> 
>> The following fixup gets ppc64 to work:
>>
>> --- a/arch/powerpc/kernel/entry_64.S
>> +++ b/arch/powerpc/kernel/entry_64.S
>> @@ -208,6 +208,7 @@ system_call_exit:
>>         /* Check whether the syscall is issued inside a restartable sequence */
>>         addi    r3,r1,STACK_FRAME_OVERHEAD
>>         bl      rseq_syscall
>> +       ld      r3,RESULT(r1)
>>  #endif
>>         /*
>>          * Disable interrupts so current_thread_info()->flags can't change,
> 
> I don't think that's safe.
> 
> If you look above that, we have r3, r8 and r12 all live:
> 
> .Lsyscall_exit:
>	std	r3,RESULT(r1)
>	CURRENT_THREAD_INFO(r12, r1)
> 
>	ld	r8,_MSR(r1)
> #ifdef CONFIG_PPC_BOOK3S
>	/* No MSR:RI on BookE */
>	andi.	r10,r8,MSR_RI
>	beq-	.Lunrecov_restore
> #endif
> 
> 
> They're all volatile across function calls:
> 
>  http://openpowerfoundation.org/wp-content/uploads/resources/leabi/content/dbdoclet.50655240_68174.html
> 
> 
> The system_call_exit symbol is actually there for kprobes and cosmetic
> purposes. The actual syscall return flow starts at .Lsyscall_exit.
> 
> So I think this would work:
> 
> diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
> index db4df061c33a..e19f377a25e0 100644
> --- a/arch/powerpc/kernel/entry_64.S
> +++ b/arch/powerpc/kernel/entry_64.S
> @@ -184,6 +184,14 @@ system_call:			/* label this so stack traces look sane */
> 
> .Lsyscall_exit:
> 	std	r3,RESULT(r1)
> +
> +#ifdef CONFIG_DEBUG_RSEQ
> +	/* Check whether the syscall is issued inside a restartable sequence */
> +	addi    r3,r1,STACK_FRAME_OVERHEAD
> +	bl      rseq_syscall
> +	ld	r3,RESULT(r1)
> +#endif
> +
> 	CURRENT_THREAD_INFO(r12, r1)
> 
> 	ld	r8,_MSR(r1)
> 
> 
> I'll try and get this series into my test setup at some point, been a
> bit busy lately :)

Yes, this was needed. I had this in my tree already, but there is still
a kernel OOPS when running the rseq selftests on ppc64 with CONFIG_DEBUG_RSEQ=y.

My current dev tree is at: https://github.com/compudj/linux-percpu-dev/tree/rseq/dev-local

So considering we are at rc7 now, should I plan to removing the powerpc bits
for merge window submission, or is there someone planning to spend time on
fixing and testing ppc integration before the merge window opens ?

Thanks,

Mathieu


> 
> cheers

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com