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;
Mime-Version: 1.0
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8
Date:   Sat, 24 Sep 2022 02:26:52 +1000
Message-Id: <CN3X6YN1FRQ3.1Z9BVD6WYQY3M@bobo>
Cc:     "Christophe Leroy" <christophe.leroy@csgroup.eu>,
        "Michael Ellerman" <mpe@ellerman.id.au>,
        <linux-kernel@vger.kernel.org>, <linuxppc-dev@lists.ozlabs.org>
Subject: Re: [PATCH] powerpc/irq: Modernise inline assembly in
 irq_soft_mask_{set,return}
From:   "Nicholas Piggin" <npiggin@gmail.com>
To:     "Segher Boessenkool" <segher@kernel.crashing.org>
References: <178f30ff62c0317061f019b3dbbc079073f104c3.1663656058.git.christophe.leroy@csgroup.eu> <CN3LB8F3D9LM.3W1RQRVS64UXU@bobo> <20220923121829.GL25951@gate.crashing.org>
In-Reply-To: <20220923121829.GL25951@gate.crashing.org>
Precedence: bulk

On Fri Sep 23, 2022 at 10:18 PM AEST, Segher Boessenkool wrote:
> On Fri, Sep 23, 2022 at 05:08:13PM +1000, Nicholas Piggin wrote:
> > 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.
>
> > 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.
>
> "Global Register Variables" in the GCC manual.
>
> > There was some assertion it would always be r13, but that can't be a
> > *general* rule. e.g., the following code:
> >=20
> > struct foo {
> > #ifdef BIGDISP
> >         int array[1024*1024];
> > #endif
> >         char bar;
> > };
> >=20
> > register struct foo *foo asm("r13");
> >=20
> > static void setval(char val)
> > {
> >         asm("stb%X0 %1,%0" : "=3Dm" (foo->bar) : "r" (val));
> > }
> >=20
> > int main(void)
> > {
> >         setval(10);
> > }
>
> Just use r13 directly in the asm, if that is what you want!
>
> > 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.
>
> GCC has all kinds of correctness guarantees, here and elsewhere, that is
> 90% of a compiler's job.  But you don't *tell* it what you consider
> "correct" here.

Right, that's what I expect. I think the confusion came from here,

https://lists.ozlabs.org/pipermail/linuxppc-dev/2022-August/247595.html

In any case it is answered now.

> You wrote "foo->bar", and this expression was translated to something
> that derived from r13.  If you made the asm something like
> 	asm("stb%X0 %1,0(%0)" : : "r" (foo), "r" (val) : "memory");
> it would work fine.  It would also work fine if you wrote 13 in the
> template directly.  These things follow the rules, so are guaranteed.
>
> The most important pieces of doc here may be
>    * Accesses to the variable may be optimized as usual and the register
>      remains available for allocation and use in any computations,
>      provided that observable values of the variable are not affected.
>    * If the variable is referenced in inline assembly, the type of
>      access must be provided to the compiler via constraints (*note
>      Constraints::).  Accesses from basic asms are not supported.
> but read the whole "Global Register Variables" chapter?

I still don't see what clauses guarantees asm("%0" ::"r"(foo)) to give
13. It doesn't say access via inline assembly is special, so if
optimized as usual means it could be accessed by any register like
access to a usual variable, then asm could also substitute a different
register for the access by the letter of it AFAIKS.

I think if it was obviously guaranteed then this might be marginally
better than explicit r13 in the asm

       asm volatile(
               "stb %0,%2(%1)"
               :
               : "r" (mask),
	         "r" (local_paca),
                 "i" (offsetof(struct paca_struct, irq_soft_mask))
               : "memory");

But as it is I think we should just stick with explicit r13 base
in the asm.

Thanks,
Nick