[PATCH v2] powerpc: Fix irq_soft_mask_set() and irq_soft_mask_return() with sanitizer

[PATCH v2] powerpc: Fix irq_soft_mask_set() and irq_soft_mask_return() with sanitizer

[Christophe Leroy]

In ppc, compiler based sanitizer will generate instrument instructions
around statement WRITE_ONCE(local_paca->irq_soft_mask, mask):

   0xc000000000295cb0 <+0>:	addis   r2,r12,774
   0xc000000000295cb4 <+4>:	addi    r2,r2,16464
   0xc000000000295cb8 <+8>:	mflr    r0
   0xc000000000295cbc <+12>:	bl      0xc00000000008bb4c <mcount>
   0xc000000000295cc0 <+16>:	mflr    r0
   0xc000000000295cc4 <+20>:	std     r31,-8(r1)
   0xc000000000295cc8 <+24>:	addi    r3,r13,2354
   0xc000000000295ccc <+28>:	mr      r31,r13
   0xc000000000295cd0 <+32>:	std     r0,16(r1)
   0xc000000000295cd4 <+36>:	stdu    r1,-48(r1)
   0xc000000000295cd8 <+40>:	bl      0xc000000000609b98 <__asan_store1+8>
   0xc000000000295cdc <+44>:	nop
   0xc000000000295ce0 <+48>:	li      r9,1
   0xc000000000295ce4 <+52>:	stb     r9,2354(r31)
   0xc000000000295ce8 <+56>:	addi    r1,r1,48
   0xc000000000295cec <+60>:	ld      r0,16(r1)
   0xc000000000295cf0 <+64>:	ld      r31,-8(r1)
   0xc000000000295cf4 <+68>:	mtlr    r0

If there is a context switch before "stb     r9,2354(r31)", r31 may
not equal to r13, in such case, irq soft mask will not work.

The same problem occurs in irq_soft_mask_return() with
READ_ONCE(local_paca->irq_soft_mask).

