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:   Tue, 06 Jun 2023 18:57:03 +1000
Message-Id: <CT5FY0YBBTYX.X4UB9DSZQCMY@wheely>
Cc:     <linux-kernel@vger.kernel.org>, <linuxppc-dev@lists.ozlabs.org>
Subject: Re: [PATCH] powerpc/interrupt: Don't read MSR from
 interrupt_exit_kernel_prepare()
From:   "Nicholas Piggin" <npiggin@gmail.com>
To:     "Christophe Leroy" <christophe.leroy@csgroup.eu>,
        "Michael Ellerman" <mpe@ellerman.id.au>
References: <df36c6205ab64326fb1b991993c82057e92ace2f.1685955214.git.christophe.leroy@csgroup.eu>
In-Reply-To: <df36c6205ab64326fb1b991993c82057e92ace2f.1685955214.git.christophe.leroy@csgroup.eu>
Precedence: bulk

On Mon Jun 5, 2023 at 6:55 PM AEST, Christophe Leroy wrote:
> A disassembly of interrupt_exit_kernel_prepare() shows a useless read
> of MSR register. This is shown by r9 being re-used immediately without
> doing anything with the value read.
>
>   c000e0e0:       60 00 00 00     nop
>   c000e0e4:       7d 3a c2 a6     mfmd_ap r9
>   c000e0e8:       7d 20 00 a6     mfmsr   r9
>   c000e0ec:       7c 51 13 a6     mtspr   81,r2
>   c000e0f0:       81 3f 00 84     lwz     r9,132(r31)
>   c000e0f4:       71 29 80 00     andi.   r9,r9,32768
>
> This is due to the use of local_irq_save(). The flags read by
> local_irq_save() are never used, use local_irq_disable() instead.

I did have a patch that warns if you do a local_irq_disable() when
irqs are disabled which is why I did this, but it is kind of silly.

You could do 'if (!irqs_disabled()) local_irq_disable()'

Unfortunately that adds another branch but if it is not taken
frequently then maybe avoiding the mtMSR/EID would make it a win?
If you don't change that I might end up doing it if I can get that
warning patch merged (needs a few core kernel changes).

I wonder how much that would help local_irq_save too, if interrupts
are already disabled then avoid the mt? Maybe those things are not
very costly on smaller in-order cores.

Reviewed-by: Nicholas Piggin <npiggin@gmail.com>

>
> Fixes: 13799748b957 ("powerpc/64: use interrupt restart table to speed up=
 return from interrupt")
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> ---
>  arch/powerpc/kernel/interrupt.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interr=
upt.c
> index e34c72285b4e..f3fc5fe919d9 100644
> --- a/arch/powerpc/kernel/interrupt.c
> +++ b/arch/powerpc/kernel/interrupt.c
> @@ -368,7 +368,6 @@ void preempt_schedule_irq(void);
> =20
>  notrace unsigned long interrupt_exit_kernel_prepare(struct pt_regs *regs=
)
>  {
> -	unsigned long flags;
>  	unsigned long ret =3D 0;
>  	unsigned long kuap;
>  	bool stack_store =3D read_thread_flags() & _TIF_EMULATE_STACK_STORE;
> @@ -392,7 +391,7 @@ notrace unsigned long interrupt_exit_kernel_prepare(s=
truct pt_regs *regs)
> =20
>  	kuap =3D kuap_get_and_assert_locked();
> =20
> -	local_irq_save(flags);
> +	local_irq_disable();
> =20
>  	if (!arch_irq_disabled_regs(regs)) {
>  		/* Returning to a kernel context with local irqs enabled. */
> --=20
> 2.40.1