[v5,3/4] riscv: Add ifunc-compatible hwprobe function

  ifunc selector functions are called very early, and are not able to call
external functions that require relocation. This presents a problem for
applications and libraries outside of glibc that may want to use
__riscv_hwprobe() in their ifunc selectors.

Add a new function, __riscv_hwprobe_early(), that is compiled into
libc_nonshared.a and is safe to use in application and library ifunc
selectors. The _early() variant checks a weak alias of __riscv_hwprobe,
__riscv_hwprobe_weak, to see if the symbol is available, and simply
calls that if so. However if it's called before the symbol is available,
it makes the system call directly. The early call has some caveats,
primarily that it does not take advantage of the cached vDSO data, and
does not set errno (another external symbol) upon failing.

The original dynamic __riscv_hwprobe() function is still available for
users that don't need the overhead of handling early init cases.

Signed-off-by: Evan Green <evan@rivosinc.com>

---

Changes in v5:
 - Introduced __riscv_hwprobe_early()

 sysdeps/unix/sysv/linux/riscv/Makefile        |  3 +-
 sysdeps/unix/sysv/linux/riscv/Versions        |  1 +
 sysdeps/unix/sysv/linux/riscv/hwprobe.c       |  2 ++
 .../unix/sysv/linux/riscv/hwprobe_static.c    | 36 +++++++++++++++++++
 .../unix/sysv/linux/riscv/rv32/libc.abilist   |  1 +
 .../unix/sysv/linux/riscv/rv64/libc.abilist   |  1 +
 sysdeps/unix/sysv/linux/riscv/sys/hwprobe.h   | 21 +++++++++++
 7 files changed, 64 insertions(+), 1 deletion(-)
 create mode 100644 sysdeps/unix/sysv/linux/riscv/hwprobe_static.c
  

* Evan Green:

