mbox series

[v3,0/6] Provide a fraemework for RISC-V ISA extensions

Message ID 20220215090211.911366-1-atishp@rivosinc.com (mailing list archive)
Headers show
Series Provide a fraemework for RISC-V ISA extensions | expand

Message

Atish Kumar Patra Feb. 15, 2022, 9:02 a.m. UTC 
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		: rv64imafdcsu
isa-ext		: sstc,sscofpmf
mmu		: sv48

processor	: 1
hart		: 1
isa		: rv64imafdcsu
isa-ext		: sstc,sscofpmf
mmu		: sv48

processor	: 2
hart		: 2
isa		: rv64imafdcsu
isa-ext		: sstc,sscofpmf
mmu		: sv48

processor	: 3
hart		: 3
isa		: rv64imafdcsu
isa-ext		: sstc,sscofpmf
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/dc6f9200033bb5a72d8fd1a179bb272c6ade17e6 


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        |  44 ++++++++++-
arch/riscv/kernel/cpufeature.c | 130 ++++++++++++++++++++++++++++-----
3 files changed, 178 insertions(+), 21 deletions(-)

--
2.30.2

Comments

Jisheng Zhang Feb. 15, 2022, 3:56 p.m. UTC | #1 
On Tue, Feb 15, 2022 at 01:02:05AM -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		: rv64imafdcsu
> isa-ext		: sstc,sscofpmf
> mmu		: sv48
> 
> processor	: 1
> hart		: 1
> isa		: rv64imafdcsu
> isa-ext		: sstc,sscofpmf
> mmu		: sv48
> 
> processor	: 2
> hart		: 2
> isa		: rv64imafdcsu
> isa-ext		: sstc,sscofpmf
> mmu		: sv48
> 
> processor	: 3
> hart		: 3
> isa		: rv64imafdcsu
> isa-ext		: sstc,sscofpmf
> 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.

Hi Atish,

Thanks for this series. I'm thinking cpu features VS ISA extenstions.
I'm converting the sv48 to static key:
https://lore.kernel.org/linux-riscv/20220125165036.987-1-jszhang@kernel.org/

Previously, I thought the SV48 as a cpu feature, and there will be
more and more cpu features, so I implemented an unified static key
mechanism for CPU features. But after reading this series, I think
I may need to rebase(even reimplement) the above patch to your series.
But I'm a bit confused by CPU features VS ISA extenstions now:

1. Is cpu feature  == ISA extension?

2. Is SV48 considered as ISA extension?
If yes, now SV48 or not is determined during runtime, but current ISA
extensions seem parsed from DT. So how to support those ISA extensions
which can be determined during runtime?

Could you please share your thought?

Thanks
Atish Kumar Patra Feb. 15, 2022, 7:06 p.m. UTC | #2 
On Tue, Feb 15, 2022 at 8:04 AM Jisheng Zhang <jszhang@kernel.org> wrote:
>
> On Tue, Feb 15, 2022 at 01:02:05AM -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           : rv64imafdcsu
> > isa-ext               : sstc,sscofpmf
> > mmu           : sv48
> >
> > processor     : 1
> > hart          : 1
> > isa           : rv64imafdcsu
> > isa-ext               : sstc,sscofpmf
> > mmu           : sv48
> >
> > processor     : 2
> > hart          : 2
> > isa           : rv64imafdcsu
> > isa-ext               : sstc,sscofpmf
> > mmu           : sv48
> >
> > processor     : 3
> > hart          : 3
> > isa           : rv64imafdcsu
> > isa-ext               : sstc,sscofpmf
> > 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.
>
> Hi Atish,
>
> Thanks for this series. I'm thinking cpu features VS ISA extenstions.
> I'm converting the sv48 to static key:
> https://lore.kernel.org/linux-riscv/20220125165036.987-1-jszhang@kernel.org/
>
> Previously, I thought the SV48 as a cpu feature, and there will be
> more and more cpu features, so I implemented an unified static key
> mechanism for CPU features. But after reading this series, I think
> I may need to rebase(even reimplement) the above patch to your series.
> But I'm a bit confused by CPU features VS ISA extenstions now:
>
> 1. Is cpu feature  == ISA extension?
>
> 2. Is SV48 considered as ISA extension?
> If yes, now SV48 or not is determined during runtime, but current ISA
> extensions seem parsed from DT. So how to support those ISA extensions
> which can be determined during runtime?
>
> Could you please share your thought?
>

Here are my two cents:

I think the cpu feature is a superset of the ISA extension.
cpu feature != ISA extension.

