Re: [PATCH v3 2/7] RISC-V: Add a syscall for HW probing

[Heiko Stübner]

Am Donnerstag, 30. März 2023, 20:30:29 CEST schrieb Evan Green:
> On Thu, Feb 23, 2023 at 2:06 AM Arnd Bergmann <arnd@xxxxxxxx> wrote:
> >
> > On Tue, Feb 21, 2023, at 20:08, Evan Green wrote:
> > > We don't have enough space for these all in ELF_HWCAP{,2} and there's no
> > > system call that quite does this, so let's just provide an arch-specific
> > > one to probe for hardware capabilities.  This currently just provides
> > > m{arch,imp,vendor}id, but with the key-value pairs we can pass more in
> > > the future.
> > >
> > > Co-developed-by: Palmer Dabbelt <palmer@xxxxxxxxxxxx>
> > > Signed-off-by: Palmer Dabbelt <palmer@xxxxxxxxxxxx>
> > > Signed-off-by: Evan Green <evan@xxxxxxxxxxxx>
> >
> > I'm still skeptical about the need for a custom syscall interface here.
> > I had not looked at the interface so far, but there are a few things
> > that stick out:
> >
> > > +RISC-V Hardware Probing Interface
> > > +---------------------------------
> > > +
> > > +The RISC-V hardware probing interface is based around a single
> > > syscall, which
> > > +is defined in <asm/hwprobe.h>::
> > > +
> > > +    struct riscv_hwprobe {
> > > +        __s64 key;
> > > +        __u64 value;
> > > +    };
> >
> > The way this is defined, the kernel will always have to know
> > about the specific set of features, it can't just forward
> > unknown features to user space after probing them from an
> > architectured hardware interface or from DT.
> 
> You're correct that this interface wasn't intended to have usermode
> come in with augmented data or additional key/value pairs. This was
> purely meant to provide access to the kernel's repository of
> architectural and microarchitectural details. If usermode wants to
> provide extra info in this same form, maybe they could wrap this
> interface.
>
> > If 'key' is just an enumerated value with a small number of
> > possible values, I don't see anything wrong with using elf
> > aux data. I understand it's hard to know how many keys
> > might be needed in the long run, from the way you define
> > the key/value pairs here, I would expect it to have a lot
> > of the same limitations that the aux data has, except for
> > a few bytes to be copied.
> 
> Correct, this makes allocating bits out of here cheaper by not
> requiring that we actively copy them into every new process forever.
> You're right that the aux vector would work as well, but the thinking
> behind this series was that an interface like this might be better for
> an architecture as extensible as risc-v.

What would be the ramifications of defining some sort of vdso-like
data-structure and just putting the address into AT_HWCAP2 ?
(similar to what vdso does) - that could then even be re-usable
with other OS kernels.

And would also save declaring numerous new AT_* keys.


Because there are already nearly 130 standard extensions and vendors
are allowed to defines their own as well, and we will probably also want
to tell userspace about them.


Heiko


> > > +    long sys_riscv_hwprobe(struct riscv_hwprobe *pairs, size_t
> > > pair_count,
> > > +                           size_t cpu_count, cpu_set_t *cpus,
> > > +                           unsigned long flags);
> >
> > The cpu set argument worries me more: there should never be a
> > need to optimize for broken hardware that has an asymmetric set
> > of features. Just let the kernel figure out the minimum set
> > of features that works across all CPUs and report that like we
> > do with HWCAP. If there is a SoC that is so broken that it has
> > important features on a subset of cores that some user might
> > actually want to rely on, then have them go through the slow
> > sysfs interface for probing the CPUs indidually, but don't make
> > the broken case easier at the expense of normal users that
> > run on working hardware.
> 
> I'm not so sure. While I agree with you for major classes of features
> (eg one CPU has floating point support but another does not), I expect
> these bits to contain more subtle details as well, which might vary
> across asymmetric implementations without breaking ABI compatibility
> per-se. Maybe some vendor has implemented exotic video decoding
> acceleration instructions that only work on the big core. Or maybe the
> big cores support v3.1 of some extension (where certain things run
> faster), but the little cores only have v3.0, where it's a little
> slower. Certain apps would likely want to know these things so they
> can allocate their work optimally across cores.
> 
> >
> > > +asmlinkage long sys_riscv_hwprobe(uintptr_t, uintptr_t, uintptr_t,
> > > uintptr_t,
> > > +                               uintptr_t, uintptr_t);
> >
> > Why 'uintptr_t' rather than the correct type?
> 
> Fixed.
> -Evan
>