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 >