From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D87C2C433EF for ; Fri, 15 Oct 2021 05:19:35 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 4A0C261164 for ; Fri, 15 Oct 2021 05:19:35 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 4A0C261164 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=csgroup.eu Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=kvack.org Received: by kanga.kvack.org (Postfix) id B2BCB6B006C; Fri, 15 Oct 2021 01:19:34 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id ADBFF6B0071; Fri, 15 Oct 2021 01:19:34 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 9A3EA900002; Fri, 15 Oct 2021 01:19:34 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0192.hostedemail.com [216.40.44.192]) by kanga.kvack.org (Postfix) with ESMTP id 87D9E6B006C for ; Fri, 15 Oct 2021 01:19:34 -0400 (EDT) Received: from smtpin08.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id 3318F39B0D for ; Fri, 15 Oct 2021 05:19:34 +0000 (UTC) X-FDA: 78697519068.08.2E9DDDB Received: from pegase2.c-s.fr (pegase2.c-s.fr [93.17.235.10]) by imf21.hostedemail.com (Postfix) with ESMTP id EE86DD04172F for ; Fri, 15 Oct 2021 05:19:32 +0000 (UTC) Received: from localhost (mailhub3.si.c-s.fr [172.26.127.67]) by localhost (Postfix) with ESMTP id 4HVvhh0J8Lz9sSF; Fri, 15 Oct 2021 07:19:32 +0200 (CEST) X-Virus-Scanned: amavisd-new at c-s.fr Received: from pegase2.c-s.fr ([172.26.127.65]) by localhost (pegase2.c-s.fr [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id jtk3Nwe2afkT; Fri, 15 Oct 2021 07:19:31 +0200 (CEST) Received: from messagerie.si.c-s.fr (messagerie.si.c-s.fr [192.168.25.192]) by pegase2.c-s.fr (Postfix) with ESMTP id 4HVvhg5r1Zz9sSD; Fri, 15 Oct 2021 07:19:31 +0200 (CEST) Received: from localhost (localhost [127.0.0.1]) by messagerie.si.c-s.fr (Postfix) with ESMTP id A27F58B788; Fri, 15 Oct 2021 07:19:31 +0200 (CEST) X-Virus-Scanned: amavisd-new at c-s.fr Received: from messagerie.si.c-s.fr ([127.0.0.1]) by localhost (messagerie.si.c-s.fr [127.0.0.1]) (amavisd-new, port 10023) with ESMTP id tLs2964MJCL0; Fri, 15 Oct 2021 07:19:31 +0200 (CEST) Received: from PO20335.IDSI0.si.c-s.fr (unknown [192.168.202.253]) by messagerie.si.c-s.fr (Postfix) with ESMTP id 721608B763; Fri, 15 Oct 2021 07:19:30 +0200 (CEST) Subject: Re: [PATCH v2 03/13] powerpc: Remove func_descr_t To: Daniel Axtens , Benjamin Herrenschmidt , Paul Mackerras , Michael Ellerman , Andrew Morton , "James E.J. Bottomley" , Helge Deller , Arnd Bergmann , Kees Cook , Greg Kroah-Hartman Cc: linux-arch@vger.kernel.org, linux-ia64@vger.kernel.org, linux-parisc@vger.kernel.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org, linuxppc-dev@lists.ozlabs.org References: <16eef6afbf7322d0c07760ebf827b8f9f50f7c6e.1634190022.git.christophe.leroy@csgroup.eu> <874k9j45fm.fsf@dja-thinkpad.axtens.net> From: Christophe Leroy Message-ID: Date: Fri, 15 Oct 2021 07:19:30 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.11.0 MIME-Version: 1.0 In-Reply-To: <874k9j45fm.fsf@dja-thinkpad.axtens.net> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: fr-FR X-Rspamd-Server: rspam06 X-Rspamd-Queue-Id: EE86DD04172F X-Stat-Signature: 4m5b9fgebjzuk7ncu871ehrdhqxux3ai Authentication-Results: imf21.hostedemail.com; dkim=none; dmarc=none; spf=pass (imf21.hostedemail.com: domain of christophe.leroy@csgroup.eu designates 93.17.235.10 as permitted sender) smtp.mailfrom=christophe.leroy@csgroup.eu X-HE-Tag: 1634275172-958487 Content-Transfer-Encoding: quoted-printable X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: Le 15/10/2021 =C3=A0 00:17, Daniel Axtens a =C3=A9crit=C2=A0: > Christophe Leroy writes: >=20 >> 'func_descr_t' is redundant with 'struct ppc64_opd_entry' >=20 > So, if I understand the overall direction of the series, you're > consolidating powerpc around one single type for function descriptors, > and then you're creating a generic typedef so that generic code can > always do ((func_desc_t)x)->addr to get the address of a function out o= f > a function descriptor regardless of arch. (And regardless of whether th= e > arch uses function descriptors or not.) An architecture not using function descriptors won't do much with=20 ((func_desc_t *)x)->addr. This is just done to allow building stuff=20 regardless. I prefer something like if (have_function_descriptors()) addr =3D (func_desc_t *)ptr)->addr; else addr =3D ptr; over #ifdef HAVE_FUNCTION_DESCRIPTORS addr =3D (func_desc_t *)ptr)->addr; #else addr =3D ptr; #endif >=20 > So: >=20 > - why pick ppc64_opd_entry over func_descr_t? Good question. At the begining it was because it was in UAPI headers,=20 and also because it was the one used in our=20 dereference_function_descriptor(). But at the end maybe that's not the more logical choice. I need to look=20 a bit more. >=20 > - Why not make our struct just called func_desc_t - why have a > ppc64_opd_entry type or a func_descr_t typedef? Well ... you usually don't flag a struct name with _t, _t will most of=20 the time refer to a typedef. If I want to avoid typedef (I know they are deprecated in kernel coding=20 stype), it means the name of the struct must be changed in every=20 architecture and it becomes tricky and it adds more churn in them, which=20 is what I want to avoid. At the end we risk to end-up with a messy set of #ifdefs. Maybe this can be done as a second step, but I would like to minimise=20 impact in this series and focus on fixing lkdtm. >=20 > - Should this patch wait until after you've made the generic > func_desc_t change and move directly to that new interface? (rather > than move from func_descr_t -> ppc64_opd_entry -> ...) Or is there = a > particular reason arch specific code should use an arch-specific > struct or named type? As mentioned in kernel coding style, typedefs reduce readability, see=20 https://www.kernel.org/doc/html/latest/process/coding-style.html#typedefs But yes, we could make a step in the direction of a common 'struct=20 func_desc'. Let's see if I can do that. Thanks for your comments. Christophe >=20 >> Remove it. >> >> Signed-off-by: Christophe Leroy >> --- >> arch/powerpc/include/asm/code-patching.h | 2 +- >> arch/powerpc/include/asm/types.h | 6 ------ >> arch/powerpc/kernel/signal_64.c | 8 ++++---- >> 3 files changed, 5 insertions(+), 11 deletions(-) >> >> diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/i= nclude/asm/code-patching.h >> index 4ba834599c4d..f3445188d319 100644 >> --- a/arch/powerpc/include/asm/code-patching.h >> +++ b/arch/powerpc/include/asm/code-patching.h >> @@ -110,7 +110,7 @@ static inline unsigned long ppc_function_entry(voi= d *func) >> * function's descriptor. The first entry in the descriptor is the >> * address of the function text. >> */ >> - return ((func_descr_t *)func)->entry; >> + return ((struct ppc64_opd_entry *)func)->addr; >> #else >> return (unsigned long)func; >> #endif >> diff --git a/arch/powerpc/include/asm/types.h b/arch/powerpc/include/a= sm/types.h >> index f1630c553efe..97da77bc48c9 100644 >> --- a/arch/powerpc/include/asm/types.h >> +++ b/arch/powerpc/include/asm/types.h >> @@ -23,12 +23,6 @@ >> =20 >> typedef __vector128 vector128; >> =20 >> -typedef struct { >> - unsigned long entry; >> - unsigned long toc; >> - unsigned long env; >> -} func_descr_t; >=20 > I was a little concerned about going from a 3-element struct to a > 2-element struct (as ppc64_opd_entry doesn't have an element for env) - > but we don't seem to take the sizeof this anywhere, nor do we use env > anywhere, nor do we do funky macro stuff with it in the signal handling > code that might implictly use the 3rd element, so I guess this will > work. Still, func_descr_t seems to describe the underlying ABI better > than ppc64_opd_entry... >=20 >> #endif /* __ASSEMBLY__ */ >> =20 >> #endif /* _ASM_POWERPC_TYPES_H */ >> diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/sig= nal_64.c >> index 1831bba0582e..63ddbe7b108c 100644 >> --- a/arch/powerpc/kernel/signal_64.c >> +++ b/arch/powerpc/kernel/signal_64.c >> @@ -933,11 +933,11 @@ int handle_rt_signal64(struct ksignal *ksig, sig= set_t *set, >> * descriptor is the entry address of signal and the second >> * entry is the TOC value we need to use. >> */ >> - func_descr_t __user *funct_desc_ptr =3D >> - (func_descr_t __user *) ksig->ka.sa.sa_handler; >> + struct ppc64_opd_entry __user *funct_desc_ptr =3D >> + (struct ppc64_opd_entry __user *)ksig->ka.sa.sa_handler; >> =20 >> - err |=3D get_user(regs->ctr, &funct_desc_ptr->entry); >> - err |=3D get_user(regs->gpr[2], &funct_desc_ptr->toc); >> + err |=3D get_user(regs->ctr, &funct_desc_ptr->addr); >> + err |=3D get_user(regs->gpr[2], &funct_desc_ptr->r2); >=20 > Likewise, r2 seems like a worse name than toc. I guess we could clean > that up another time though. >=20 > Kind regards, > Daniel >=20 >> } >> =20 >> /* enter the signal handler in native-endian mode */ >> --=20 >> 2.31.1