Re: [PATCH v2 02/13] powerpc: Rename 'funcaddr' to 'addr' in 'struct ppc64_opd_entry'

[Christophe Leroy <christophe.leroy@csgroup.eu>]

Le 14/10/2021 à 23:45, Daniel Axtens a écrit :
> Christophe Leroy <christophe.leroy@csgroup.eu> writes:
> 
>> There are three architectures with function descriptors, try to
>> have common names for the address they contain in order to
>> refactor some functions into generic functions later.
>>
>> powerpc has 'funcaddr'
>> ia64 has 'ip'
>> parisc has 'addr'
>>
>> Vote for 'addr' and update 'struct ppc64_opd_entry' accordingly.
> 
> I would have picked 'funcaddr', but at least 'addr' is better than 'ip'!
> And I agree that consistency, and then making things generic is worthwhile.

It's a function descriptor, there is only one address field, I don't 
think there is any ambiguïty here, and I prefer modifying the least 
impacted architectures.

Changing addr to funcaddr in PARISC would result in the following 
changes, on an architecture I know nothing about. It's more changes than 
we have on powerpc.

  arch/parisc/include/asm/elf.h |  4 ++--
  arch/parisc/kernel/kexec.c    |  2 +-
  arch/parisc/kernel/module.c   | 12 ++++++------
  arch/parisc/kernel/process.c  |  2 +-
  arch/parisc/kernel/signal.c   |  4 ++--
  5 files changed, 12 insertions(+), 12 deletions(-)



> 
> I grepped the latest powerpc/next for uses of 'funcaddr'. There were 5,
> your patch changes all 5.
> 
> The series passes build tests and this patch has no checkpatch or other
> style concerns.
> 
> On that basis:
> Reviewed-by: Daniel Axtens <dja@axtens.net>
> 
> Kind regards,
> Daniel
> 
>> Reviewed-by: Kees Cook <keescook@chromium.org>
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> ---
>>   arch/powerpc/include/asm/elf.h      | 2 +-
>>   arch/powerpc/include/asm/sections.h | 2 +-
>>   arch/powerpc/kernel/module_64.c     | 6 +++---
>>   3 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/elf.h b/arch/powerpc/include/asm/elf.h
>> index a4406714c060..bb0f278f9ed4 100644
>> --- a/arch/powerpc/include/asm/elf.h
>> +++ b/arch/powerpc/include/asm/elf.h
>> @@ -178,7 +178,7 @@ void relocate(unsigned long final_address);
>>   
>>   /* There's actually a third entry here, but it's unused */
>>   struct ppc64_opd_entry {
>> -	unsigned long funcaddr;
>> +	unsigned long addr;
>>   	unsigned long r2;
>>   };
>>   
>> diff --git a/arch/powerpc/include/asm/sections.h b/arch/powerpc/include/asm/sections.h
>> index 6e4af4492a14..32e7035863ac 100644
>> --- a/arch/powerpc/include/asm/sections.h
>> +++ b/arch/powerpc/include/asm/sections.h
>> @@ -77,7 +77,7 @@ static inline void *dereference_function_descriptor(void *ptr)
>>   	struct ppc64_opd_entry *desc = ptr;
>>   	void *p;
>>   
>> -	if (!get_kernel_nofault(p, (void *)&desc->funcaddr))
>> +	if (!get_kernel_nofault(p, (void *)&desc->addr))
>>   		ptr = p;
>>   	return ptr;
>>   }
>> diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
>> index 6baa676e7cb6..82908c9be627 100644
>> --- a/arch/powerpc/kernel/module_64.c
>> +++ b/arch/powerpc/kernel/module_64.c
>> @@ -72,11 +72,11 @@ static func_desc_t func_desc(unsigned long addr)
>>   }
>>   static unsigned long func_addr(unsigned long addr)
>>   {
>> -	return func_desc(addr).funcaddr;
>> +	return func_desc(addr).addr;
>>   }
>>   static unsigned long stub_func_addr(func_desc_t func)
>>   {
>> -	return func.funcaddr;
>> +	return func.addr;
>>   }
>>   static unsigned int local_entry_offset(const Elf64_Sym *sym)
>>   {
>> @@ -187,7 +187,7 @@ static int relacmp(const void *_x, const void *_y)
>>   static unsigned long get_stubs_size(const Elf64_Ehdr *hdr,
>>   				    const Elf64_Shdr *sechdrs)
>>   {
>> -	/* One extra reloc so it's always 0-funcaddr terminated */
>> +	/* One extra reloc so it's always 0-addr terminated */
>>   	unsigned long relocs = 1;
>>   	unsigned i;
>>   
>> -- 
>> 2.31.1