While all ISA extensions are cpu features, all CPU features may not be
an ISA extension.
e.g. sv48 is not a ISA extension but F/D are (used to set the
cpu_hwcap_fpu static key)

Moreover, not all cpu feature/ISA extension requires a static key.
e.g SSTC extension will require a static key because the check has to
happen in the hot path.
However, sscofpmf extension don't need a static key as the check
happens only one time during boot.

We should keep these two separate but a common static framework would
be very useful.

Here is the flow that I have in my mind.
1. All ISA extensions will be parsed through riscv,isa DT property
2. Any supported/enabled extension will be set in riscv_isa bitmap
3. Any extension requiring a static key will invoke the cpus_set_cap.

cpus_set_cap will be invoked from a different code path that uses a
static key for a specific ISA
extension or a CPU feature.

The only problem I see here is that we have to set a bit in both
cpu_hwcaps & riscv_isa bitmap.
We also have to define the value of that bit for any extension
requiring a static key twice as well.

I think that should be okay. But I would like to hear what everybody
else thinks as well.

> Thanks
Jisheng Zhang March 26, 2022, 7:18 a.m. UTC | #3 
On Tue, Feb 15, 2022 at 11:06:24AM -0800, Atish Kumar Patra wrote:
> On Tue, Feb 15, 2022 at 8:04 AM Jisheng Zhang <jszhang@kernel.org> wrote:
> >
> > On Tue, Feb 15, 2022 at 01:02:05AM -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           : rv64imafdcsu
> > > isa-ext               : sstc,sscofpmf
> > > mmu           : sv48
> > >
> > > processor     : 1
> > > hart          : 1
> > > isa           : rv64imafdcsu
> > > isa-ext               : sstc,sscofpmf
> > > mmu           : sv48
> > >
> > > processor     : 2
> > > hart          : 2
> > > isa           : rv64imafdcsu
> > > isa-ext               : sstc,sscofpmf
> > > mmu           : sv48
> > >
> > > processor     : 3
> > > hart          : 3
> > > isa           : rv64imafdcsu
> > > isa-ext               : sstc,sscofpmf
> > > 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.
> >
> > Hi Atish,
> >
> > Thanks for this series. I'm thinking cpu features VS ISA extenstions.
> > I'm converting the sv48 to static key:
> > https://lore.kernel.org/linux-riscv/20220125165036.987-1-jszhang@kernel.org/
> >
> > Previously, I thought the SV48 as a cpu feature, and there will be
> > more and more cpu features, so I implemented an unified static key
> > mechanism for CPU features. But after reading this series, I think
> > I may need to rebase(even reimplement) the above patch to your series.
> > But I'm a bit confused by CPU features VS ISA extenstions now:
> >
> > 1. Is cpu feature  == ISA extension?
> >
> > 2. Is SV48 considered as ISA extension?
> > If yes, now SV48 or not is determined during runtime, but current ISA
> > extensions seem parsed from DT. So how to support those ISA extensions
> > which can be determined during runtime?
> >
> > Could you please share your thought?
> >
> 
> Here are my two cents:
> 
> I think the cpu feature is a superset of the ISA extension.
> cpu feature != ISA extension.
> 
> While all ISA extensions are cpu features, all CPU features may not be
> an ISA extension.
> e.g. sv48 is not a ISA extension but F/D are (used to set the
> cpu_hwcap_fpu static key)
> 
> Moreover, not all cpu feature/ISA extension requires a static key.
> e.g SSTC extension will require a static key because the check has to
> happen in the hot path.
> However, sscofpmf extension don't need a static key as the check
> happens only one time during boot.
> 
> We should keep these two separate but a common static framework would
> be very useful.
> 
> Here is the flow that I have in my mind.
> 1. All ISA extensions will be parsed through riscv,isa DT property
> 2. Any supported/enabled extension will be set in riscv_isa bitmap
> 3. Any extension requiring a static key will invoke the cpus_set_cap.
> 
> cpus_set_cap will be invoked from a different code path that uses a
> static key for a specific ISA
> extension or a CPU feature.
> 
> The only problem I see here is that we have to set a bit in both
> cpu_hwcaps & riscv_isa bitmap.
> We also have to define the value of that bit for any extension
> requiring a static key twice as well.
> 
> I think that should be okay. But I would like to hear what everybody
> else thinks as well.
> 

Thank Atish's input. I notice that SV57 support is merged, I'll
send a new version to apply static mechanism to both SV48 and SV57
once rc1 is released.