Re: [PATCH] powerpc/irq: Modernise inline assembly in irq_soft_mask_{set,return}

[Nicholas Piggin]

On Tue Sep 20, 2022 at 4:41 PM AEST, Christophe Leroy wrote:
> local_paca is declared as global register asm("r13"), it is therefore
> garantied to always ever be r13.
>
> It is therefore not required to opencode r13 in the assembly, use
> a reference to local_paca->irq_soft_mask instead.
>
> This also allows removing the 'memory' clobber in irq_soft_mask_set()
> as GCC now knows what is being modified in memory.

The code matches the changelog AFAIKS. But I don't know where it is
guaranteed it will always be r13 in GCC and Clang. I still don't know
where in the specification or documentation suggests this.

There was some assertion it would always be r13, but that can't be a
*general* rule. e.g., the following code:

struct foo {
#ifdef BIGDISP
        int array[1024*1024];
#endif
        char bar;
};

register struct foo *foo asm("r13");

static void setval(char val)
{
        asm("stb%X0 %1,%0" : "=m" (foo->bar) : "r" (val));
}

int main(void)
{
        setval(10);
}

With -O0 this generates stb 9,0(10) for me for GCC 12, and with -O2
-DBIGDISP it generates stb 10,0(9). So that makes me nervious. GCC
does not have some kind of correctness guarantee here, so it must not
have this in its regression tests etc., and who knows about clang.

If it is true for some particular subset of cases that we can guarantee,
e.g., using -O2 and irq_soft_mask offset within range of stb offset and
we can point to specification such that both GCC and Clang will follow
it, then okay. Otherwise I still think it's more trouble than it is
worth.

Thanks,
Nick

>
> Using %X modifier will give GCC a bit more flexibility on the code
> generation.
>
> Signed-off-by: Christophe Leroy <christophe.leroy@xxxxxxxxxx>
> ---
>  arch/powerpc/include/asm/hw_irq.h | 12 ++----------
>  1 file changed, 2 insertions(+), 10 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/hw_irq.h b/arch/powerpc/include/asm/hw_irq.h
> index e8de249339d8..dbe037ff4474 100644
> --- a/arch/powerpc/include/asm/hw_irq.h
> +++ b/arch/powerpc/include/asm/hw_irq.h
> @@ -115,10 +115,7 @@ static inline notrace unsigned long irq_soft_mask_return(void)
>  {
>  	unsigned long flags;
>  
> -	asm volatile(
> -		"lbz %0,%1(13)"
> -		: "=r" (flags)
> -		: "i" (offsetof(struct paca_struct, irq_soft_mask)));
> +	asm volatile("lbz%X1 %0,%1" : "=r" (flags) : "m" (local_paca->irq_soft_mask));
>  
>  	return flags;
>  }
> @@ -147,12 +144,7 @@ static inline notrace void irq_soft_mask_set(unsigned long mask)
>  	if (IS_ENABLED(CONFIG_PPC_IRQ_SOFT_MASK_DEBUG))
>  		WARN_ON(mask && !(mask & IRQS_DISABLED));
>  
> -	asm volatile(
> -		"stb %0,%1(13)"
> -		:
> -		: "r" (mask),
> -		  "i" (offsetof(struct paca_struct, irq_soft_mask))
> -		: "memory");
> +	asm volatile("stb%X0 %1,%0" : "=m" (local_paca->irq_soft_mask) : "r" (mask));
>  }
>  
>  static inline notrace unsigned long irq_soft_mask_set_return(unsigned long mask)
> -- 
> 2.37.1