Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 09 Feb 2021 17:56:38 +1000
From:   Nicholas Piggin <npiggin@gmail.com>
Subject: Re: [PATCH v5 17/22] powerpc/syscall: Do not check unsupported scv
 vector on PPC32
To:     Benjamin Herrenschmidt <benh@kernel.crashing.org>,
        Christophe Leroy <christophe.leroy@csgroup.eu>,
        Michael Ellerman <mpe@ellerman.id.au>, msuchanek@suse.de,
        Paul Mackerras <paulus@samba.org>
Cc:     linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org
References: <cover.1612796617.git.christophe.leroy@csgroup.eu>
        <fc3afe1870f943b2010805fcb045b718a638b3c6.1612796617.git.christophe.leroy@csgroup.eu>
        <1612835741.qmlhg8iwmj.astroid@bobo.none>
        <82c4abb1-cb52-e856-b2dd-d7c7d48bd292@csgroup.eu>
In-Reply-To: <82c4abb1-cb52-e856-b2dd-d7c7d48bd292@csgroup.eu>
MIME-Version: 1.0
Message-Id: <1612857042.f9qpgkdvz6.astroid@bobo.none>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Excerpts from Christophe Leroy's message of February 9, 2021 4:13 pm:
>=20
>=20
> Le 09/02/2021 =C3=A0 03:00, Nicholas Piggin a =C3=A9crit=C2=A0:
>> Excerpts from Christophe Leroy's message of February 9, 2021 1:10 am:
>>> Only PPC64 has scv. No need to check the 0x7ff0 trap on PPC32.
>>> For that, add a helper trap_is_unsupported_scv() similar to
>>> trap_is_scv().
>>>
>>> And ignore the scv parameter in syscall_exit_prepare (Save 14 cycles
>>> 346 =3D> 332 cycles)
>>>
>>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>>> ---
>>> v5: Added a helper trap_is_unsupported_scv()
>>> ---
>>>   arch/powerpc/include/asm/ptrace.h | 5 +++++
>>>   arch/powerpc/kernel/entry_32.S    | 1 -
>>>   arch/powerpc/kernel/interrupt.c   | 7 +++++--
>>>   3 files changed, 10 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/arch/powerpc/include/asm/ptrace.h b/arch/powerpc/include/a=
sm/ptrace.h
>>> index 58f9dc060a7b..2c842b11a924 100644
>>> --- a/arch/powerpc/include/asm/ptrace.h
>>> +++ b/arch/powerpc/include/asm/ptrace.h
>>> @@ -229,6 +229,11 @@ static inline bool trap_is_scv(struct pt_regs *reg=
s)
>>>   	return (IS_ENABLED(CONFIG_PPC_BOOK3S_64) && TRAP(regs) =3D=3D 0x3000=
);
>>>   }
>>>  =20
>>> +static inline bool trap_is_unsupported_scv(struct pt_regs *regs)
>>> +{
>>> +	return (IS_ENABLED(CONFIG_PPC_BOOK3S_64) && TRAP(regs) =3D=3D 0x7ff0)=
;
>>> +}
>>=20
>> This change is good.
>>=20
>>> +
>>>   static inline bool trap_is_syscall(struct pt_regs *regs)
>>>   {
>>>   	return (trap_is_scv(regs) || TRAP(regs) =3D=3D 0xc00);
>>> diff --git a/arch/powerpc/kernel/entry_32.S b/arch/powerpc/kernel/entry=
_32.S
>>> index cffe58e63356..7c824e8928d0 100644
>>> --- a/arch/powerpc/kernel/entry_32.S
>>> +++ b/arch/powerpc/kernel/entry_32.S
>>> @@ -344,7 +344,6 @@ transfer_to_syscall:
>>>  =20
>>>   ret_from_syscall:
>>>   	addi    r4,r1,STACK_FRAME_OVERHEAD
>>> -	li	r5,0
>>>   	bl	syscall_exit_prepare
>>=20
>> For this one, I think it would be nice to do the "right" thing and make
>> the function prototypes different on !64S. They could then declare a
>> local const bool scv =3D 0.
>>=20
>> We could have syscall_exit_prepare and syscall_exit_prepare_maybe_scv
>> or something like that, 64s can use the latter one and the former can be
>> a wrapper that passes constant 0 for scv. Then we don't have different
>> prototypes for the same function, but you just have to make the 32-bit
>> version static inline and the 64-bit version exported to asm.
>=20
> You can't call a static inline function from ASM, I don't understand you.

I mean

#ifdef CONFIG_PPC_BOOK3S_64
notrace unsigned long syscall_exit_prepare_scv(unsigned long r3,
                                           struct pt_regs *regs,
                                           long scv)
#else
static inline long syscall_exit_prepare_scv(unsigned long r3,
                                           struct pt_regs *regs,
                                           long scv)
#endif

#ifndef CONFIG_PPC_BOOK3S_64
notrace unsigned long syscall_exit_prepare(unsigned long r3,
                                           struct pt_regs *regs)
{
	return syscall_exit_prepare_scv(r3, regs, 0);
}
#endif


>=20
> What is wrong for you really here ? Is that the fact we leave scv random,=
 or is that the below=20
> IS_ENABLED() ?

That scv arg is random. I know generated code essentially would be no=20
different and no possibility of tracing, but would just prefer to call=20
the C "correctly" if possible.

> I don't mind keeping the 'li r5,0' before calling the function if you fin=
d it cleaner, the real=20
> performance gain is with setting scv to 0 below for PPC32 (and maybe it s=
hould be set to zero for=20
> book3e/64 too ?).

Yes 64e would like this optimisation.

Thanks,
Nick