Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 1 Sep 2022 09:37:42 +0200
From:   Gabriel Paubert <paubert@iram.es>
To:     Christophe Leroy <christophe.leroy@csgroup.eu>
Cc:     Segher Boessenkool <segher@kernel.crashing.org>,
        Michael Ellerman <mpe@ellerman.id.au>,
        Zhouyi Zhou <zhouzhouyi@gmail.com>,
        "linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Nicholas Piggin <npiggin@gmail.com>
Subject: Re: [PATCH v2] powerpc: Fix irq_soft_mask_set() and
 irq_soft_mask_return() with sanitizer
Message-ID: <YxBhRtTuJ4Ut7wUt@lt-gp.iram.es>
References: <c0b486e782b6695092dcdb2cd340a3d44c8c266d.1661272738.git.christophe.leroy@csgroup.eu>
 <CMJ3VICKD1CI.SVFJOKYJPKZQ@bobo>
 <e022754d-b4d3-bc9f-cc79-2cf556180459@csgroup.eu>
 <CMJ8P06JA9OY.1S8VDV2XRU3W5@bobo>
 <7c11b659-5b8e-256c-508e-39395041fccb@csgroup.eu>
 <20220831224522.GX25951@gate.crashing.org>
 <ad5ebb37-6b6f-372c-dd07-fc4cfc5f5fe5@csgroup.eu>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
In-Reply-To: <ad5ebb37-6b6f-372c-dd07-fc4cfc5f5fe5@csgroup.eu>
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Sep 01, 2022 at 05:22:32AM +0000, Christophe Leroy wrote:
>=20
>=20
> Le 01/09/2022 =E0 00:45, Segher Boessenkool a =E9crit=A0:
> > Hi!
> >=20
> > On Tue, Aug 30, 2022 at 09:10:02AM +0000, Christophe Leroy wrote:
> >> Le 30/08/2022 =E0 11:01, Nicholas Piggin a =E9crit=A0:
> >>> On Tue Aug 30, 2022 at 3:24 PM AEST, Christophe Leroy wrote:
> >>>>> This is still slightly concerning to me. Is there any guarantee t=
hat 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");
> >=20
> > And this is in global scope, making it a global register variable.
> >=20
> >>>> Why would the compiler use another register ?
> >>>
> >>> Hopefully it doesn't. Is it guaranteed that it won't?
> >=20
> > Yes, this is guaranteed.
> >=20
> > 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 t=
o
> > put it in the specified register.
> >=20
> > But this is a global register variable.  The only thing that would ma=
ke
> > 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).
> >=20
> >>> I'm sure Segher will be delighted with the creative asm in __do_IRQ
> >>> and call_do_irq :) *Grabs popcorn*
> >=20
> > All that %% is blinding, yes.
> >=20
> > Inline tabs are bad taste.
> >=20
> > Operand names instead of numbers are great for obfuscation, and nothi=
ng
> > else -- unless you have more than four or five operands, in which cas=
e
> > you have bigger problems already.
> >=20
> > Oh, this function is a good example of proper use of local register a=
sm,
> > btw.
> >=20
> > Comments like "// Inputs" are just harmful.  As is the "creative"
> > indentation here.  Both harm readability and do not help understandin=
g
> > in any other way either.
> >=20
> > 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.  Keepin=
g
> > it small, simple, readable is paramount.
> >=20
> > The rules for using inline asm:
> >=20
> > 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 latt=
er,
> >     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.
> >=20
> > Inline asm is a very powerful escape hatch.  Like all emergency exits=
,
> > you should not use them if you do not need them!  :-)
> >=20
> > 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 perfect=
ly
> > fine (this is very dependent on exact ABI used, etc.)
> >=20
> > I would have used real assembler code here (in a .s file).  But there
> > probably are reasons to do things this way, performance probably?
>=20
> 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.

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

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