RE: [PATCH v2 1/4] ARC: allow to override default mcpu compiler flag

[Alexey Brodkin]

Hi Eugeniy,

A couple of minor notes below.

> -Original Message-
> From: Eugeniy Paltsev 
> Sent: Thursday, June 4, 2020 8:39 PM
> To: linux-snps-arc@lists.infradead.org; Vineet Gupta 
> Cc: linux-ker...@vger.kernel.org; Alexey Brodkin ; 
> Eugeniy Paltsev
> 
> Subject: [PATCH v2 1/4] ARC: allow to override default mcpu compiler flag
> 
> Kernel builds set their own default -mcpu for a given ISA build.

We used to use a default "-mcpu" per ARC ISA version (one for ARCompact
and one for ARCv2).

> But that gets in the way of "custom" -mcpu flags from propagating
> into kernel build.

But with more versions of CPUs & SoCs becoming available we want to
be able to fine-tune generated code more precise.

> This will also be used in next patches for HSDK-4xD board support which
> uses a different -mcpu to effect dual issue scheduling.

"...for utilization of the new CPU's dual-issue capabilities"?

> +++ b/arch/arc/Makefile
> @@ -10,8 +10,25 @@ CROSS_COMPILE := $(call cc-cross-prefix, arc-linux- 
> arceb-linux-)
>  endif
> 
>  cflags-y += -fno-common -pipe -fno-builtin -mmedium-calls -D__linux__
> -cflags-$(CONFIG_ISA_ARCOMPACT)   += -mA7
> -cflags-$(CONFIG_ISA_ARCV2)   += -mcpu=hs38
> +
> +tune-mcpu-def-$(CONFIG_ISA_ARCOMPACT):= -mA7

I'd suggest to either swap "-mA7" which is being obsoleted with "-mcpu=arc700"
right here or as a separate change, otherwise we may soon get ATC700 builds
broken with newer compilers.

-Alexey

___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc

Re: [PATCH v6 06/13] ARC: hardware floating point support

[Adhemerval Zanella]

On 05/06/2020 01:44, Vineet Gupta wrote:
> On 5/29/20 4:50 PM, Vineet Gupta via Libc-alpha wrote:
 Although this code follow other architectures, I think it woudl be better
 to move forward a macro that emulates function calls and use proper
 static inline function instead for _FPU_* (as for get-rounding-mode.h).
>>> OK. do you have a preference for names, existing upper case names OK ?
>> Something like below ?
>>
>> +# define _FPU_FPSR_FWE  0x8000
>> +
>> -#  define _FPU_GETCW(cw) __asm__ volatile ("lr %0, [0x300]" : "=r" (cw))
>> -#  define _FPU_SETCW(cw) __asm__ volatile ("sr %0, [0x300]" : : "r" (cw))
>> +static inline unsigned int arc_fpu_getcw(void)
>> +{
>> +  unsigned int cw;
>> +  __asm__ volatile ("lr %0, [0x300]" : "=r" (cw));
>> +  return cw;
>> +}
>> +
>> +static inline void arc_fpu_setcw(unsigned int cw)
>> +{
>> +  __asm__ volatile ("sr %0, [0x300]" : : "r" (cw));
>> +}
> 
> It seems there is more discussiosn to be had here. Can we just punt on this
> specific item and keep the status quo macros please. We are heading towards 
> july
> and I'd rather have the port go in this cycle !

I don't have a strong opinion here, it was more a suggestion. It seems
to follow other architectures, although at least alpha does not 
provide the _FPU_GETCW macro.  Another option would to just
move this definition to an internal header as well.

___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc

Re: [PATCH v2 0/2] rework dl-runtime: reloc_{offset,index}

[Vineet Gupta]

On 6/1/20 3:18 PM, Vineet Gupta via Libc-alpha wrote:
> Vineet Gupta (2):
>   dl-runtime: reloc_{offset,index} now functions arch overide'able
>   ARC/dl-runtime helper macros
> 
>  elf/dl-runtime.c| 28 +++--
>  elf/dl-runtime.h| 30 ++
>  sysdeps/arc/dl-runtime.h| 42 +
>  sysdeps/hppa/dl-runtime.c   |  4 
>  sysdeps/hppa/dl-runtime.h   | 31 +++
>  sysdeps/x86_64/dl-runtime.c |  9 
>  sysdeps/x86_64/dl-runtime.h | 35 +++
>  7 files changed, 155 insertions(+), 24 deletions(-)
>  create mode 100644 elf/dl-runtime.h
>  create mode 100644 sysdeps/arc/dl-runtime.h
>  create mode 100644 sysdeps/hppa/dl-runtime.h
>  delete mode 100644 sysdeps/x86_64/dl-runtime.c
>  create mode 100644 sysdeps/x86_64/dl-runtime.h

Apologies for eager ping on this but this is in the way of respin/repost of ARC 
port.

Thx,
-Vineet
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc

Re: [PATCH v2 1/2] dl-runtime: reloc_{offset,index} now functions arch overide'able

[Adhemerval Zanella]

