Re: [kvm-unit-tests PATCH] lib/stack: Restrengthen base_address

[Andrew Jones <andrew.jones@linux.dev>]

On Wed, Sep 11, 2024 at 10:55:34AM GMT, Nicholas Piggin wrote:
> On Thu Sep 5, 2024 at 12:51 AM AEST, Andrew Jones wrote:
> > commit a1f2b0e1efd5 ("treewide: lib/stack: Make base_address arch
> > specific") made base_address() a weak function in order to allow
> > architectures to override it. Linking for EFI doesn't seem to figure
> > out the right one to use though [anymore?]. It must have worked at
> > one point because the commit calls outs EFI as the motivation.
> > Anyway, just drop the weakness in favor of another HAVE_ define.
> 
> I prefer HAVE_ style than weak so fine by me.
> 
> How is the linker not resolving it properly? Some calls still
> point to weak symbol despite non-weak symbol also existing?

Yeah, I noticed traces stopped working with EFI because it was using the
weak version of the function instead of the riscv non-weak version.
Since I'm 99% sure it used to work, then I need to find time to try and
figure out if it's something that changed in k-u-t that is now confusing
the toolchain or a toolchain regression. (It's on the TODO, but there's
lots of stuff on the TODO...)

> 
> 
> >
> > Signed-off-by: Andrew Jones <andrew.jones@linux.dev>
> > ---
> >  lib/riscv/asm/stack.h |  1 +
> >  lib/riscv/stack.c     |  2 +-
> >  lib/stack.c           | 10 ++++++----
> >  lib/stack.h           |  2 +-
> >  4 files changed, 9 insertions(+), 6 deletions(-)
> >
> > diff --git a/lib/riscv/asm/stack.h b/lib/riscv/asm/stack.h
> > index f003ca37c913..708fa4215007 100644
> > --- a/lib/riscv/asm/stack.h
> > +++ b/lib/riscv/asm/stack.h
> > @@ -8,5 +8,6 @@
> >  
> >  #define HAVE_ARCH_BACKTRACE_FRAME
> >  #define HAVE_ARCH_BACKTRACE
> > +#define HAVE_ARCH_BASE_ADDRESS
> >  
> >  #endif
> > diff --git a/lib/riscv/stack.c b/lib/riscv/stack.c
> > index 2cd7f012738b..a143c22a570a 100644
> > --- a/lib/riscv/stack.c
> > +++ b/lib/riscv/stack.c
> > @@ -5,7 +5,7 @@
> >  #ifdef CONFIG_RELOC
> >  extern char ImageBase, _text, _etext;
> >  
> > -bool arch_base_address(const void *rebased_addr, unsigned long *addr)
> > +bool base_address(const void *rebased_addr, unsigned long *addr)
> >  {
> >  	unsigned long ra = (unsigned long)rebased_addr;
> >  	unsigned long base = (unsigned long)&ImageBase;
> > diff --git a/lib/stack.c b/lib/stack.c
> > index 086fec544a81..e1c981085176 100644
> > --- a/lib/stack.c
> > +++ b/lib/stack.c
> > @@ -12,9 +12,10 @@
> >  #define MAX_DEPTH 20
> >  
> >  #ifdef CONFIG_RELOC
> > +#ifndef HAVE_ARCH_BASE_ADDRESS
> >  extern char _text, _etext;
> >  
> > -bool __attribute__((weak)) arch_base_address(const void *rebased_addr, unsigned long *addr)
> > +bool base_address(const void *rebased_addr, unsigned long *addr)
> >  {
> >  	unsigned long ra = (unsigned long)rebased_addr;
> >  	unsigned long start = (unsigned long)&_text;
> > @@ -26,8 +27,9 @@ bool __attribute__((weak)) arch_base_address(const void *rebased_addr, unsigned
> >  	*addr = ra - start;
> >  	return true;
> >  }
> > +#endif
> >  #else
> > -bool __attribute__((weak)) arch_base_address(const void *rebased_addr, unsigned long *addr)
> > +bool base_address(const void *rebased_addr, unsigned long *addr)
> >  {
> >  	*addr = (unsigned long)rebased_addr;
> >  	return true;
> 
> Shouldn't HAVE_ARCH_BASE_ADDRESS also cover this?

Yes, I suppose that would be the cleanest thing to do. And then in
lib/$ARCH/asm/stack.h we should have

#ifdef CONFIG_RELOC
#define HAVE_ARCH_BASE_ADDRESS
#endif

when the arch wants to use the implementation here (which is probably
would).

Thanks,
drew