From: "Arnd Bergmann" <arnd@arndb.de> To: "Evan Green" <evan@rivosinc.com>, "Palmer Dabbelt" <palmer@rivosinc.com> Cc: "Heiko Stübner" <heiko@sntech.de>, "Conor Dooley" <conor@kernel.org>, slewis@rivosinc.com, "Vineet Gupta" <vineetg@rivosinc.com>, "Albert Ou" <aou@eecs.berkeley.edu>, "Andrew Bresticker" <abrestic@rivosinc.com>, "Andrew Jones" <ajones@ventanamicro.com>, "Anup Patel" <apatel@ventanamicro.com>, "Atish Patra" <atishp@rivosinc.com>, "Bagas Sanjaya" <bagasdotme@gmail.com>, "Celeste Liu" <coelacanthus@outlook.com>, "Conor.Dooley" <conor.dooley@microchip.com>, guoren <guoren@kernel.org>, "Jonathan Corbet" <corbet@lwn.net>, "Niklas Cassel" <niklas.cassel@wdc.com>, "Palmer Dabbelt" <palmer@dabbelt.com>, "Paul Walmsley" <paul.walmsley@sifive.com>, "Randy Dunlap" <rdunlap@infradead.org>, "Ruizhe Pan" <c141028@gmail.com>, "Sunil V L" <sunilvl@ventanamicro.com>, "Tobias Klauser" <tklauser@distanz.ch>, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, linux-riscv@lists.infradead.org Subject: Re: [PATCH v3 2/7] RISC-V: Add a syscall for HW probing Date: Thu, 23 Feb 2023 11:06:11 +0100 [thread overview] Message-ID: <605fb2fd-bda2-4922-92bf-e3e416d54398@app.fastmail.com> (raw) In-Reply-To: <20230221190858.3159617-3-evan@rivosinc.com> 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@rivosinc.com> > Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com> > Signed-off-by: Evan Green <evan@rivosinc.com> 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. 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. > + 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. > +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? Arnd
WARNING: multiple messages have this Message-ID (diff)
From: "Arnd Bergmann" <arnd@arndb.de> To: "Evan Green" <evan@rivosinc.com>, "Palmer Dabbelt" <palmer@rivosinc.com> Cc: "Niklas Cassel" <niklas.cassel@wdc.com>, "Heiko Stübner" <heiko@sntech.de>, linux-doc@vger.kernel.org, "Andrew Bresticker" <abrestic@rivosinc.com>, "Atish Patra" <atishp@rivosinc.com>, "Conor.Dooley" <conor.dooley@microchip.com>, "Celeste Liu" <coelacanthus@outlook.com>, slewis@rivosinc.com, "Bagas Sanjaya" <bagasdotme@gmail.com>, linux-riscv@lists.infradead.org, "Jonathan Corbet" <corbet@lwn.net>, "Tobias Klauser" <tklauser@distanz.ch>, "Andrew Jones" <ajones@ventanamicro.com>, "Albert Ou" <aou@eecs.berkeley.edu>, "Vineet Gupta" <vineetg@rivosinc.com>, "Paul Walmsley" <paul.walmsley@sifive.com>, "Ruizhe Pan" <c141028@gmail.com>, "Anup Patel" <apatel@ventanamicro.com>, "Randy Dunlap" <rdunlap@infradead.org>, linux-kernel@vger.kernel.org, "Conor Dooley" <conor@kernel.org>, "Palmer Dabbelt" <palmer@dabbelt.com>, guoren <guoren@kernel.org> Subject: Re: [PATCH v3 2/7] RISC-V: Add a syscall for HW probing Date: Thu, 23 Feb 2023 11:06:11 +0100 [thread overview] Message-ID: <605fb2fd-bda2-4922-92bf-e3e416d54398@app.fastmail.com> (raw) In-Reply-To: <20230221190858.3159617-3-evan@rivosinc.com> 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@rivosinc.com> > Signed-off-by: Palmer Dabbelt <palmer@rivosinc.com> > Signed-off-by: Evan Green <evan@rivosinc.com> 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. 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. > + 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. > +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? Arnd _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv
next prev parent reply other threads:[~2023-02-23 10:06 UTC|newest] Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-02-21 19:08 [PATCH v3 0/7] RISC-V Hardware Probing User Interface Evan Green 2023-02-21 19:08 ` Evan Green 2023-02-21 19:08 ` [PATCH v3 1/7] RISC-V: Move struct riscv_cpuinfo to new header Evan Green 2023-02-21 19:08 ` Evan Green 2023-02-21 19:08 ` [PATCH v3 2/7] RISC-V: Add a syscall for HW probing Evan Green 2023-02-21 19:08 ` Evan Green 2023-02-23 10:06 ` Arnd Bergmann [this message] 2023-02-23 10:06 ` Arnd Bergmann 2023-03-30 18:30 ` Evan Green 2023-03-30 18:30 ` Evan Green 2023-03-30 20:20 ` Heiko Stübner 2023-03-30 20:20 ` Heiko Stübner 2023-03-30 21:24 ` Evan Green 2023-03-30 21:24 ` Evan Green 2023-03-31 13:21 ` Arnd Bergmann 2023-03-31 13:21 ` Arnd Bergmann 2023-03-31 17:51 ` Evan Green 2023-03-31 17:51 ` Evan Green 2023-02-27 22:19 ` Conor Dooley 2023-02-27 22:19 ` Conor Dooley 2023-02-21 19:08 ` [PATCH v3 3/7] RISC-V: hwprobe: Add support for RISCV_HWPROBE_BASE_BEHAVIOR_IMA Evan Green 2023-02-21 19:08 ` Evan Green 2023-02-27 22:47 ` Conor Dooley 2023-02-27 22:47 ` Conor Dooley 2023-03-03 0:56 ` Evan Green 2023-03-03 0:56 ` Evan Green 2023-02-21 19:08 ` [PATCH v3 4/7] dt-bindings: Add RISC-V misaligned access performance Evan Green 2023-02-21 19:08 ` Evan Green 2023-02-27 22:57 ` Conor Dooley 2023-02-27 22:57 ` Conor Dooley 2023-02-28 14:57 ` Rob Herring 2023-02-28 14:57 ` Rob Herring 2023-02-21 19:08 ` [PATCH v3 5/7] RISC-V: hwprobe: Support probing of " Evan Green 2023-02-21 19:08 ` Evan Green 2023-02-22 9:39 ` Joe Perches 2023-02-22 9:39 ` Joe Perches 2023-02-27 23:14 ` Conor Dooley 2023-02-27 23:14 ` Conor Dooley 2023-02-21 19:08 ` [PATCH v3 6/7] selftests: Test the new RISC-V hwprobe interface Evan Green 2023-02-21 19:08 ` Evan Green 2023-02-21 22:45 ` Mark Brown 2023-02-21 22:45 ` Mark Brown 2023-02-21 19:08 ` [PATCH v3 7/7] RISC-V: Add hwprobe vDSO function and data Evan Green 2023-02-21 19:08 ` Evan Green 2023-02-21 21:17 ` kernel test robot 2023-02-21 21:17 ` kernel test robot 2023-02-22 6:55 ` kernel test robot 2023-02-22 6:55 ` kernel test robot
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=605fb2fd-bda2-4922-92bf-e3e416d54398@app.fastmail.com \ --to=arnd@arndb.de \ --cc=abrestic@rivosinc.com \ --cc=ajones@ventanamicro.com \ --cc=aou@eecs.berkeley.edu \ --cc=apatel@ventanamicro.com \ --cc=atishp@rivosinc.com \ --cc=bagasdotme@gmail.com \ --cc=c141028@gmail.com \ --cc=coelacanthus@outlook.com \ --cc=conor.dooley@microchip.com \ --cc=conor@kernel.org \ --cc=corbet@lwn.net \ --cc=evan@rivosinc.com \ --cc=guoren@kernel.org \ --cc=heiko@sntech.de \ --cc=linux-doc@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-riscv@lists.infradead.org \ --cc=niklas.cassel@wdc.com \ --cc=palmer@dabbelt.com \ --cc=palmer@rivosinc.com \ --cc=paul.walmsley@sifive.com \ --cc=rdunlap@infradead.org \ --cc=slewis@rivosinc.com \ --cc=sunilvl@ventanamicro.com \ --cc=tklauser@distanz.ch \ --cc=vineetg@rivosinc.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.