Re: [PATCH v2 5/9] LoongArch: Simplify the invtlb wrappers

[WANG Xuerui]

Good evening! (I really have to get some sleep real soon.)

On 6/25/23 02:55, Xi Ruoyao wrote:
> On Sun, 2023-06-25 at 02:40 +0800, WANG Xuerui wrote:
> > From: WANG Xuerui <git@xxxxxxxxxx>
> >
> > The invtlb instruction has been supported by upstream LoongArch
> > toolchains from day one, so ditch the raw opcode trickery and just use
> > plain inline asm for it.
> >
> > While at it, also make the invtlb asm statements barriers, for proper
> > modeling of the side effects.
> >
> > The signature of the other more specific invtlb wrappers contain unused
> > arguments right now, but these are not removed right away in order for
> > the patch to be focused. In the meantime, assertions are added to ensure
> > no accidental misuse happens before the refactor. (The more specific
> > wrappers cannot re-use the generic invtlb wrapper, because the ISA
> > manual says $zero shall be used in case a particular op does not take
> > the respective argument: re-using the generic wrapper would mean losing
> > control over the register usage.)
> >
> > Signed-off-by: WANG Xuerui <git@xxxxxxxxxx>
> > ---
> >   arch/loongarch/include/asm/tlb.h | 39 ++++++++++++++++----------------
> >   1 file changed, 19 insertions(+), 20 deletions(-)
> >
> > diff --git a/arch/loongarch/include/asm/tlb.h b/arch/loongarch/include/asm/tlb.h
> > index 0dc9ee2b05d2..15750900540c 100644
> > --- a/arch/loongarch/include/asm/tlb.h
> > +++ b/arch/loongarch/include/asm/tlb.h
> > @@ -88,52 +88,51 @@ enum invtlb_ops {
> >          INVTLB_GID_ADDR = 0x16,
> >   };
> >   
> > -/*
> > - * invtlb op info addr
> > - * (0x1 << 26) | (0x24 << 20) | (0x13 << 15) |
> > - * (addr << 10) | (info << 5) | op
> > - */
> >   static inline void invtlb(u32 op, u32 info, u64 addr)
> Oh, technically these wrappers should be __always_inline, not only
> inline because they don't work at all if not inlined.  Should we change
> them to __always_inline in this patch by the way?
Makes sense... let me send a quick v3 tomorrow (and maybe after Huacai 
has taken a look).
> >   {
> > +       BUILD_BUG_ON(!__builtin_constant_p(op));
> Hmm, I guess it's redundant.  If op is not a compile-time constant, it
> won't satisfy the "i" constraint then the compiler will complain anyway.
You're right (in v1 this wasn't done, yet it compiled just fine with GCC 
and Clang). I'll remove the op assertions in v3. Thanks for the reviews.
> >          __asm__ __volatile__(
> > -               "parse_r addr,%0\n\t"
> > -               "parse_r info,%1\n\t"
> > -               ".word ((0x6498000) | (addr << 10) | (info << 5) | %2)\n\t"
> > -               :
> > -               : "r"(addr), "r"(info), "i"(op)
> > +               "invtlb %0, %1, %2\n\t"
> >                  :
> > +               : "i"(op), "r"(info), "r"(addr)
> > +               : "memory"
> >                  );
> >   }
> Likewise for other wrappers.
>
> /* snip */
--
WANG "xen0n" Xuerui

Linux/LoongArch mailing list: https://lore.kernel.org/loongarch/