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

[Evan Green]

On Thu, Mar 30, 2023 at 1:20 PM Heiko Stübner <heiko@xxxxxxxxx> wrote:
>
> 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.

Yeah I mulled that approach over a bit originally as well. The
downside is the vdso data then becomes part of the ABI. So you can
never change the layout of that vdso data, and you lose the ability to
change what gets cached in the vdso versus what bounces up to the
syscall. To poach a scenario from a glibc discussion underway, if for
instance cpu hotplug comes along and you need to invalidate some
portion of your cached data, that's easy when there's a function in
front of it, but difficult if apps are crawling the data themselves.

130 extensions is certainly a lot, and illustrates how auxvec may get
out of hand quickly. One nice thing about this mechanism (though other
approaches share this trait) is that it's agnostic of where the data
comes from. In other words, it doesn't require that data come from the
DT, or alternative.c, etc, as long as the kernel can access it and
plunk it in a key/value store.
-Evan

>
>
> 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
> >
>
>
>
>