> diff --git a/sysdeps/unix/sysv/linux/riscv/sys/hwprobe.h b/sysdeps/unix/sysv/linux/riscv/sys/hwprobe.h
> index b27af5cb07..d739371806 100644
> --- a/sysdeps/unix/sysv/linux/riscv/sys/hwprobe.h
> +++ b/sysdeps/unix/sysv/linux/riscv/sys/hwprobe.h
> @@ -67,6 +67,27 @@ extern int __riscv_hwprobe (struct riscv_hwprobe *pairs, size_t pair_count,
>       __fortified_attr_access (__read_write__, 1, 2)
>       __fortified_attr_access (__read_only__, 4, 3);
>  
> +extern int __riscv_hwprobe_weak (struct riscv_hwprobe *pairs, size_t pair_count,
> +				 size_t cpu_count, unsigned long int *cpus,
> +				 unsigned int flags)
> +     __THROW __nonnull ((1)) __wur
> +     __fortified_attr_access (__read_write__, 1, 2)
> +     __fortified_attr_access (__read_only__, 4, 3);
> +
> +# ifdef weak_extern
> +weak_extern (__riscv_hwprobe_weak)
> +# else
> +#  pragma weak __riscv_hwprobe_weak
> +# endif
> +
> +extern int __riscv_hwprobe_early (struct riscv_hwprobe *pairs, size_t pair_count,
> +				  size_t cpu_count, unsigned long int *cpus,
> +				  unsigned int flags)
> +     __THROW __nonnull ((1)) __wur
> +     __fortified_attr_access (__read_write__, 1, 2)
> +     __fortified_attr_access (__read_only__, 4, 3)
> +     __attribute__ ((__visibility__ ("hidden")));
> +
>  __END_DECLS
>  
>  #endif /* sys/hwprobe.h */

I would call the dynamic symbol ___riscv_hwprobe, not
__riscv_hwprobe_weak.  The weak property is an implementation detail in
the C file that goes into libc_nonshared.  And ___riscv_hwprobe does not
need to be declared in an installed header, I think it's fine to offer
__riscv_hwprobe only.

Use of INTERNAL_SYSCALL is correct because without relocations, errno
will not work, but it needs to match across all implementations.  This
means using INTERNAL_VSYSCALL instead of INLINE_VSYSCALL.

There needs to be manual entry that documents the exact interface
conventions.  If the function returns 0 on success (no positive return
values), it may make sense to negate the result value that comes back
from the kernel because that matches what some pthread interfaces use.
We probabl use the -EINVAL convention on some external API (it's a big
library), but I think it's on the unusual side.

Thanks,
Florian
  

On Thu, Jul 13, 2023 at 12:07 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * Evan Green:
>
> > diff --git a/sysdeps/unix/sysv/linux/riscv/sys/hwprobe.h b/sysdeps/unix/sysv/linux/riscv/sys/hwprobe.h
> > index b27af5cb07..d739371806 100644
> > --- a/sysdeps/unix/sysv/linux/riscv/sys/hwprobe.h
> > +++ b/sysdeps/unix/sysv/linux/riscv/sys/hwprobe.h
> > @@ -67,6 +67,27 @@ extern int __riscv_hwprobe (struct riscv_hwprobe *pairs, size_t pair_count,
> >       __fortified_attr_access (__read_write__, 1, 2)
> >       __fortified_attr_access (__read_only__, 4, 3);
> >
> > +extern int __riscv_hwprobe_weak (struct riscv_hwprobe *pairs, size_t pair_count,
> > +                              size_t cpu_count, unsigned long int *cpus,
> > +                              unsigned int flags)
> > +     __THROW __nonnull ((1)) __wur
> > +     __fortified_attr_access (__read_write__, 1, 2)
> > +     __fortified_attr_access (__read_only__, 4, 3);
> > +
> > +# ifdef weak_extern
> > +weak_extern (__riscv_hwprobe_weak)
> > +# else
> > +#  pragma weak __riscv_hwprobe_weak
> > +# endif
> > +
> > +extern int __riscv_hwprobe_early (struct riscv_hwprobe *pairs, size_t pair_count,
> > +                               size_t cpu_count, unsigned long int *cpus,
> > +                               unsigned int flags)
> > +     __THROW __nonnull ((1)) __wur
> > +     __fortified_attr_access (__read_write__, 1, 2)
> > +     __fortified_attr_access (__read_only__, 4, 3)
> > +     __attribute__ ((__visibility__ ("hidden")));
> > +
> >  __END_DECLS
> >
> >  #endif /* sys/hwprobe.h */
>
> I would call the dynamic symbol ___riscv_hwprobe, not
> __riscv_hwprobe_weak.  The weak property is an implementation detail in
> the C file that goes into libc_nonshared.  And ___riscv_hwprobe does not
> need to be declared in an installed header, I think it's fine to offer
> __riscv_hwprobe only.

Ok. Just to reiterate what I'm planning to export to users, I've got
__riscv_hwprobe() as a dynamic function from libc that non-ifunc
callers can use without the overhead of checking the weak symbol, as
well as __riscv_hwprobe_early() that ifunc selectors can use
pre-relocation. Based on your comment, I'll rename the
__riscv_hwprobe_weak alias to ___riscv_hwprobe.

>
> Use of INTERNAL_SYSCALL is correct because without relocations, errno
> will not work, but it needs to match across all implementations.  This
> means using INTERNAL_VSYSCALL instead of INLINE_VSYSCALL.

Oh yeah, otherwise the return values of __riscv_hwprobe_early() would
suddenly change from -EINVAL to -1 once the symbol is available. So
then we'll document errno is always left unchanged by this function.

>
> There needs to be manual entry that documents the exact interface
> conventions.  If the function returns 0 on success (no positive return
> values), it may make sense to negate the result value that comes back
> from the kernel because that matches what some pthread interfaces use.
> We probabl use the -EINVAL convention on some external API (it's a big
> library), but I think it's on the unusual side.

Correct, it's 0 on success or negative error code on failure. I guess
you're binning it in with pthreads because those functions also return
error codes directly? I can do that.

After discussing with Palmer, based on the timing of things we're
thinking it's probably too late to try and rush this into the upcoming
release. There was also a suggestion internally to try and get to the
vDSO pointer early, though I don't understand how/if this is possible.
I need to dig more on that one. I may still spin this today if I can,
but we'll see.

-Evan
  

On Thu, 13 Jul 2023 09:33:20 PDT (-0700), Evan Green wrote:
> On Thu, Jul 13, 2023 at 12:07 AM Florian Weimer <fweimer@redhat.com> wrote:
>>
>> * Evan Green:
>>
>> > diff --git a/sysdeps/unix/sysv/linux/riscv/sys/hwprobe.h b/sysdeps/unix/sysv/linux/riscv/sys/hwprobe.h
>> > index b27af5cb07..d739371806 100644
>> > --- a/sysdeps/unix/sysv/linux/riscv/sys/hwprobe.h
>> > +++ b/sysdeps/unix/sysv/linux/riscv/sys/hwprobe.h
>> > @@ -67,6 +67,27 @@ extern int __riscv_hwprobe (struct riscv_hwprobe *pairs, size_t pair_count,
>> >       __fortified_attr_access (__read_write__, 1, 2)
>> >       __fortified_attr_access (__read_only__, 4, 3);
>> >
>> > +extern int __riscv_hwprobe_weak (struct riscv_hwprobe *pairs, size_t pair_count,
>> > +                              size_t cpu_count, unsigned long int *cpus,
>> > +                              unsigned int flags)
>> > +     __THROW __nonnull ((1)) __wur
>> > +     __fortified_attr_access (__read_write__, 1, 2)
>> > +     __fortified_attr_access (__read_only__, 4, 3);
>> > +
>> > +# ifdef weak_extern
>> > +weak_extern (__riscv_hwprobe_weak)
>> > +# else
>> > +#  pragma weak __riscv_hwprobe_weak
>> > +# endif
>> > +
>> > +extern int __riscv_hwprobe_early (struct riscv_hwprobe *pairs, size_t pair_count,
>> > +                               size_t cpu_count, unsigned long int *cpus,
>> > +                               unsigned int flags)
>> > +     __THROW __nonnull ((1)) __wur
>> > +     __fortified_attr_access (__read_write__, 1, 2)
>> > +     __fortified_attr_access (__read_only__, 4, 3)
>> > +     __attribute__ ((__visibility__ ("hidden")));
>> > +
>> >  __END_DECLS
>> >
>> >  #endif /* sys/hwprobe.h */
>>
>> I would call the dynamic symbol ___riscv_hwprobe, not
>> __riscv_hwprobe_weak.  The weak property is an implementation detail in
>> the C file that goes into libc_nonshared.  And ___riscv_hwprobe does not
>> need to be declared in an installed header, I think it's fine to offer
>> __riscv_hwprobe only.
>
> Ok. Just to reiterate what I'm planning to export to users, I've got
> __riscv_hwprobe() as a dynamic function from libc that non-ifunc
> callers can use without the overhead of checking the weak symbol, as
> well as __riscv_hwprobe_early() that ifunc selectors can use
> pre-relocation. Based on your comment, I'll rename the
> __riscv_hwprobe_weak alias to ___riscv_hwprobe.
>
>>
>> Use of INTERNAL_SYSCALL is correct because without relocations, errno
>> will not work, but it needs to match across all implementations.  This
>> means using INTERNAL_VSYSCALL instead of INLINE_VSYSCALL.
>
> Oh yeah, otherwise the return values of __riscv_hwprobe_early() would
> suddenly change from -EINVAL to -1 once the symbol is available. So
> then we'll document errno is always left unchanged by this function.
>
>>
>> There needs to be manual entry that documents the exact interface
>> conventions.  If the function returns 0 on success (no positive return
>> values), it may make sense to negate the result value that comes back
>> from the kernel because that matches what some pthread interfaces use.
>> We probabl use the -EINVAL convention on some external API (it's a big
>> library), but I think it's on the unusual side.
>
> Correct, it's 0 on success or negative error code on failure. I guess
> you're binning it in with pthreads because those functions also return
> error codes directly? I can do that.
>
> After discussing with Palmer, based on the timing of things we're
> thinking it's probably too late to try and rush this into the upcoming
> release. There was also a suggestion internally to try and get to the
> vDSO pointer early, though I don't understand how/if this is possible.
> I need to dig more on that one. I may still spin this today if I can,
> but we'll see.

Ya, thanks.  Given how late we are in the cycle I think it's best to 
avoid rushing in any new ABI here, it's just too risky.

The vDSO idea would be to pre-populate HWCAP2 with a pointer to the full 
output of a riscv_hwprobe() run on all harts.  We'd store that data in 
the vDSO, but IIUC it'd fix the symbol lookup problem as it'd come in 
via the auxvec so we wouldn't need to poke around for symbols.  We 
probably need to think through some extensibility issues as well, but I 
think that doesn't change the core of the IFUNC discussion.

We'd talked about this before, but it was just in the context of 
performance (it's essentially a pre-cached version of the syscall).  Had 
I realized all these IFUNC-related symbol lookup headaches we probably 
would have done it the first time.  We probably would have ended up with 
it anyway, though, so it's not super scary.

The syscall would still be useful, both because it's sometimes easier to 
get at than HWCAP2 and because we can handle the more complex cases like 
heterogenous systems.  It'll probably still be worth allowing IFUNC 
resolvers to plumb into the syscall, but if we can make life easier for 
users who just want the simple case that seems valuable too.

> -Evan
  

On Thu, Jul 13, 2023 at 9:47 AM Palmer Dabbelt <palmer@rivosinc.com> wrote:
>
> On Thu, 13 Jul 2023 09:33:20 PDT (-0700), Evan Green wrote:
> > On Thu, Jul 13, 2023 at 12:07 AM Florian Weimer <fweimer@redhat.com> wrote:
> >>
> >> * Evan Green:
> >>
> >> > diff --git a/sysdeps/unix/sysv/linux/riscv/sys/hwprobe.h b/sysdeps/unix/sysv/linux/riscv/sys/hwprobe.h
> >> > index b27af5cb07..d739371806 100644
> >> > --- a/sysdeps/unix/sysv/linux/riscv/sys/hwprobe.h
> >> > +++ b/sysdeps/unix/sysv/linux/riscv/sys/hwprobe.h
> >> > @@ -67,6 +67,27 @@ extern int __riscv_hwprobe (struct riscv_hwprobe *pairs, size_t pair_count,
> >> >       __fortified_attr_access (__read_write__, 1, 2)
> >> >       __fortified_attr_access (__read_only__, 4, 3);
> >> >
> >> > +extern int __riscv_hwprobe_weak (struct riscv_hwprobe *pairs, size_t pair_count,
> >> > +                              size_t cpu_count, unsigned long int *cpus,
> >> > +                              unsigned int flags)
> >> > +     __THROW __nonnull ((1)) __wur
> >> > +     __fortified_attr_access (__read_write__, 1, 2)
> >> > +     __fortified_attr_access (__read_only__, 4, 3);
> >> > +
> >> > +# ifdef weak_extern
> >> > +weak_extern (__riscv_hwprobe_weak)
> >> > +# else
> >> > +#  pragma weak __riscv_hwprobe_weak
> >> > +# endif
> >> > +
> >> > +extern int __riscv_hwprobe_early (struct riscv_hwprobe *pairs, size_t pair_count,
> >> > +                               size_t cpu_count, unsigned long int *cpus,
> >> > +                               unsigned int flags)
> >> > +     __THROW __nonnull ((1)) __wur
> >> > +     __fortified_attr_access (__read_write__, 1, 2)
> >> > +     __fortified_attr_access (__read_only__, 4, 3)
> >> > +     __attribute__ ((__visibility__ ("hidden")));
> >> > +
> >> >  __END_DECLS
> >> >
> >> >  #endif /* sys/hwprobe.h */
> >>
> >> I would call the dynamic symbol ___riscv_hwprobe, not
> >> __riscv_hwprobe_weak.  The weak property is an implementation detail in
> >> the C file that goes into libc_nonshared.  And ___riscv_hwprobe does not
> >> need to be declared in an installed header, I think it's fine to offer
> >> __riscv_hwprobe only.
> >
> > Ok. Just to reiterate what I'm planning to export to users, I've got
> > __riscv_hwprobe() as a dynamic function from libc that non-ifunc
> > callers can use without the overhead of checking the weak symbol, as
> > well as __riscv_hwprobe_early() that ifunc selectors can use
> > pre-relocation. Based on your comment, I'll rename the
> > __riscv_hwprobe_weak alias to ___riscv_hwprobe.
> >
> >>
> >> Use of INTERNAL_SYSCALL is correct because without relocations, errno
> >> will not work, but it needs to match across all implementations.  This
> >> means using INTERNAL_VSYSCALL instead of INLINE_VSYSCALL.
> >
> > Oh yeah, otherwise the return values of __riscv_hwprobe_early() would
> > suddenly change from -EINVAL to -1 once the symbol is available. So
> > then we'll document errno is always left unchanged by this function.
> >
> >>
> >> There needs to be manual entry that documents the exact interface
> >> conventions.  If the function returns 0 on success (no positive return
> >> values), it may make sense to negate the result value that comes back
> >> from the kernel because that matches what some pthread interfaces use.
> >> We probabl use the -EINVAL convention on some external API (it's a big
> >> library), but I think it's on the unusual side.
> >
> > Correct, it's 0 on success or negative error code on failure. I guess
> > you're binning it in with pthreads because those functions also return
> > error codes directly? I can do that.
> >
> > After discussing with Palmer, based on the timing of things we're
> > thinking it's probably too late to try and rush this into the upcoming
> > release. There was also a suggestion internally to try and get to the
> > vDSO pointer early, though I don't understand how/if this is possible.
> > I need to dig more on that one. I may still spin this today if I can,
> > but we'll see.
>
> Ya, thanks.  Given how late we are in the cycle I think it's best to
> avoid rushing in any new ABI here, it's just too risky.
>
> The vDSO idea would be to pre-populate HWCAP2 with a pointer to the full
> output of a riscv_hwprobe() run on all harts.  We'd store that data in
> the vDSO, but IIUC it'd fix the symbol lookup problem as it'd come in
> via the auxvec so we wouldn't need to poke around for symbols.  We
> probably need to think through some extensibility issues as well, but I
> think that doesn't change the core of the IFUNC discussion.

Palmer and I chatted a bit more about this idea. The evolution of it
was to define something like AT_RISCV_HWPROBE_FUNC whose value is a
pointer to the vDSO function. The __riscv_hwprobe_early() in
libc_nonshared can then just get the auxval and call it, with similar
semantics as we have here of not setting errno, etc.

Florian, one question about your other possible solution suggestion,
reproduced below:

> You could pass the function pointer to the IFUNC resolver
> (which may require a marker symbol and GCC changes).

I'm unsure what a marker symbol is, so I'm having trouble following
this suggestion. I'd like to understand what that suggestion was,
especially if you think it's better than what we're planning above.

-Evan
  

* Evan Green:

> Palmer and I chatted a bit more about this idea. The evolution of it
> was to define something like AT_RISCV_HWPROBE_FUNC whose value is a
> pointer to the vDSO function. The __riscv_hwprobe_early() in
> libc_nonshared can then just get the auxval and call it, with similar
> semantics as we have here of not setting errno, etc.

I don't think it can access the auxiliary vector without relocations.

> Florian, one question about your other possible solution suggestion,
> reproduced below:
>
>> You could pass the function pointer to the IFUNC resolver
>> (which may require a marker symbol and GCC changes).
>
> I'm unsure what a marker symbol is, so I'm having trouble following
> this suggestion. I'd like to understand what that suggestion was,
> especially if you think it's better than what we're planning above.

POWER does this to signal that a binary needs data in the TCB for use by
IFUNC resolvers:

/* Newer LIBCs explicitly export this symbol to declare that they provide
   the AT_PLATFORM and AT_HWCAP/AT_HWCAP2 values in the TCB.  We emit a
   reference to this symbol whenever we expand a CPU builtin, so that
   we never link against an old LIBC.  */
const char *tcb_verification_symbol = "__parse_hwcap_and_convert_at_platform";

You are already passing an additional unused argument to IFUNC
resolvers, though:

static inline ElfW(Addr)
__attribute ((always_inline))
elf_ifunc_invoke (ElfW(Addr) addr)
{
  /* The second argument is a void pointer to preserve the extension
     fexibility.  */
  return ((ElfW(Addr) (*) (uint64_t, void *)) (addr))
         (GLRO(dl_hwcap), NULL);
}

So you could pass something there (like the address of __riscv_hwprobe),
and add a third argument (NULL for now) for future extensions.

This change does not even have ABI impact, applications just need to
remember to check the function pointer for NULL.  I haven't check if GCC
changes are needed before the compiler accepts the additional argument
in IFUNC resolvers.

Thanks,
Florian