Re: [RFC PATCH v2 02/10] lib: vdso: move call to fallback out of common code.

[Andy Lutomirski]

> On Dec 24, 2019, at 7:41 PM, christophe leroy <christophe.leroy@xxxxxx> wrote:
> 
> ï
> 
>> Le 24/12/2019 Ã 03:24, Andy Lutomirski a Ãcrit :
>>> On Mon, Dec 23, 2019 at 6:31 AM Christophe Leroy
>>> <christophe.leroy@xxxxxx> wrote:
>>> 
>>> On powerpc, VDSO functions and syscalls cannot be implemented in C
>>> because the Linux kernel ABI requires that CR[SO] bit is set in case
>>> of error and cleared when no error.
>>> 
>>> As this cannot be done in C, C VDSO functions and syscall'based
>>> fallback need a trampoline in ASM.
>>> 
>>> By moving the fallback calls out of the common code, arches like
>>> powerpc can implement both the call to C VDSO and the fallback call
>>> in a single trampoline function.
>> Maybe the issue is that I'm not a powerpc person, but I don't
>> understand this.  The common vDSO code is in C.  Presumably this means
>> that you need an asm trampoline no matter what to call the C code.  Is
>> the improvement that, with this change, you can have the asm
>> trampoline do a single branch, so it's logically:
>> ret = [call the C code];
>> if (ret == 0) {
>>  set success bit;
>> } else {
>>  ret = fallback;
>>  if (ret == 0)
>>   set success bit;
>> else
>>   set failure bit;
>> }
> 
> More simple than above, in fact it is:
> 
> ret = [call the C code];
> if (ret == 0) {
> set success bit;
> } else {
> ret = fallback [ which sets the success/failure bit];
> }
> return ret

Cute.

> 
> 
>> return ret;
>> instead of:
>> ret = [call the C code, which includes the fallback];
> 
> C code cannot handle the success/failure bit so we need to do something which does:
> 
> int assembly_to_fallback()
> {
>    ret = [syscall the fallback]
>    if (success bit set)
>        return ret;
>    else
>        return -ret;
> }

Wait, your calling convention has syscalls return positive values on error?

But I think this is moot. The syscalls in question never return nonzero success values, so you should be able to inline the syscall without worrying about this.

> 
> Also means going back and forth between the success bit and negative return.
> 
>> if (ret == 0)
>>   set success bit;
>> else
>>   set failure bit;
>> It's not obvious to me that the former ought to be faster.
>>> 
>>> The two advantages are:
>>> - No need play back and forth with CR[SO] and negative return value.
>>> - No stack frame is required in VDSO C functions for the fallbacks.
>> How is no stack frame required?  Do you mean that the presence of the
>> fallback causes worse code generation?  Can you improve the fallback
>> instead?
> 
> When function F1 calls function F2 (with BL insn), the link register (LR) is set with the return address in F1, so that at the end of F2, F2 branches to LR (with BLR insn), that's how you return from functions.
> 
> When F2 calls function F3, the same happens, LR is set to the return of F3 into F2. It means that F2 has to save LR in order to be able to return to F1, otherwise the return address from F2 into F1 is lost.
> 
> But ... thinking about it once more, indeed fallback means doing a syscall, and in fact I realise that syscalls won't clobber LR, so it should be possible to do something. Let me try it.
> 

With that plus assume that nonzero return means failure, I think you should have all your bases covered.