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

[Palmer Dabbelt <palmer@rivosinc.com>]

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