| Message ID | 20220222204811.2281949-1-atishp@rivosinc.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Provide a fraemework for RISC-V ISA extensions | expand |
On Tue, 22 Feb 2022 12:48:05 PST (-0800), Atish Patra wrote: > This series implements a generic framework to parse multi-letter ISA > extensions. This series is based on Tsukasa's v3 isa extension improvement > series[1]. I have fixed few bugs and improved comments from that series > (PATCH1-3). I have not used PATCH 4 from that series as we are not using > ISA extension versioning as of now. We can add that later if required. > > PATCH 4 allows the probing of multi-letter extensions via a macro. > It continues to use the common isa extensions between all the harts. > Thus hetergenous hart systems will only see the common ISA extensions. > > PATCH 6 improves the /proc/cpuinfo interface for the available ISA extensions > via /proc/cpuinfo. > > Here is the example output of /proc/cpuinfo: > (with debug patches in Qemu and Linux kernel) > > # cat /proc/cpuinfo > processor : 0 > hart : 0 > isa : rv64imafdch > isa-ext : svpbmt svnapot svinval I know it might seem a bit pedantic, but I really don't want to introduce a new format for encoding ISA extensions -- doubly so if this is the only way we're giving this info to userspace, as then we're just asking folks to turn this into a defacto ABI. Every time we try to do something that's sort of like an ISA string but not exactly what's in the spec we end up getting burned, and while I don't see a specific way that could happen here that's what's happened so many times before I just don't want to risk it. I've gone ahead and removed some of this information (isa-ext, and all the single-letter extensions we can't properly turn on yet) from /proc/cpuinfo, modifying the last patch to do so. It's at palmer/riscv-isa, LMK if that's OK. I'm not opposed to doing something here, I just really don't want to rush into it as we've already got enough complexity around the various flavors of ISA strings. I don't see a big pressing need to provide this information to userspace, but having the rest of this sorted out is great (and there's some dependencies on this) so I'd prefer to just stick to what we know is good. > mmu : sv48 > > processor : 1 > hart : 1 > isa : rv64imafdch > isa-ext : svpbmt svnapot svinval > mmu : sv48 > > processor : 2 > hart : 2 > isa : rv64imafdch > isa-ext : svpbmt svnapot svinval > mmu : sv48 > > processor : 3 > hart : 3 > isa : rv64imafdch > isa-ext : svpbmt svnapot svinval > mmu : sv48 > > Anybody adding support for any new multi-letter extensions should add an > entry to the riscv_isa_ext_id and the isa extension array. > E.g. The patch[2] adds the support for various ISA extensions. > > [1] https://lore.kernel.org/all/0f568515-a05e-8204-aae3-035975af3ee8@irq.a4lg.com/T/ > [2] https://github.com/atishp04/linux/commit/e9e240c9a854dceb434ceb53bdbe82a657bee5f2 > > Changes from v4->v5: > 1. Improved the /proc/cpuinfo to include only valid & enabled extensions > 2. Improved the multi-letter parsing by skipping the 'su' modes generated in > Qemu as suggested by Tsukasa. > > Changes from v3->v4: > 1. Changed temporary variable for current hart isa to a bitmap > 2. Added reviewed-by tags. > 3. Improved comments > > Changes from v2->v3: > 1. Updated comments to mark clearly a fix required for Qemu only. > 2. Fixed a bug where the 1st multi-letter extension can be present without _ > 3. Added Tested by tags. > > Changes from v1->v2: > 1. Instead of adding a separate DT property use the riscv,isa property. > 2. Based on Tsukasa's v3 isa extension improvement series. > > Atish Patra (3): > RISC-V: Implement multi-letter ISA extension probing framework > RISC-V: Do no continue isa string parsing without correct XLEN > RISC-V: Improve /proc/cpuinfo output for ISA extensions > > Tsukasa OI (3): > RISC-V: Correctly print supported extensions > RISC-V: Minimal parser for "riscv, isa" strings > RISC-V: Extract multi-letter extension names from "riscv, isa" > > arch/riscv/include/asm/hwcap.h | 25 +++++++ > arch/riscv/kernel/cpu.c | 51 ++++++++++++- > arch/riscv/kernel/cpufeature.c | 130 +++++++++++++++++++++++++++------ > 3 files changed, 183 insertions(+), 23 deletions(-)
On Thu, Mar 10, 2022 at 3:50 PM Palmer Dabbelt <palmer@dabbelt.com> wrote: > > On Tue, 22 Feb 2022 12:48:05 PST (-0800), Atish Patra wrote: > > This series implements a generic framework to parse multi-letter ISA > > extensions. This series is based on Tsukasa's v3 isa extension improvement > > series[1]. I have fixed few bugs and improved comments from that series > > (PATCH1-3). I have not used PATCH 4 from that series as we are not using > > ISA extension versioning as of now. We can add that later if required. > > > > PATCH 4 allows the probing of multi-letter extensions via a macro. > > It continues to use the common isa extensions between all the harts. > > Thus hetergenous hart systems will only see the common ISA extensions. > > > > PATCH 6 improves the /proc/cpuinfo interface for the available ISA extensions > > via /proc/cpuinfo. > > > > Here is the example output of /proc/cpuinfo: > > (with debug patches in Qemu and Linux kernel) > > > > # cat /proc/cpuinfo > > processor : 0 > > hart : 0 > > isa : rv64imafdch > > isa-ext : svpbmt svnapot svinval > > I know it might seem a bit pedantic, but I really don't want to > introduce a new format for encoding ISA extensions -- doubly so if this > is the only way we're giving this info to userspace, as then we're just > asking folks to turn this into a defacto ABI. Every time we try to do > something that's sort of like an ISA string but not exactly what's in > the spec we end up getting burned, and while I don't see a specific way I agree that this is an ABI change/improvement which is impossible to modify later. However, this is a Linux specific ABI. Do you think the RISC-V spec will ever say anything about how /proc/cpuinfo is shown to the user ? and we do have similar precedence in other arch /proc/cpuinfo output in x86: flags : fpu vme .... arch_capabilities vmx flags : vnmi preemption_timer ..tsc_scaling /proc/cpuinfo output in arm64: Features : fp asimd evtstrm aes pmull sha1 sha2 crc32 > that could happen here that's what's happened so many times before I > just don't want to risk it. > > I've gone ahead and removed some of this information (isa-ext, and all > the single-letter extensions we can't properly turn on yet) from > /proc/cpuinfo, modifying the last patch to do so. It's at > palmer/riscv-isa, LMK if that's OK. > Few changes required on your tree: riscv_isa_ext_data definition is no longer required and this should be to show hypervisor extension: +static const char *base_riscv_exts = "imafdch"; > I'm not opposed to doing something here, I just really don't want to > rush into it as we've already got enough complexity around the various > flavors of ISA strings. I don't see a big pressing need to provide this > information to userspace, but having the rest of this sorted out is > great (and there's some dependencies on this) so I'd prefer to just > stick to what we know is good. > "isa-ext" row is kind of helpful to inform the developer to verify that a specific extension is available without looking at extension specific dmesg logs. It proved to be useful while developing sstc/sscofpmf. Is it better if we just dump the entire ISA string as before so that we know which extensions are available at least ? > > mmu : sv48 > > > > processor : 1 > > hart : 1 > > isa : rv64imafdch > > isa-ext : svpbmt svnapot svinval > > mmu : sv48 > > > > processor : 2 > > hart : 2 > > isa : rv64imafdch > > isa-ext : svpbmt svnapot svinval > > mmu : sv48 > > > > processor : 3 > > hart : 3 > > isa : rv64imafdch > > isa-ext : svpbmt svnapot svinval > > mmu : sv48 > > > > Anybody adding support for any new multi-letter extensions should add an > > entry to the riscv_isa_ext_id and the isa extension array. > > E.g. The patch[2] adds the support for various ISA extensions. > > > > [1] https://lore.kernel.org/all/0f568515-a05e-8204-aae3-035975af3ee8@irq.a4lg.com/T/ > > [2] https://github.com/atishp04/linux/commit/e9e240c9a854dceb434ceb53bdbe82a657bee5f2 > > > > Changes from v4->v5: > > 1. Improved the /proc/cpuinfo to include only valid & enabled extensions > > 2. Improved the multi-letter parsing by skipping the 'su' modes generated in > > Qemu as suggested by Tsukasa. > > > > Changes from v3->v4: > > 1. Changed temporary variable for current hart isa to a bitmap > > 2. Added reviewed-by tags. > > 3. Improved comments > > > > Changes from v2->v3: > > 1. Updated comments to mark clearly a fix required for Qemu only. > > 2. Fixed a bug where the 1st multi-letter extension can be present without _ > > 3. Added Tested by tags. > > > > Changes from v1->v2: > > 1. Instead of adding a separate DT property use the riscv,isa property. > > 2. Based on Tsukasa's v3 isa extension improvement series. > > > > Atish Patra (3): > > RISC-V: Implement multi-letter ISA extension probing framework > > RISC-V: Do no continue isa string parsing without correct XLEN > > RISC-V: Improve /proc/cpuinfo output for ISA extensions > > > > Tsukasa OI (3): > > RISC-V: Correctly print supported extensions > > RISC-V: Minimal parser for "riscv, isa" strings > > RISC-V: Extract multi-letter extension names from "riscv, isa" > > > > arch/riscv/include/asm/hwcap.h | 25 +++++++ > > arch/riscv/kernel/cpu.c | 51 ++++++++++++- > > arch/riscv/kernel/cpufeature.c | 130 +++++++++++++++++++++++++++------ > > 3 files changed, 183 insertions(+), 23 deletions(-)
Στις 2022-03-11 02:21, Atish Kumar Patra έγραψε: > On Thu, Mar 10, 2022 at 3:50 PM Palmer Dabbelt <palmer@dabbelt.com> > wrote: >> >> On Tue, 22 Feb 2022 12:48:05 PST (-0800), Atish Patra wrote: >> > This series implements a generic framework to parse multi-letter ISA >> > extensions. This series is based on Tsukasa's v3 isa extension improvement >> > series[1]. I have fixed few bugs and improved comments from that series >> > (PATCH1-3). I have not used PATCH 4 from that series as we are not using >> > ISA extension versioning as of now. We can add that later if required. >> > >> > PATCH 4 allows the probing of multi-letter extensions via a macro. >> > It continues to use the common isa extensions between all the harts. >> > Thus hetergenous hart systems will only see the common ISA extensions. >> > >> > PATCH 6 improves the /proc/cpuinfo interface for the available ISA extensions >> > via /proc/cpuinfo. >> > >> > Here is the example output of /proc/cpuinfo: >> > (with debug patches in Qemu and Linux kernel) >> > >> > # cat /proc/cpuinfo >> > processor : 0 >> > hart : 0 >> > isa : rv64imafdch >> > isa-ext : svpbmt svnapot svinval >> >> I know it might seem a bit pedantic, but I really don't want to >> introduce a new format for encoding ISA extensions -- doubly so if >> this >> is the only way we're giving this info to userspace, as then we're >> just >> asking folks to turn this into a defacto ABI. Every time we try to do >> something that's sort of like an ISA string but not exactly what's in >> the spec we end up getting burned, and while I don't see a specific >> way > > I agree that this is an ABI change/improvement which is impossible to > modify later. > However, this is a Linux specific ABI. Do you think the RISC-V spec > will ever say anything about how /proc/cpuinfo is shown to the user ? > Actually there was a discussion on chairs at some point on how isa extensions will be represented as a single string. If I recall correctly they wanted a way to compare features between implementations so this was something the user should be able to read as well. I'm ccing Philipp from the Software HC in case he has more details on this. I also believe we need to discuss this a bit further, also I thought we agreed that having everything as a single string (riscv-isa) on the device tree doesn't scale, there were some other suggestions regarding for example mmu extensions being declared inside an mmu sub-node etc. This patch series will not only make it hard to change /proc/cpuinfo output in the future, but also establishes a device-tree binding for all isa extensions through the riscv-isa string that we also won't be able to modify later on. Regards, Nick
On Fri, Mar 11, 2022 at 6:13 PM Nick Kossifidis <mick@ics.forth.gr> wrote: > > Στις 2022-03-11 02:21, Atish Kumar Patra έγραψε: > > On Thu, Mar 10, 2022 at 3:50 PM Palmer Dabbelt <palmer@dabbelt.com> > > wrote: > >> > >> On Tue, 22 Feb 2022 12:48:05 PST (-0800), Atish Patra wrote: > >> > This series implements a generic framework to parse multi-letter ISA > >> > extensions. This series is based on Tsukasa's v3 isa extension improvement > >> > series[1]. I have fixed few bugs and improved comments from that series > >> > (PATCH1-3). I have not used PATCH 4 from that series as we are not using > >> > ISA extension versioning as of now. We can add that later if required. > >> > > >> > PATCH 4 allows the probing of multi-letter extensions via a macro. > >> > It continues to use the common isa extensions between all the harts. > >> > Thus hetergenous hart systems will only see the common ISA extensions. > >> > > >> > PATCH 6 improves the /proc/cpuinfo interface for the available ISA extensions > >> > via /proc/cpuinfo. > >> > > >> > Here is the example output of /proc/cpuinfo: > >> > (with debug patches in Qemu and Linux kernel) > >> > > >> > # cat /proc/cpuinfo > >> > processor : 0 > >> > hart : 0 > >> > isa : rv64imafdch > >> > isa-ext : svpbmt svnapot svinval > >> > >> I know it might seem a bit pedantic, but I really don't want to > >> introduce a new format for encoding ISA extensions -- doubly so if > >> this > >> is the only way we're giving this info to userspace, as then we're > >> just > >> asking folks to turn this into a defacto ABI. Every time we try to do > >> something that's sort of like an ISA string but not exactly what's in > >> the spec we end up getting burned, and while I don't see a specific > >> way > > > > I agree that this is an ABI change/improvement which is impossible to > > modify later. > > However, this is a Linux specific ABI. Do you think the RISC-V spec > > will ever say anything about how /proc/cpuinfo is shown to the user ? > > > > Actually there was a discussion on chairs at some point on how isa > extensions will be represented as a single string. If I recall correctly > they wanted a way to compare features between implementations so this > was something the user should be able to read as well. I'm ccing Philipp > from the Software HC in case he has more details on this. > > I also believe we need to discuss this a bit further, also I thought we > agreed that having everything as a single string (riscv-isa) on the > device tree doesn't scale, there were some other suggestions regarding > for example mmu extensions being declared inside an mmu sub-node etc. > This patch series will not only make it hard to change /proc/cpuinfo > output in the future, but also establishes a device-tree binding for all > isa extensions through the riscv-isa string that we also won't be able > to modify later on. Initially we can just follow the ISA string approach because this is what RISC-V unpric spec defines. Specifying ISA extensions via DT or ACPI, can be incrementally done in future. We have a lot of patches (Svpbmt, Sstc, Scofpmf, Zcboxyz, etc) blocked because we don't have a way to detect multi-letter extensions in Linux. Regards, Anup > > Regards, > Nick > > _______________________________________________ > linux-riscv mailing list > linux-riscv@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-riscv