On 01/06/2020 19:18, Vineet Gupta wrote:
> The existing macros are fragile and expect local variables with a
> certain name. Fix this by defining them as functions with default
> implementation in a new header dl-runtime.h which arches can override
> if need be.
> 
> This came up during ARC port review, hence the need for argument pltgot
> in reloc_index() which is not needed by existing ports.
> 
> This patch potentially only affects hppa/x86 ports,
> build tested for both those configs and a few more.
> 
> Suggested-by: Adhemerval Zanella 

LGTM, thanks.  There are just style nits in the one-line comment
for new files.

Reviewed-by: Adhemerval Zanella 

> ---
>  elf/dl-runtime.c| 28 +---
>  elf/dl-runtime.h| 30 ++
>  sysdeps/hppa/dl-runtime.c   |  4 
>  sysdeps/hppa/dl-runtime.h   | 31 +++
>  sysdeps/x86_64/dl-runtime.c |  9 -
>  sysdeps/x86_64/dl-runtime.h | 35 +++
>  6 files changed, 113 insertions(+), 24 deletions(-)
>  create mode 100644 elf/dl-runtime.h
>  create mode 100644 sysdeps/hppa/dl-runtime.h
>  delete mode 100644 sysdeps/x86_64/dl-runtime.c
>  create mode 100644 sysdeps/x86_64/dl-runtime.h
> 
> diff --git a/elf/dl-runtime.c b/elf/dl-runtime.c
> index cf5f1d3e82e1..85a229f6f019 100644
> --- a/elf/dl-runtime.c
> +++ b/elf/dl-runtime.c
> @@ -27,6 +27,7 @@
>  #include "dynamic-link.h"
>  #include 
>  #include 
> +#include 
>  
>  
>  #if (!ELF_MACHINE_NO_RELA && !defined ELF_MACHINE_PLT_REL) \
> @@ -42,13 +43,6 @@
>  # define ARCH_FIXUP_ATTRIBUTE
>  #endif
>  
> -#ifndef reloc_offset
> -# define reloc_offset reloc_arg
> -# define reloc_index  reloc_arg / sizeof (PLTREL)
> -#endif
> -
> -
> -
>  /* This function is called through a special trampoline from the PLT the
> first time each PLT entry is called.  We must perform the relocation
> specified in the PLT of the given shared object, and return the resolved
> @@ -68,8 +62,11 @@ _dl_fixup (
>  = (const void *) D_PTR (l, l_info[DT_SYMTAB]);
>const char *strtab = (const void *) D_PTR (l, l_info[DT_STRTAB]);
>  
> +  const uintptr_t pltgot = (uintptr_t) D_PTR (l, l_info[DT_PLTGOT]);
> +
>const PLTREL *const reloc
> -= (const void *) (D_PTR (l, l_info[DT_JMPREL]) + reloc_offset);
> += (const void *) (D_PTR (l, l_info[DT_JMPREL])
> +   + reloc_offset (pltgot, reloc_arg));
>const ElfW(Sym) *sym = &symtab[ELFW(R_SYM) (reloc->r_info)];
>const ElfW(Sym) *refsym = sym;
>void *const rel_addr = (void *)(l->l_addr + reloc->r_offset);

Ok.

> @@ -180,9 +177,12 @@ _dl_profile_fixup (
>   l, reloc_arg);
>  }
>  
> +  const uintptr_t pltgot = (uintptr_t) D_PTR (l, l_info[DT_PLTGOT]);
> +
>/* This is the address in the array where we store the result of previous
>   relocations.  */
> -  struct reloc_result *reloc_result = &l->l_reloc_result[reloc_index];
> +  struct reloc_result *reloc_result
> += &l->l_reloc_result[reloc_index (pltgot, reloc_arg, sizeof (PLTREL))];
>  
>   /* CONCURRENCY NOTES:
>  


Ok.
> @@ -219,8 +219,11 @@ _dl_profile_fixup (
>   = (const void *) D_PTR (l, l_info[DT_SYMTAB]);
>const char *strtab = (const char *) D_PTR (l, l_info[DT_STRTAB]);
>  
> +  const uintptr_t pltgot = (uintptr_t) D_PTR (l, l_info[DT_PLTGOT]);
> +
>const PLTREL *const reloc
> - = (const void *) (D_PTR (l, l_info[DT_JMPREL]) + reloc_offset);
> + = (const void *) (D_PTR (l, l_info[DT_JMPREL])
> +   + reloc_offset (pltgot, reloc_arg));
>const ElfW(Sym) *refsym = &symtab[ELFW(R_SYM) (reloc->r_info)];
>const ElfW(Sym) *defsym = refsym;
>lookup_t result;

Ok.

> @@ -485,11 +488,14 @@ _dl_call_pltexit (struct link_map *l, ElfW(Word) 
> reloc_arg,
> const void *inregs, void *outregs)
>  {
>  #ifdef SHARED
> +  const uintptr_t pltgot = (uintptr_t) D_PTR (l, l_info[DT_PLTGOT]);
> +
>/* This is the address in the array where we store the result of previous
>   relocations.  */
>// XXX Maybe the bound information must be stored on the stack since
>// XXX with bind_not a new value could have been stored in the meantime.
> -  struct reloc_result *reloc_result = &l->l_reloc_result[reloc_index];
> +  struct reloc_result *reloc_result =
> +&l->l_reloc_result[reloc_index (pltgot, reloc_arg, sizeof (PLTREL))];
>ElfW(Sym) *defsym = ((ElfW(Sym) *) D_PTR (reloc_result->bound,
>   l_info[DT_SYMTAB])
>  + reloc_result->boundndx);