This patch partially reverts commit ef5b570d3700 ("powerpc/irq: Don't
open code irq_soft_mask helpers") with a more modern inline assembly.

Reported-by: Zhouyi Zhou <zhouzhouyi@gmail.com>
Fixes: ef5b570d3700 ("powerpc/irq: Don't open code irq_soft_mask helpers")
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
---
v2: Use =m constraint for stb instead of m constraint
---
 arch/powerpc/include/asm/hw_irq.h | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/include/asm/hw_irq.h b/arch/powerpc/include/asm/hw_irq.h
index 26ede09c521d..815420988ef3 100644
--- a/arch/powerpc/include/asm/hw_irq.h
+++ b/arch/powerpc/include/asm/hw_irq.h
@@ -113,7 +113,11 @@ static inline void __hard_RI_enable(void)
 
 static inline notrace unsigned long irq_soft_mask_return(void)
 {
-	return READ_ONCE(local_paca->irq_soft_mask);
+	unsigned long flags;
+
+	asm volatile("lbz%X1 %0,%1" : "=r" (flags) : "m" (local_paca->irq_soft_mask));
+
+	return flags;
 }
 
 /*
@@ -140,8 +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));
 
-	WRITE_ONCE(local_paca->irq_soft_mask, mask);
-	barrier();
+	asm volatile("stb%X0 %1,%0" : "=m" (local_paca->irq_soft_mask) : "r" (mask) : "memory");
 }
 
 static inline notrace unsigned long irq_soft_mask_set_return(unsigned long mask)
-- 
2.37.1

Re: [PATCH v2] powerpc: Fix irq_soft_mask_set() and irq_soft_mask_return() with sanitizer

[Nicholas Piggin]

On Wed Aug 24, 2022 at 2:39 AM AEST, Christophe Leroy wrote:
> In ppc, compiler based sanitizer will generate instrument instructions
> around statement WRITE_ONCE(local_paca->irq_soft_mask, mask):
>
>    0xc000000000295cb0 <+0>:	addis   r2,r12,774
>    0xc000000000295cb4 <+4>:	addi    r2,r2,16464
>    0xc000000000295cb8 <+8>:	mflr    r0
>    0xc000000000295cbc <+12>:	bl      0xc00000000008bb4c <mcount>
>    0xc000000000295cc0 <+16>:	mflr    r0
>    0xc000000000295cc4 <+20>:	std     r31,-8(r1)
>    0xc000000000295cc8 <+24>:	addi    r3,r13,2354
>    0xc000000000295ccc <+28>:	mr      r31,r13
>    0xc000000000295cd0 <+32>:	std     r0,16(r1)
>    0xc000000000295cd4 <+36>:	stdu    r1,-48(r1)
>    0xc000000000295cd8 <+40>:	bl      0xc000000000609b98 <__asan_store1+8>
>    0xc000000000295cdc <+44>:	nop
>    0xc000000000295ce0 <+48>:	li      r9,1
>    0xc000000000295ce4 <+52>:	stb     r9,2354(r31)
>    0xc000000000295ce8 <+56>:	addi    r1,r1,48
>    0xc000000000295cec <+60>:	ld      r0,16(r1)
>    0xc000000000295cf0 <+64>:	ld      r31,-8(r1)
>    0xc000000000295cf4 <+68>:	mtlr    r0
>
> If there is a context switch before "stb     r9,2354(r31)", r31 may
> not equal to r13, in such case, irq soft mask will not work.
>
> The same problem occurs in irq_soft_mask_return() with
> READ_ONCE(local_paca->irq_soft_mask).

WRITE_ONCE doesn't require address generation to be atomic with the
store so this is a bug without sanitizer too. I have seen gcc put r13
into a nvgpr before.

READ_ONCE maybe could be argued is safe in this case because data
could be stale when you use it anyway, but pointless and risky
in some cases (imagine cpu offline -> store poison value to irq soft
mask.

> This patch partially reverts commit ef5b570d3700 ("powerpc/irq: Don't
> open code irq_soft_mask helpers") with a more modern inline assembly.
>
> Reported-by: Zhouyi Zhou <zhouzhouyi@gmail.com>
> Fixes: ef5b570d3700 ("powerpc/irq: Don't open code irq_soft_mask helpers")
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
> v2: Use =m constraint for stb instead of m constraint
> ---
>  arch/powerpc/include/asm/hw_irq.h | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/hw_irq.h b/arch/powerpc/include/asm/hw_irq.h
> index 26ede09c521d..815420988ef3 100644
> --- a/arch/powerpc/include/asm/hw_irq.h
> +++ b/arch/powerpc/include/asm/hw_irq.h
> @@ -113,7 +113,11 @@ static inline void __hard_RI_enable(void)
>  
>  static inline notrace unsigned long irq_soft_mask_return(void)
>  {
> -	return READ_ONCE(local_paca->irq_soft_mask);
> +	unsigned long flags;
> +
> +	asm volatile("lbz%X1 %0,%1" : "=r" (flags) : "m" (local_paca->irq_soft_mask));
> +
> +	return flags;
>  }
>  
>  /*
> @@ -140,8 +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));
>  
> -	WRITE_ONCE(local_paca->irq_soft_mask, mask);
> -	barrier();
> +	asm volatile("stb%X0 %1,%0" : "=m" (local_paca->irq_soft_mask) : "r" (mask) : "memory");

This is still slightly concerning to me. Is there any guarantee that the
compiler would not use a different sequence for the address here?

Maybe explicit r13 is required.

Thanks,
Nick

Re: [PATCH v2] powerpc: Fix irq_soft_mask_set() and irq_soft_mask_return() with sanitizer

[Christophe Leroy]

Le 30/08/2022 à 07:15, Nicholas Piggin a écrit :
> On Wed Aug 24, 2022 at 2:39 AM AEST, Christophe Leroy wrote:
>> In ppc, compiler based sanitizer will generate instrument instructions
>> around statement WRITE_ONCE(local_paca->irq_soft_mask, mask):
>>

[...]

>>
>> If there is a context switch before "stb     r9,2354(r31)", r31 may
>> not equal to r13, in such case, irq soft mask will not work.
>>
>> The same problem occurs in irq_soft_mask_return() with
>> READ_ONCE(local_paca->irq_soft_mask).
> 
> WRITE_ONCE doesn't require address generation to be atomic with the
> store so this is a bug without sanitizer too. I have seen gcc put r13
> into a nvgpr before.
> 
> READ_ONCE maybe could be argued is safe in this case because data
> could be stale when you use it anyway, but pointless and risky
> in some cases (imagine cpu offline -> store poison value to irq soft
> mask.
> 
>> This patch partially reverts commit ef5b570d3700 ("powerpc/irq: Don't
>> open code irq_soft_mask helpers") with a more modern inline assembly.
>>
>> Reported-by: Zhouyi Zhou <zhouzhouyi@gmail.com>
>> Fixes: ef5b570d3700 ("powerpc/irq: Don't open code irq_soft_mask helpers")
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>> v2: Use =m constraint for stb instead of m constraint
>> ---
>>   arch/powerpc/include/asm/hw_irq.h | 9 ++++++---
>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/hw_irq.h b/arch/powerpc/include/asm/hw_irq.h
>> index 26ede09c521d..815420988ef3 100644
>> --- a/arch/powerpc/include/asm/hw_irq.h
>> +++ b/arch/powerpc/include/asm/hw_irq.h
>> @@ -113,7 +113,11 @@ static inline void __hard_RI_enable(void)
>>   
>>   static inline notrace unsigned long irq_soft_mask_return(void)
>>   {
>> -	return READ_ONCE(local_paca->irq_soft_mask);
>> +	unsigned long flags;
>> +
>> +	asm volatile("lbz%X1 %0,%1" : "=r" (flags) : "m" (local_paca->irq_soft_mask));
>> +
>> +	return flags;
>>   }
>>   
>>   /*
>> @@ -140,8 +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));
>>   
>> -	WRITE_ONCE(local_paca->irq_soft_mask, mask);
>> -	barrier();
>> +	asm volatile("stb%X0 %1,%0" : "=m" (local_paca->irq_soft_mask) : "r" (mask) : "memory");
> 
> This is still slightly concerning to me. Is there any guarantee that the
> compiler would not use a different sequence for the address here?
> 
> Maybe explicit r13 is required.
> 

local_paca is defined as:

	register struct paca_struct *local_paca asm("r13");

Why would the compiler use another register ? If so, do we also have an 
issue with the use of current_stack_pointer in irq.c ?

Segher ?

Re: [PATCH v2] powerpc: Fix irq_soft_mask_set() and irq_soft_mask_return() with sanitizer

[Nicholas Piggin]

On Tue Aug 30, 2022 at 3:24 PM AEST, Christophe Leroy wrote:
>
>
> Le 30/08/2022 à 07:15, Nicholas Piggin a écrit :
> > On Wed Aug 24, 2022 at 2:39 AM AEST, Christophe Leroy wrote:
> >> In ppc, compiler based sanitizer will generate instrument instructions
> >> around statement WRITE_ONCE(local_paca->irq_soft_mask, mask):
> >>
>
> [...]
>
> >>
> >> If there is a context switch before "stb     r9,2354(r31)", r31 may
> >> not equal to r13, in such case, irq soft mask will not work.
> >>
> >> The same problem occurs in irq_soft_mask_return() with
> >> READ_ONCE(local_paca->irq_soft_mask).
> > 
> > WRITE_ONCE doesn't require address generation to be atomic with the
> > store so this is a bug without sanitizer too. I have seen gcc put r13
> > into a nvgpr before.
> > 
> > READ_ONCE maybe could be argued is safe in this case because data
> > could be stale when you use it anyway, but pointless and risky
> > in some cases (imagine cpu offline -> store poison value to irq soft
> > mask.
> > 
> >> This patch partially reverts commit ef5b570d3700 ("powerpc/irq: Don't
> >> open code irq_soft_mask helpers") with a more modern inline assembly.
> >>
> >> Reported-by: Zhouyi Zhou <zhouzhouyi@gmail.com>
> >> Fixes: ef5b570d3700 ("powerpc/irq: Don't open code irq_soft_mask helpers")
> >> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> >> ---
> >> v2: Use =m constraint for stb instead of m constraint
> >> ---
> >>   arch/powerpc/include/asm/hw_irq.h | 9 ++++++---
> >>   1 file changed, 6 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/arch/powerpc/include/asm/hw_irq.h b/arch/powerpc/include/asm/hw_irq.h
> >> index 26ede09c521d..815420988ef3 100644
> >> --- a/arch/powerpc/include/asm/hw_irq.h
> >> +++ b/arch/powerpc/include/asm/hw_irq.h
> >> @@ -113,7 +113,11 @@ static inline void __hard_RI_enable(void)
> >>   
> >>   static inline notrace unsigned long irq_soft_mask_return(void)
> >>   {
> >> -	return READ_ONCE(local_paca->irq_soft_mask);
> >> +	unsigned long flags;
> >> +
> >> +	asm volatile("lbz%X1 %0,%1" : "=r" (flags) : "m" (local_paca->irq_soft_mask));
> >> +
> >> +	return flags;
> >>   }
> >>   
> >>   /*
> >> @@ -140,8 +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));
> >>   
> >> -	WRITE_ONCE(local_paca->irq_soft_mask, mask);
> >> -	barrier();
> >> +	asm volatile("stb%X0 %1,%0" : "=m" (local_paca->irq_soft_mask) : "r" (mask) : "memory");
> > 
> > This is still slightly concerning to me. Is there any guarantee that the
> > compiler would not use a different sequence for the address here?
> > 
> > Maybe explicit r13 is required.
> > 
>
> local_paca is defined as:
>
> 	register struct paca_struct *local_paca asm("r13");
>
> Why would the compiler use another register ?

Hopefully it doesn't. Is it guaranteed that it won't?

> If so, do we also have an 
> issue with the use of current_stack_pointer in irq.c ?

What problems do you think it might have?  I think it may be okay
because we're only using it to check what stack we are using so doesn't
really matter what value it is when we sample it.

The overflow check similarly probably doesn't matter the exact value.

> Segher ?

I'm sure Segher will be delighted with the creative asm in __do_IRQ
and call_do_irq :) *Grabs popcorn*

Thanks,
Nick

Re: [PATCH v2] powerpc: Fix irq_soft_mask_set() and irq_soft_mask_return() with sanitizer

[Christophe Leroy]

Le 30/08/2022 à 11:01, Nicholas Piggin a écrit :
> On Tue Aug 30, 2022 at 3:24 PM AEST, Christophe Leroy wrote:
>>> This is still slightly concerning to me. Is there any guarantee that the
>>> compiler would not use a different sequence for the address here?
>>>
>>> Maybe explicit r13 is required.
>>>
>>
>> local_paca is defined as:
>>
>> 	register struct paca_struct *local_paca asm("r13");
>>
>> Why would the compiler use another register ?
> 
> Hopefully it doesn't. Is it guaranteed that it won't?
> 
>> If so, do we also have an
>> issue with the use of current_stack_pointer in irq.c ?
> 
> What problems do you think it might have?  I think it may be okay
> because we're only using it to check what stack we are using so doesn't
> really matter what value it is when we sample it.
> 
> The overflow check similarly probably doesn't matter the exact value.
> 
>> Segher ?
> 
> I'm sure Segher will be delighted with the creative asm in __do_IRQ
> and call_do_irq :) *Grabs popcorn*
> 

Segher was in the loop, 
https://patchwork.ozlabs.org/project/linuxppc-dev/patch/5ca6639b7c1c21ee4b4138b7cfb31d6245c4195c.1570684298.git.christophe.leroy@c-s.fr/

Re: [PATCH v2] powerpc: Fix irq_soft_mask_set() and irq_soft_mask_return() with sanitizer

[Segher Boessenkool]

Hi!

On Tue, Aug 30, 2022 at 09:10:02AM +0000, Christophe Leroy wrote:
> Le 30/08/2022 à 11:01, Nicholas Piggin a écrit :
> > On Tue Aug 30, 2022 at 3:24 PM AEST, Christophe Leroy wrote:
> >>> This is still slightly concerning to me. Is there any guarantee that the
> >>> compiler would not use a different sequence for the address here?
> >>>
> >>> Maybe explicit r13 is required.
> >>>
> >>
> >> local_paca is defined as:
> >>
> >> 	register struct paca_struct *local_paca asm("r13");

And this is in global scope, making it a global register variable.

> >> Why would the compiler use another register ?
> > 
> > Hopefully it doesn't. Is it guaranteed that it won't?

Yes, this is guaranteed.

For a local register variable this is guaranteed only for operands to an
extended inline asm; any other access to the variable does not have to
put it in the specified register.

But this is a global register variable.  The only thing that would make
this crash and burn is if *any* translation unit did not see this
declaration: it could then use r13 (if that was allowed by the ABI in
effect, heh).

> > I'm sure Segher will be delighted with the creative asm in __do_IRQ
> > and call_do_irq :) *Grabs popcorn*

All that %% is blinding, yes.

Inline tabs are bad taste.

Operand names instead of numbers are great for obfuscation, and nothing
else -- unless you have more than four or five operands, in which case
you have bigger problems already.

Oh, this function is a good example of proper use of local register asm,
btw.

Comments like "// Inputs" are just harmful.  As is the "creative"
indentation here.  Both harm readability and do not help understanding
in any other way either.

The thing about inline asm is the smallest details change meaning of the
whole, it is a very harsh environment, you are programming both in C and
directly assembler code as well, and things have to be valid for both,
although on the other hand there is almost no error checking.  Keeping
it small, simple, readable is paramount.

The rules for using inline asm:

0) Do no use inline asm.
1) Use extended asm, unless you know all differences with basic asm, and
   you know you want that.  And if you answer "yes I do" to the latter,
   you answered wrong to the former.
2) Do not use toplevel asm.
3) Do no use inline asm.
4) Do no use inline asm.
5) Do no use inline asm.

Inline asm is a very powerful escape hatch.  Like all emergency exits,
you should not use them if you do not need them!  :-)

But, you are talking about the function calling and the frame change I
bet :-)  Both of these are only okay because everything is back as it
was when this (single!) asm is done, and the state created is perfectly
fine (this is very dependent on exact ABI used, etc.)

I would have used real assembler code here (in a .s file).  But there
probably are reasons to do things this way, performance probably?


Segher

Re: [PATCH v2] powerpc: Fix irq_soft_mask_set() and irq_soft_mask_return() with sanitizer

[Christophe Leroy]

Le 01/09/2022 à 00:45, Segher Boessenkool a écrit :
> Hi!
> 
> On Tue, Aug 30, 2022 at 09:10:02AM +0000, Christophe Leroy wrote:
>> Le 30/08/2022 à 11:01, Nicholas Piggin a écrit :
>>> On Tue Aug 30, 2022 at 3:24 PM AEST, Christophe Leroy wrote:
>>>>> This is still slightly concerning to me. Is there any guarantee that the
>>>>> compiler would not use a different sequence for the address here?
>>>>>
>>>>> Maybe explicit r13 is required.
>>>>>
>>>>
>>>> local_paca is defined as:
>>>>
>>>> 	register struct paca_struct *local_paca asm("r13");
> 
> And this is in global scope, making it a global register variable.
> 
>>>> Why would the compiler use another register ?
>>>
>>> Hopefully it doesn't. Is it guaranteed that it won't?
> 
> Yes, this is guaranteed.
> 
> For a local register variable this is guaranteed only for operands to an
> extended inline asm; any other access to the variable does not have to
> put it in the specified register.
> 
> But this is a global register variable.  The only thing that would make
> this crash and burn is if *any* translation unit did not see this
> declaration: it could then use r13 (if that was allowed by the ABI in
> effect, heh).
> 
>>> I'm sure Segher will be delighted with the creative asm in __do_IRQ
>>> and call_do_irq :) *Grabs popcorn*
> 
> All that %% is blinding, yes.
> 
> Inline tabs are bad taste.
> 
> Operand names instead of numbers are great for obfuscation, and nothing
> else -- unless you have more than four or five operands, in which case
> you have bigger problems already.
> 
> Oh, this function is a good example of proper use of local register asm,
> btw.
> 
> Comments like "// Inputs" are just harmful.  As is the "creative"
> indentation here.  Both harm readability and do not help understanding
> in any other way either.
> 
> The thing about inline asm is the smallest details change meaning of the
> whole, it is a very harsh environment, you are programming both in C and
> directly assembler code as well, and things have to be valid for both,
> although on the other hand there is almost no error checking.  Keeping
> it small, simple, readable is paramount.
> 
> The rules for using inline asm:
> 
> 0) Do no use inline asm.
> 1) Use extended asm, unless you know all differences with basic asm, and
>     you know you want that.  And if you answer "yes I do" to the latter,
>     you answered wrong to the former.
> 2) Do not use toplevel asm.
> 3) Do no use inline asm.
> 4) Do no use inline asm.
> 5) Do no use inline asm.
> 
> Inline asm is a very powerful escape hatch.  Like all emergency exits,
> you should not use them if you do not need them!  :-)
> 
> But, you are talking about the function calling and the frame change I
> bet :-)  Both of these are only okay because everything is back as it
> was when this (single!) asm is done, and the state created is perfectly
> fine (this is very dependent on exact ABI used, etc.)
> 
> I would have used real assembler code here (in a .s file).  But there
> probably are reasons to do things this way, performance probably?

We changed it to inline asm in order to ... inline it in the caller.

I also find that those operand names make it awull more difficult to 
read that traditional numbering. I really dislike that new trend.
And same with those // comments, better use meaningfull C variable names.

Christophe

Re: [PATCH v2] powerpc: Fix irq_soft_mask_set() and irq_soft_mask_return() with sanitizer

[Gabriel Paubert]

On Thu, Sep 01, 2022 at 05:22:32AM +0000, Christophe Leroy wrote:
> 
> 
> Le 01/09/2022 à 00:45, Segher Boessenkool a écrit :
> > Hi!
> > 
> > On Tue, Aug 30, 2022 at 09:10:02AM +0000, Christophe Leroy wrote:
> >> Le 30/08/2022 à 11:01, Nicholas Piggin a écrit :
> >>> On Tue Aug 30, 2022 at 3:24 PM AEST, Christophe Leroy wrote:
> >>>>> This is still slightly concerning to me. Is there any guarantee that the
> >>>>> compiler would not use a different sequence for the address here?
> >>>>>
> >>>>> Maybe explicit r13 is required.
> >>>>>
> >>>>
> >>>> local_paca is defined as:
> >>>>
> >>>> 	register struct paca_struct *local_paca asm("r13");
> > 
> > And this is in global scope, making it a global register variable.
> > 
> >>>> Why would the compiler use another register ?
> >>>
> >>> Hopefully it doesn't. Is it guaranteed that it won't?
> > 
> > Yes, this is guaranteed.
> > 
> > For a local register variable this is guaranteed only for operands to an
> > extended inline asm; any other access to the variable does not have to
> > put it in the specified register.
> > 
> > But this is a global register variable.  The only thing that would make
> > this crash and burn is if *any* translation unit did not see this
> > declaration: it could then use r13 (if that was allowed by the ABI in
> > effect, heh).
> > 
> >>> I'm sure Segher will be delighted with the creative asm in __do_IRQ
> >>> and call_do_irq :) *Grabs popcorn*
> > 
> > All that %% is blinding, yes.
> > 
> > Inline tabs are bad taste.
> > 
> > Operand names instead of numbers are great for obfuscation, and nothing
> > else -- unless you have more than four or five operands, in which case
> > you have bigger problems already.
> > 
> > Oh, this function is a good example of proper use of local register asm,
> > btw.
> > 
> > Comments like "// Inputs" are just harmful.  As is the "creative"
> > indentation here.  Both harm readability and do not help understanding
> > in any other way either.
> > 
> > The thing about inline asm is the smallest details change meaning of the
> > whole, it is a very harsh environment, you are programming both in C and
> > directly assembler code as well, and things have to be valid for both,
> > although on the other hand there is almost no error checking.  Keeping
> > it small, simple, readable is paramount.
> > 
> > The rules for using inline asm:
> > 
> > 0) Do no use inline asm.
> > 1) Use extended asm, unless you know all differences with basic asm, and
> >     you know you want that.  And if you answer "yes I do" to the latter,
> >     you answered wrong to the former.
> > 2) Do not use toplevel asm.
> > 3) Do no use inline asm.
> > 4) Do no use inline asm.
> > 5) Do no use inline asm.
> > 
> > Inline asm is a very powerful escape hatch.  Like all emergency exits,
> > you should not use them if you do not need them!  :-)
> > 
> > But, you are talking about the function calling and the frame change I
> > bet :-)  Both of these are only okay because everything is back as it
> > was when this (single!) asm is done, and the state created is perfectly
> > fine (this is very dependent on exact ABI used, etc.)
> > 
> > I would have used real assembler code here (in a .s file).  But there
> > probably are reasons to do things this way, performance probably?
> 
> We changed it to inline asm in order to ... inline it in the caller.

And there is a single caller it seems. Typically GCC inlines function
with a single call site, but it may be confused by asm statements.

> 
> I also find that those operand names make it awull more difficult to 
> read that traditional numbering. I really dislike that new trend.
> And same with those // comments, better use meaningfull C variable names.

Agree, but there is one thing which escapes me: why is r3 listed in the
outputs section (actually as a read write operand with the "+"
constraint modifier) but is not used after the asm which is the last
statement of function returning void?

Do I miss something?

	Gabriel

Re: [PATCH v2] powerpc: Fix irq_soft_mask_set() and irq_soft_mask_return() with sanitizer

[Christophe Leroy]

Le 01/09/2022 à 09:37, Gabriel Paubert a écrit :
> On Thu, Sep 01, 2022 at 05:22:32AM +0000, Christophe Leroy wrote:
>>
>>
>> Le 01/09/2022 à 00:45, Segher Boessenkool a écrit :
>>> Hi!
>>>
>>> On Tue, Aug 30, 2022 at 09:10:02AM +0000, Christophe Leroy wrote:
>>>> Le 30/08/2022 à 11:01, Nicholas Piggin a écrit :
>>>>> On Tue Aug 30, 2022 at 3:24 PM AEST, Christophe Leroy wrote:
>>>>>>> This is still slightly concerning to me. Is there any guarantee that the
>>>>>>> compiler would not use a different sequence for the address here?
>>>>>>>
>>>>>>> Maybe explicit r13 is required.
>>>>>>>
>>>>>>
>>>>>> local_paca is defined as:
>>>>>>
>>>>>> 	register struct paca_struct *local_paca asm("r13");
>>>
>>> And this is in global scope, making it a global register variable.
>>>
>>>>>> Why would the compiler use another register ?
>>>>>
>>>>> Hopefully it doesn't. Is it guaranteed that it won't?
>>>
>>> Yes, this is guaranteed.
>>>
>>> For a local register variable this is guaranteed only for operands to an
>>> extended inline asm; any other access to the variable does not have to
>>> put it in the specified register.
>>>
>>> But this is a global register variable.  The only thing that would make
>>> this crash and burn is if *any* translation unit did not see this
>>> declaration: it could then use r13 (if that was allowed by the ABI in
>>> effect, heh).
>>>
>>>>> I'm sure Segher will be delighted with the creative asm in __do_IRQ
>>>>> and call_do_irq :) *Grabs popcorn*
>>>
>>> All that %% is blinding, yes.
>>>
>>> Inline tabs are bad taste.
>>>
>>> Operand names instead of numbers are great for obfuscation, and nothing
>>> else -- unless you have more than four or five operands, in which case
>>> you have bigger problems already.
>>>
>>> Oh, this function is a good example of proper use of local register asm,
>>> btw.
>>>
>>> Comments like "// Inputs" are just harmful.  As is the "creative"
>>> indentation here.  Both harm readability and do not help understanding
>>> in any other way either.
>>>
>>> The thing about inline asm is the smallest details change meaning of the
>>> whole, it is a very harsh environment, you are programming both in C and
>>> directly assembler code as well, and things have to be valid for both,
>>> although on the other hand there is almost no error checking.  Keeping
>>> it small, simple, readable is paramount.
>>>
>>> The rules for using inline asm:
>>>
>>> 0) Do no use inline asm.
>>> 1) Use extended asm, unless you know all differences with basic asm, and
>>>      you know you want that.  And if you answer "yes I do" to the latter,
>>>      you answered wrong to the former.
>>> 2) Do not use toplevel asm.
>>> 3) Do no use inline asm.
>>> 4) Do no use inline asm.
>>> 5) Do no use inline asm.
>>>
>>> Inline asm is a very powerful escape hatch.  Like all emergency exits,
>>> you should not use them if you do not need them!  :-)
>>>
>>> But, you are talking about the function calling and the frame change I
>>> bet :-)  Both of these are only okay because everything is back as it
>>> was when this (single!) asm is done, and the state created is perfectly
>>> fine (this is very dependent on exact ABI used, etc.)
>>>
>>> I would have used real assembler code here (in a .s file).  But there
>>> probably are reasons to do things this way, performance probably?
>>
>> We changed it to inline asm in order to ... inline it in the caller.
> 
> And there is a single caller it seems. Typically GCC inlines function
> with a single call site, but it may be confused by asm statements.

Regardless, it is tagged __always_inline.

> 
>>
>> I also find that those operand names make it awull more difficult to
>> read that traditional numbering. I really dislike that new trend.
>> And same with those // comments, better use meaningfull C variable names.
> 
> Agree, but there is one thing which escapes me: why is r3 listed in the
> outputs section (actually as a read write operand with the "+"
> constraint modifier) but is not used after the asm which is the last
> statement of function returning void?
> 
> Do I miss something?

As far as I remember, that's to tell GCC that r3 register is modified by 
the callee. As it is an input, it couldn't be listed in the clobber list.

Christophe

Re: [PATCH v2] powerpc: Fix irq_soft_mask_set() and irq_soft_mask_return() with sanitizer

[Segher Boessenkool]

On Thu, Sep 01, 2022 at 07:47:10AM +0000, Christophe Leroy wrote:
> Le 01/09/2022 à 09:37, Gabriel Paubert a écrit :
> > Agree, but there is one thing which escapes me: why is r3 listed in the
> > outputs section (actually as a read write operand with the "+"
> > constraint modifier) but is not used after the asm which is the last
> > statement of function returning void?
> > 
> > Do I miss something?
> 
> As far as I remember, that's to tell GCC that r3 register is modified by 
> the callee. As it is an input, it couldn't be listed in the clobber list.

Inputs can be clobbered just fine, in general.  But here the operand
is tied to a register variable, and that causes the error ("'asm'
specifier for variable 'r3' conflicts with 'asm' clobber list").

Marking it in/out here is more appropriate anyway :-)


Segher

Re: [PATCH v2] powerpc: Fix irq_soft_mask_set() and irq_soft_mask_return() with sanitizer

[Segher Boessenkool]

On Thu, Sep 01, 2022 at 09:37:42AM +0200, Gabriel Paubert wrote:
> On Thu, Sep 01, 2022 at 05:22:32AM +0000, Christophe Leroy wrote:
> > Le 01/09/2022 à 00:45, Segher Boessenkool a écrit :
> > > I would have used real assembler code here (in a .s file).  But there
> > > probably are reasons to do things this way, performance probably?
> > 
> > We changed it to inline asm in order to ... inline it in the caller.
> 
> And there is a single caller it seems. Typically GCC inlines function
> with a single call site, but it may be confused by asm statements.

"Confused"...  It might estimate the assembler statement to be
significantly more expensive than it really is.  The compiler has to be
somewhat conservative, to be able to generate code that can be assembled
without errors.  It counts instructions by counting newlines and semis
and that kind of thing.  Since GCC 7 there is "asm inline", to make the
compiler think for inlining purposes that the asm outputs minimum size
code.  You can use asm_inline in the kernel for this.

> > I also find that those operand names make it awull more difficult to 
> > read that traditional numbering. I really dislike that new trend.

Yup.  All the extra markup it needs doesn't benefit readability either.

> > And same with those // comments, better use meaningfull C variable names.

I wrote:

> > > Comments like "// Inputs" are just harmful.  As is the "creative"
> > > indentation here.  Both harm readability and do not help understanding
> > > in any other way either.

This is a comment trying to explain (GNU) C syntax.  I certainly agree
that variable naming is very important, but this was meant as commentary
on a worse comment offence :-)


Segher

Re: [PATCH v2] powerpc: Fix irq_soft_mask_set() and irq_soft_mask_return() with sanitizer

[Peter Bergner]

On 8/31/22 5:45 PM, Segher Boessenkool wrote:
> On Tue, Aug 30, 2022 at 09:10:02AM +0000, Christophe Leroy wrote:
>> Le 30/08/2022 à 11:01, Nicholas Piggin a écrit :
>>> On Tue Aug 30, 2022 at 3:24 PM AEST, Christophe Leroy wrote:
>>>>> This is still slightly concerning to me. Is there any guarantee that the
>>>>> compiler would not use a different sequence for the address here?
>>>>>
>>>>> Maybe explicit r13 is required.
>>>>>
>>>>
>>>> local_paca is defined as:
>>>>
>>>> 	register struct paca_struct *local_paca asm("r13");
> 
> And this is in global scope, making it a global register variable.
> 
>>>> Why would the compiler use another register ?
>>>
>>> Hopefully it doesn't. Is it guaranteed that it won't?
> 
> Yes, this is guaranteed.

Agree with Segher here.  That said, there was a gcc bug a looooong time
ago where gcc copied r13 into a temporary register and used it from there.
That's ok (correctness wise, but not ideal) from user land standpoint,
but we took a context switch after the reg copy and it was restarted on
a different cpu, so differnt local_paca and r13 value.  We went boom
because the copy wasn't pointing to the correct local_paca anymore.
So it is very important the compiler always use r13 when accessing
the local_paca.

Peter

Re: [PATCH v2] powerpc: Fix irq_soft_mask_set() and irq_soft_mask_return() with sanitizer

[Segher Boessenkool]

On Fri, Sep 02, 2022 at 10:57:27AM -0500, Peter Bergner wrote:
> On 8/31/22 5:45 PM, Segher Boessenkool wrote:
> > Yes, this is guaranteed.
> 
> Agree with Segher here.  That said, there was a gcc bug a looooong time
> ago where gcc copied r13 into a temporary register and used it from there.

r13 is a fixed register on most of our ABIs (everything that is not AIX
or Darwin, even), so this can never happen.  Except if there are bugs,
of course ;-)

> That's ok (correctness wise, but not ideal) from user land standpoint,
> but we took a context switch after the reg copy and it was restarted on
> a different cpu, so differnt local_paca and r13 value.  We went boom
> because the copy wasn't pointing to the correct local_paca anymore.
> So it is very important the compiler always use r13 when accessing
> the local_paca.

Yes.  So we either whould use -ffixed-r13, or just not use unsupported
compilers.  powerpc*-linux and powerpc*-elf work fine for example :-)


Segher