linux-kernel - Re: [PATCH v3 2/2] powerpc/irq: inline call_do_irq() and call_do

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20190919190524.GK9749@gate.crashing.org>
Date:   Thu, 19 Sep 2019 14:05:24 -0500
From:   Segher Boessenkool <segher@...nel.crashing.org>
To:     Christophe Leroy <christophe.leroy@....fr>
Cc:     Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>,
        Michael Ellerman <mpe@...erman.id.au>, npiggin@...il.com,
        linux-kernel@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org
Subject: Re: [PATCH v3 2/2] powerpc/irq: inline call_do_irq() and call_do_softirq()

On Thu, Sep 19, 2019 at 07:23:18AM +0200, Christophe Leroy wrote:
> Le 18/09/2019 à 18:39, Segher Boessenkool a écrit :
> >I realise the original code had this...  Loading the old stack pointer
> >value back from the stack creates a bottleneck (via the store->load
> >forwarding it requires).  It could just use
> >   addi 1,1,-(%2)
> >here, which can also be written as
> >   addi 1,1,%n2
> >(that is portable to all architectures btw).
> 
> No, we switched stack before the bl call, we replaced r1 by r3 after 
> saving r1 into r3 stack. Now we have to restore the original r1.

Yeah wow, I missed that once again.  Whoops.

Add a comment for this?  Just before the asm maybe, "we temporarily switch
r1 to a different stack" or something.

> >What about r2?  Various ABIs handle that differently.  This might make
> >it impossible to share implementation between 32-bit and 64-bit for this.
> >But we could add it to the clobber list worst case, that will always work.
> 
> Isn't r2 non-volatile on all ABIs ?

It is not.  On ELFv2 it is (or will be) optionally volatile, but like on
ELFv1 it already has special rules as well: the linker is responsible for
restoring it if it is non-volatile, and for that there needs to be a nop
after the bl, etc.

But the existing code was in a similar situation and we survived that, I
think we should be fine this way too.  And it won't be too hard to change
again if needed.

> >So anyway, it looks to me like it will work.  Nice cleanup.  Would be
> >better if you could do the call to __do_irq from C code, but maybe we
> >cannot have everything ;-)
> 
> sparc do it the following way, is there no risk that GCC adds unwanted 
> code inbetween that is not aware there the stack pointer has changed ?
> 
> void do_softirq_own_stack(void)
> {
> 	void *orig_sp, *sp = softirq_stack[smp_processor_id()];
> 
> 	sp += THREAD_SIZE - 192 - STACK_BIAS;
> 
> 	__asm__ __volatile__("mov %%sp, %0\n\t"
> 			     "mov %1, %%sp"
> 			     : "=&r" (orig_sp)
> 			     : "r" (sp));
> 	__do_softirq();
> 	__asm__ __volatile__("mov %0, %%sp"
> 			     : : "r" (orig_sp));
> }
> 
> If the above is no risk, then can we do the same on powerpc ?

No, that is a quite bad idea: it depends on the stack pointer not being
used in any way between the two asms.  Which this code does not guarantee
(what if it is inlined, for example).

Doing the stack juggling and the actual call in a single asm is much more
safe and correct.  It's just that you then need asm for the actual call
that works for all ABIs you support, etc. :-)

Segher