Ok.

> diff --git a/elf/dl-runtime.h b/elf/dl-runtime.h
> new file mode 100644
> index ..dd87d799465e
> --- /dev/null
> +++ b/elf/dl-runtime.h
> @@ -0,0 +1,30 @@
> +/* Helpers for On-demand PLT fixup for shared objects. Generic version.

Double space after period.

> +   Copyright (C) 2020 Free Software Foundation, Inc.
> +   This file is p

Re: [PATCH v2 2/2] ARC/dl-runtime helper macros

[Adhemerval Zanella]

On 01/06/2020 19:18, Vineet Gupta wrote:
> This is purely for review purposes to attest the interface defined
> in prior patch

LGTM with some style nits, however I think it should be pushed along with 
the ARC patchset.

Reviewed-by: Adhemerval Zanella 

> ---
>  sysdeps/arc/dl-runtime.h | 42 
>  1 file changed, 42 insertions(+)
>  create mode 100644 sysdeps/arc/dl-runtime.h
> 
> diff --git a/sysdeps/arc/dl-runtime.h b/sysdeps/arc/dl-runtime.h
> new file mode 100644
> index ..529d49f5d0a1
> --- /dev/null
> +++ b/sysdeps/arc/dl-runtime.h
> @@ -0,0 +1,42 @@
> +/* Helpers for On-demand PLT fixup for shared objects. ARC version.

Double space after period.

> +   Copyright (C) 2017-2020 Free Software Foundation, Inc.
> +   This file is part of the GNU C Library.
> +
> +   The GNU C Library is free software; you can redistribute it and/or
> +   modify it under the terms of the GNU Lesser General Public License as
> +   published by the Free Software Foundation; either version 2.1 of the
> +   License, or (at your option) any later version.
> +
> +   The GNU C Library is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> +   Lesser General Public License for more details.
> +
> +   You should have received a copy of the GNU Lesser General Public
> +   License along with the GNU C Library.  If not, see
> +   .  */
> +
> +/* PLT jump into resolver passes PC of PLTn, while _dl_fixup expects the
> +   address of corresponding .rela.plt entry.
> +
> +- @plt0: runtime pc of first plt entry (DT_PLTGOT)
> +- @pltn: runtime pc of plt entry being resolved
> +- @size: size of .plt.rela entry (unused).  */
> +static inline uintptr_t
> +reloc_index (uintptr_t plt0, uintptr_t pltn, size_t size)
> +{
> +  unsigned long int idx = (unsigned long)pltn - (unsigned long)plt0;

I don't think the explicit cast is required here.

> +
> +  /* PLT trampoline is 16 bytes. */
> +  idx /= 16;
> +
> +  /* Exclude PLT0 and PLT1.  */
> +  return idx - 2;
> +}
> +
> +static inline uintptr_t
> +reloc_offset (uintptr_t plt0, uintptr_t pltn)
> +{
> +  size_t sz = sizeof (ElfW(Rela));
> +  return reloc_index(plt0, pltn, sz) * sz;

Space before parenthesis.

> +}
> 

___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc

Re: [PATCH v2 2/2] ARC/dl-runtime helper macros

[Vineet Gupta]

On 6/5/20 1:26 PM, Adhemerval Zanella via Libc-alpha wrote:
> 
> On 01/06/2020 19:18, Vineet Gupta wrote:
>> This is purely for review purposes to attest the interface defined
>> in prior patch
> LGTM with some style nits, however I think it should be pushed along with 
> the ARC patchset.

Thx. Indeed this will be subsumed into the port submission, I just sent it as a
reference to justify the interface.

> 
> Reviewed-by: Adhemerval Zanella 
> 


___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc

Re: [PATCH v2 1/2] dl-runtime: reloc_{offset,index} now functions arch overide'able

[Vineet Gupta]

On 6/5/20 1:16 PM, Adhemerval Zanella via Libc-alpha wrote:
> 
> On 01/06/2020 19:18, Vineet Gupta wrote:
>> The existing macros are fragile and expect local variables with a
>> certain name. Fix this by defining them as functions with default
>> implementation in a new header dl-runtime.h which arches can override
>> if need be.
>>
>> This came up during ARC port review, hence the need for argument pltgot
>> in reloc_index() which is not needed by existing ports.
>>
>> This patch potentially only affects hppa/x86 ports,
>> build tested for both those configs and a few more.
>>
>> Suggested-by: Adhemerval Zanella 
> LGTM, thanks.  There are just style nits in the one-line comment
> for new files.
> 
> Reviewed-by: Adhemerval Zanella 
> 

Pushed with the fixes. Thx.
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc