[PATCH -fixes v2 0/4] riscv: cbo.zero fixes

[PATCH -fixes v2 0/4] riscv: cbo.zero fixes

[Samuel Holland]

This series fixes a couple issues related to using the cbo.zero
instruction in userspace. The first patches fixes a bug where the wrong
enable bit gets set if the kernel is running in M-mode. The remaining
patches fix a bug where the enable bit gets reset to its default value
after a nonretentive idle state. I have hardware which reproduces this:

Before this series (or without ss1p12 in the devicetree):
  $ tools/testing/selftests/riscv/hwprobe/cbo
  TAP version 13
  1..3
  ok 1 Zicboz block size
  # Zicboz block size: 64
  Illegal instruction

After applying this series:
  $ tools/testing/selftests/riscv/hwprobe/cbo
  TAP version 13
  1..3
  ok 1 Zicboz block size
  # Zicboz block size: 64
  ok 2 cbo.zero
  ok 3 cbo.zero check
  # Totals: pass:3 fail:0 xfail:0 xpass:0 skip:0 error:0

Changes in v2:
 - Add patches to allow parsing the privileged ISA version from the DT
 - Check for privileged ISA v1.12 instead of the specific CSR
 - Use riscv_has_extension_likely() instead of new ALTERNATIVE()s

Samuel Holland (4):
  riscv: Fix enabling cbo.zero when running in M-mode
  dt-bindings: riscv: Add ratified privileged ISA versions
  riscv: Add ISA extension parsing for Sm and Ss
  riscv: Save/restore envcfg CSR during CPU suspend

 .../devicetree/bindings/riscv/extensions.yaml | 20 +++++++++
 arch/riscv/include/asm/cpufeature.h           |  1 +
 arch/riscv/include/asm/csr.h                  |  2 +
 arch/riscv/include/asm/hwcap.h                |  8 ++++
 arch/riscv/include/asm/suspend.h              |  1 +
 arch/riscv/kernel/cpu.c                       |  5 +++
 arch/riscv/kernel/cpufeature.c                | 44 ++++++++++++++++---
 arch/riscv/kernel/suspend.c                   |  4 ++
 8 files changed, 79 insertions(+), 6 deletions(-)

-- 
2.43.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH -fixes v2 1/4] riscv: Fix enabling cbo.zero when running in M-mode

[Samuel Holland]

When the kernel is running in M-mode, the CBZE bit must be set in the
menvcfg CSR, not in senvcfg.

Cc: <stable@vger.kernel.org>
Fixes: 43c16d51a19b ("RISC-V: Enable cbo.zero in usermode")
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
---

(no changes since v1)

 arch/riscv/include/asm/csr.h   | 2 ++
 arch/riscv/kernel/cpufeature.c | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/riscv/include/asm/csr.h b/arch/riscv/include/asm/csr.h
index 510014051f5d..2468c55933cd 100644
--- a/arch/riscv/include/asm/csr.h
+++ b/arch/riscv/include/asm/csr.h
@@ -424,6 +424,7 @@
 # define CSR_STATUS	CSR_MSTATUS
 # define CSR_IE		CSR_MIE
 # define CSR_TVEC	CSR_MTVEC
+# define CSR_ENVCFG	CSR_MENVCFG
 # define CSR_SCRATCH	CSR_MSCRATCH
 # define CSR_EPC	CSR_MEPC
 # define CSR_CAUSE	CSR_MCAUSE
@@ -448,6 +449,7 @@
 # define CSR_STATUS	CSR_SSTATUS
 # define CSR_IE		CSR_SIE
 # define CSR_TVEC	CSR_STVEC
+# define CSR_ENVCFG	CSR_SENVCFG
 # define CSR_SCRATCH	CSR_SSCRATCH
 # define CSR_EPC	CSR_SEPC
 # define CSR_CAUSE	CSR_SCAUSE
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index 89920f84d0a3..c5b13f7dd482 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -950,7 +950,7 @@ arch_initcall(check_unaligned_access_all_cpus);
 void riscv_user_isa_enable(void)
 {
 	if (riscv_cpu_has_extension_unlikely(smp_processor_id(), RISCV_ISA_EXT_ZICBOZ))
-		csr_set(CSR_SENVCFG, ENVCFG_CBZE);
+		csr_set(CSR_ENVCFG, ENVCFG_CBZE);
 }
 
 #ifdef CONFIG_RISCV_ALTERNATIVE
-- 
2.43.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH -fixes v2 2/4] dt-bindings: riscv: Add ratified privileged ISA versions

[Samuel Holland]

The baseline for the RISC-V privileged ISA is version 1.10. Using
features from newer versions of the privileged ISA requires the
supported version to be reported by platform firmware, either in the ISA
string (where the binding already accepts version numbers) or in the
riscv,isa-extensions property. So far two newer versions are ratified.

Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
---

Changes in v2:
 - New patch for v2

 .../devicetree/bindings/riscv/extensions.yaml | 20 +++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/Documentation/devicetree/bindings/riscv/extensions.yaml b/Documentation/devicetree/bindings/riscv/extensions.yaml
index 63d81dc895e5..7faf22df01af 100644
--- a/Documentation/devicetree/bindings/riscv/extensions.yaml
+++ b/Documentation/devicetree/bindings/riscv/extensions.yaml
@@ -121,6 +121,16 @@ properties:
             version of the privileged ISA specification.
 
         # multi-letter extensions, sorted alphanumerically
+        - const: sm1p11
+          description:
+            The standard Machine ISA v1.11, as ratified in the 20190608
+            version of the privileged ISA specification.
+
+        - const: sm1p12
+          description:
+            The standard Machine ISA v1.12, as ratified in the 20211203
+            version of the privileged ISA specification.
+
         - const: smaia
           description: |
             The standard Smaia supervisor-level extension for the advanced
@@ -134,6 +144,16 @@ properties:
             added by other RISC-V extensions in H/S/VS/U/VU modes and as
             ratified at commit a28bfae (Ratified (#7)) of riscv-state-enable.
 
+        - const: ss1p11
+          description:
+            The standard Supervisor ISA v1.11, as ratified in the 20190608
+            version of the privileged ISA specification.
+
+        - const: ss1p12
+          description:
+            The standard Supervisor ISA v1.12, as ratified in the 20211203
+            version of the privileged ISA specification.
+
         - const: ssaia
           description: |
             The standard Ssaia supervisor-level extension for the advanced
-- 
2.43.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH -fixes v2 2/4] dt-bindings: riscv: Add ratified privileged ISA versions

[Krzysztof Kozlowski]

On 13/02/2024 04:37, Samuel Holland wrote:
> The baseline for the RISC-V privileged ISA is version 1.10. Using
> features from newer versions of the privileged ISA requires the
> supported version to be reported by platform firmware, either in the ISA
> string (where the binding already accepts version numbers) or in the
> riscv,isa-extensions property. So far two newer versions are ratified.
> 
> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
> ---

Please Cc DT list.

Standard disclaimer:

Please use scripts/get_maintainers.pl to get a list of necessary people
and lists to CC. It might happen, that command when run on an older
kernel, gives you outdated entries. Therefore please be sure you base
your patches on recent Linux kernel.

Tools like b4 or scripts/get_maintainer.pl provide you proper list of
people, so fix your workflow. Tools might also fail if you work on some
ancient tree (don't, instead use mainline), work on fork of kernel
(don't, instead use mainline) or you ignore some maintainers (really
don't). Just use b4 and everything should be fine, although remember
about `b4 prep --auto-to-cc` if you added new patches to the patchset.

You missed at least devicetree list (maybe more), so this won't be
tested by automated tooling.

Please kindly resend and include all necessary To/Cc entries.

Best regards,
Krzysztof


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH -fixes v2 2/4] dt-bindings: riscv: Add ratified privileged ISA versions

[Andrew Jones]

On Mon, Feb 12, 2024 at 07:37:33PM -0800, Samuel Holland wrote:
> The baseline for the RISC-V privileged ISA is version 1.10. Using
> features from newer versions of the privileged ISA requires the
> supported version to be reported by platform firmware, either in the ISA
> string (where the binding already accepts version numbers) or in the
> riscv,isa-extensions property. So far two newer versions are ratified.
> 
> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
> ---
> 
> Changes in v2:
>  - New patch for v2
> 
>  .../devicetree/bindings/riscv/extensions.yaml | 20 +++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/riscv/extensions.yaml b/Documentation/devicetree/bindings/riscv/extensions.yaml
> index 63d81dc895e5..7faf22df01af 100644
> --- a/Documentation/devicetree/bindings/riscv/extensions.yaml
> +++ b/Documentation/devicetree/bindings/riscv/extensions.yaml
> @@ -121,6 +121,16 @@ properties:
>              version of the privileged ISA specification.
>  
>          # multi-letter extensions, sorted alphanumerically
> +        - const: sm1p11
> +          description:
> +            The standard Machine ISA v1.11, as ratified in the 20190608
> +            version of the privileged ISA specification.
> +
> +        - const: sm1p12
> +          description:
> +            The standard Machine ISA v1.12, as ratified in the 20211203
> +            version of the privileged ISA specification.
> +
>          - const: smaia
>            description: |
>              The standard Smaia supervisor-level extension for the advanced
> @@ -134,6 +144,16 @@ properties:
>              added by other RISC-V extensions in H/S/VS/U/VU modes and as
>              ratified at commit a28bfae (Ratified (#7)) of riscv-state-enable.
>  
> +        - const: ss1p11
> +          description:
> +            The standard Supervisor ISA v1.11, as ratified in the 20190608
> +            version of the privileged ISA specification.
> +
> +        - const: ss1p12
> +          description:
> +            The standard Supervisor ISA v1.12, as ratified in the 20211203
> +            version of the privileged ISA specification.
> +
>          - const: ssaia
>            description: |
>              The standard Ssaia supervisor-level extension for the advanced
> -- 
> 2.43.0
>

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

Note, QEMU doesn't add these extensions to the ISA string yet, but I think
it should start, particularly the profile CPU types which should ensure
all the profile's mandatory extensions are added to the ISA string in
order to avoid any confusion.

Thanks,
drew

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH -fixes v2 2/4] dt-bindings: riscv: Add ratified privileged ISA versions

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 864 bytes --]

On Tue, Feb 13, 2024 at 03:25:44PM +0100, Andrew Jones wrote:
> On Mon, Feb 12, 2024 at 07:37:33PM -0800, Samuel Holland wrote:

> Note, QEMU doesn't add these extensions to the ISA string yet, but I think
> it should start, particularly the profile CPU types which should ensure
> all the profile's mandatory extensions are added to the ISA string in
> order to avoid any confusion.

Something to note about these "mandatory extensions" that are names for
things we already assumed were present - they're utterly useless and any
DT property should note their absence, not presence, in order to be of any
use. Anything parsing a DT cannot see "svbare" and gain any new
information, since the lack of it could be something that predates the
definition of "svbare" or something without "svbare".

Shit, but that's exactly why I deprecated riscv,isa.

Cheers,
Conor.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH -fixes v2 2/4] dt-bindings: riscv: Add ratified privileged ISA versions

[Stefan O'Rear]

On Thu, Feb 15, 2024, at 8:14 AM, Conor Dooley wrote:
> On Tue, Feb 13, 2024 at 03:25:44PM +0100, Andrew Jones wrote:
>> On Mon, Feb 12, 2024 at 07:37:33PM -0800, Samuel Holland wrote:
>
>> Note, QEMU doesn't add these extensions to the ISA string yet, but I think
>> it should start, particularly the profile CPU types which should ensure
>> all the profile's mandatory extensions are added to the ISA string in
>> order to avoid any confusion.
>
> Something to note about these "mandatory extensions" that are names for
> things we already assumed were present - they're utterly useless and any
> DT property should note their absence, not presence, in order to be of any
> use. Anything parsing a DT cannot see "svbare" and gain any new
> information, since the lack of it could be something that predates the
> definition of "svbare" or something without "svbare".

This is consistent with the way we handle other extensions that are assumed
at compile time - if you build with RISCV_ISA_C=y, omitting "c" from
riscv,isa-extensions will not cause an error.

It's also the case for any extension whatsoever that if that extension is
not present in the device tree, no information is provided.

It might be useful for diagnostic purposes to have a "binding version"
somewhere to indicate which extensions _would_ be documented; not sure if
there is already a mechanism for this.  For extensions that the kernel has
a hard requirement on like svbare, see above.

> Shit, but that's exactly why I deprecated riscv,isa.

If zicsr and zifencei were broken out from i today, there would not be a
problem, because i as specified by riscv,isa-extensions would refer to a
specific version that included zicsr and zifencei with a new name for the
new, smaller version of i.

It's not working here because privileged architecture versions aren't (yet)
included in riscv,isa-extensions.

-s

> Cheers,
> Conor.
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv
>
> Attachments:
> * signature.asc

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH -fixes v2 2/4] dt-bindings: riscv: Add ratified privileged ISA versions

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 2767 bytes --]

On Mon, Feb 12, 2024 at 07:37:33PM -0800, Samuel Holland wrote:
> The baseline for the RISC-V privileged ISA is version 1.10. Using
> features from newer versions of the privileged ISA requires the
> supported version to be reported by platform firmware, either in the ISA
> string (where the binding already accepts version numbers) or in the
> riscv,isa-extensions property. So far two newer versions are ratified.
> 
> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
> ---
> 
> Changes in v2:
>  - New patch for v2
> 
>  .../devicetree/bindings/riscv/extensions.yaml | 20 +++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/riscv/extensions.yaml b/Documentation/devicetree/bindings/riscv/extensions.yaml
> index 63d81dc895e5..7faf22df01af 100644
> --- a/Documentation/devicetree/bindings/riscv/extensions.yaml
> +++ b/Documentation/devicetree/bindings/riscv/extensions.yaml
> @@ -121,6 +121,16 @@ properties:
>              version of the privileged ISA specification.
>  
>          # multi-letter extensions, sorted alphanumerically

> +        - const: sm1p11

Why are we beholden to this "1p11" format of RVI's? We have free choice
of characters here, what's stopping us using "machine-v1.11", for
example?

Thanks,
Conor.


> +          description:
> +            The standard Machine ISA v1.11, as ratified in the 20190608
> +            version of the privileged ISA specification.
> +
> +        - const: sm1p12
> +          description:
> +            The standard Machine ISA v1.12, as ratified in the 20211203
> +            version of the privileged ISA specification.
> +
>          - const: smaia
>            description: |
>              The standard Smaia supervisor-level extension for the advanced
> @@ -134,6 +144,16 @@ properties:
>              added by other RISC-V extensions in H/S/VS/U/VU modes and as
>              ratified at commit a28bfae (Ratified (#7)) of riscv-state-enable.
>  
> +        - const: ss1p11
> +          description:
> +            The standard Supervisor ISA v1.11, as ratified in the 20190608
> +            version of the privileged ISA specification.
> +
> +        - const: ss1p12
> +          description:
> +            The standard Supervisor ISA v1.12, as ratified in the 20211203
> +            version of the privileged ISA specification.
> +
>          - const: ssaia
>            description: |
>              The standard Ssaia supervisor-level extension for the advanced
> -- 
> 2.43.0
> 
> 
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH -fixes v2 2/4] dt-bindings: riscv: Add ratified privileged ISA versions

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 1557 bytes --]

On Tue, Feb 13, 2024 at 05:03:46PM +0000, Conor Dooley wrote:
> On Mon, Feb 12, 2024 at 07:37:33PM -0800, Samuel Holland wrote:
> > The baseline for the RISC-V privileged ISA is version 1.10. Using
> > features from newer versions of the privileged ISA requires the
> > supported version to be reported by platform firmware, either in the ISA
> > string (where the binding already accepts version numbers) or in the
> > riscv,isa-extensions property. So far two newer versions are ratified.
> > 
> > Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
> > ---
> > 
> > Changes in v2:
> >  - New patch for v2
> > 
> >  .../devicetree/bindings/riscv/extensions.yaml | 20 +++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/riscv/extensions.yaml b/Documentation/devicetree/bindings/riscv/extensions.yaml
> > index 63d81dc895e5..7faf22df01af 100644
> > --- a/Documentation/devicetree/bindings/riscv/extensions.yaml
> > +++ b/Documentation/devicetree/bindings/riscv/extensions.yaml
> > @@ -121,6 +121,16 @@ properties:
> >              version of the privileged ISA specification.
> >  
> >          # multi-letter extensions, sorted alphanumerically
> 
> > +        - const: sm1p11
> 
> Why are we beholden to this "1p11" format of RVI's? We have free choice
> of characters here, what's stopping us using "machine-v1.11", for
> example?

We could also choose to communicate this using a specific property, but
I have not really thought that one through yet.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH -fixes v2 2/4] dt-bindings: riscv: Add ratified privileged ISA versions

[Stefan O'Rear]

On Tue, Feb 13, 2024, at 12:03 PM, Conor Dooley wrote:
> On Mon, Feb 12, 2024 at 07:37:33PM -0800, Samuel Holland wrote:
>> The baseline for the RISC-V privileged ISA is version 1.10. Using
>> features from newer versions of the privileged ISA requires the
>> supported version to be reported by platform firmware, either in the ISA
>> string (where the binding already accepts version numbers) or in the
>> riscv,isa-extensions property. So far two newer versions are ratified.
>> 
>> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
>> ---
>> 
>> Changes in v2:
>>  - New patch for v2
>> 
>>  .../devicetree/bindings/riscv/extensions.yaml | 20 +++++++++++++++++++
>>  1 file changed, 20 insertions(+)
>> 
>> diff --git a/Documentation/devicetree/bindings/riscv/extensions.yaml b/Documentation/devicetree/bindings/riscv/extensions.yaml
>> index 63d81dc895e5..7faf22df01af 100644
>> --- a/Documentation/devicetree/bindings/riscv/extensions.yaml
>> +++ b/Documentation/devicetree/bindings/riscv/extensions.yaml
>> @@ -121,6 +121,16 @@ properties:
>>              version of the privileged ISA specification.
>>  
>>          # multi-letter extensions, sorted alphanumerically
>
>> +        - const: sm1p11
>
> Why are we beholden to this "1p11" format of RVI's? We have free choice
> of characters here, what's stopping us using "machine-v1.11", for
> example?
>
> Thanks,
> Conor.

I'd prefer to use names that are at least somewhat recognizable, e.g. in
the profiles spec, rather than making up something from whole cloth.

-s

>
>> +          description:
>> +            The standard Machine ISA v1.11, as ratified in the 20190608
>> +            version of the privileged ISA specification.
>> +
>> +        - const: sm1p12
>> +          description:
>> +            The standard Machine ISA v1.12, as ratified in the 20211203
>> +            version of the privileged ISA specification.
>> +
>>          - const: smaia
>>            description: |
>>              The standard Smaia supervisor-level extension for the advanced
>> @@ -134,6 +144,16 @@ properties:
>>              added by other RISC-V extensions in H/S/VS/U/VU modes and as
>>              ratified at commit a28bfae (Ratified (#7)) of riscv-state-enable.
>>  
>> +        - const: ss1p11
>> +          description:
>> +            The standard Supervisor ISA v1.11, as ratified in the 20190608
>> +            version of the privileged ISA specification.
>> +
>> +        - const: ss1p12
>> +          description:
>> +            The standard Supervisor ISA v1.12, as ratified in the 20211203
>> +            version of the privileged ISA specification.
>> +
>>          - const: ssaia
>>            description: |
>>              The standard Ssaia supervisor-level extension for the advanced
>> -- 
>> 2.43.0
>> 
>> 
>> _______________________________________________
>> linux-riscv mailing list
>> linux-riscv@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-riscv
>
> Attachments:
> * signature.asc

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH -fixes v2 2/4] dt-bindings: riscv: Add ratified privileged ISA versions

[Samuel Holland]

On 2024-02-13 11:42 AM, Stefan O'Rear wrote:
> On Tue, Feb 13, 2024, at 12:03 PM, Conor Dooley wrote:
>> On Mon, Feb 12, 2024 at 07:37:33PM -0800, Samuel Holland wrote:
>>> The baseline for the RISC-V privileged ISA is version 1.10. Using
>>> features from newer versions of the privileged ISA requires the
>>> supported version to be reported by platform firmware, either in the ISA
>>> string (where the binding already accepts version numbers) or in the
>>> riscv,isa-extensions property. So far two newer versions are ratified.
>>>
>>> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
>>> ---
>>>
>>> Changes in v2:
>>>  - New patch for v2
>>>
>>>  .../devicetree/bindings/riscv/extensions.yaml | 20 +++++++++++++++++++
>>>  1 file changed, 20 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/riscv/extensions.yaml b/Documentation/devicetree/bindings/riscv/extensions.yaml
>>> index 63d81dc895e5..7faf22df01af 100644
>>> --- a/Documentation/devicetree/bindings/riscv/extensions.yaml
>>> +++ b/Documentation/devicetree/bindings/riscv/extensions.yaml
>>> @@ -121,6 +121,16 @@ properties:
>>>              version of the privileged ISA specification.
>>>  
>>>          # multi-letter extensions, sorted alphanumerically
>>
>>> +        - const: sm1p11
>>
>> Why are we beholden to this "1p11" format of RVI's? We have free choice
>> of characters here, what's stopping us using "machine-v1.11", for
>> example?
> 
> I'd prefer to use names that are at least somewhat recognizable, e.g. in
> the profiles spec, rather than making up something from whole cloth.

Right, exactly. My two immediate reasons for choosing this are:
1) this is the exact name used in the profiles[1][2], and
2) the same list of extensions is used for the riscv,isa-extensions property and
the ISA string, and this is the expected format for the ISA string.

If we want invent something brand new for the DT binding (which I'm not sure we
do), then I would recommend adding a property like riscv,privileged-isa-version,
because that removes the duplication between the Sm and Ss extensions (since
almost all implementations would have both).

On the other hand, there will likely be other extensions in the future that need
version numbers in riscv,isa-extensions. Adding a special case for the
privileged ISA doesn't help with this, whereas deciding on a syntax for version
numbers in the extension name does.

Regards,
Samuel

[1]:
https://github.com/riscv/riscv-profiles/blob/main/profiles.adoc#rva20s64-mandatory-extensions
[2]:
https://github.com/riscv/riscv-profiles/blob/main/profiles.adoc#rva22s64-mandatory-extensions


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH -fixes v2 3/4] riscv: Add ISA extension parsing for Sm and Ss

[Samuel Holland]

Previously, all extension version numbers were ignored. However, the
version number is important for these two extensions. The simplest way
to implement this is to use a separate bitmap bit for each supported
version, with each successive version implying all of the previous ones.
This allows alternatives and riscv_has_extension_[un]likely() to work
naturally.

To avoid duplicate extensions in /proc/cpuinfo, the new successor_id
field allows hiding all but the newest implemented version of an
extension.

Cc: <stable@vger.kernel.org> # v6.7+
Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
---

Changes in v2:
 - New patch for v2

 arch/riscv/include/asm/cpufeature.h |  1 +
 arch/riscv/include/asm/hwcap.h      |  8 ++++++
 arch/riscv/kernel/cpu.c             |  5 ++++
 arch/riscv/kernel/cpufeature.c      | 42 +++++++++++++++++++++++++----
 4 files changed, 51 insertions(+), 5 deletions(-)

diff --git a/arch/riscv/include/asm/cpufeature.h b/arch/riscv/include/asm/cpufeature.h
index 0bd11862b760..ac71384e7bc4 100644
--- a/arch/riscv/include/asm/cpufeature.h
+++ b/arch/riscv/include/asm/cpufeature.h
@@ -61,6 +61,7 @@ struct riscv_isa_ext_data {
 	const char *property;
 	const unsigned int *subset_ext_ids;
 	const unsigned int subset_ext_size;
+	const unsigned int successor_id;
 };
 
 extern const struct riscv_isa_ext_data riscv_isa_ext[];
diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
index 5340f818746b..5b51aa1db15b 100644
--- a/arch/riscv/include/asm/hwcap.h
+++ b/arch/riscv/include/asm/hwcap.h
@@ -80,13 +80,21 @@
 #define RISCV_ISA_EXT_ZFA		71
 #define RISCV_ISA_EXT_ZTSO		72
 #define RISCV_ISA_EXT_ZACAS		73
+#define RISCV_ISA_EXT_SM1p11		74
+#define RISCV_ISA_EXT_SM1p12		75
+#define RISCV_ISA_EXT_SS1p11		76
+#define RISCV_ISA_EXT_SS1p12		77
 
 #define RISCV_ISA_EXT_MAX		128
 #define RISCV_ISA_EXT_INVALID		U32_MAX
 
 #ifdef CONFIG_RISCV_M_MODE
+#define RISCV_ISA_EXT_Sx1p11		RISCV_ISA_EXT_SM1p11
+#define RISCV_ISA_EXT_Sx1p12		RISCV_ISA_EXT_SM1p12
 #define RISCV_ISA_EXT_SxAIA		RISCV_ISA_EXT_SMAIA
 #else
+#define RISCV_ISA_EXT_Sx1p11		RISCV_ISA_EXT_SS1p11
+#define RISCV_ISA_EXT_Sx1p12		RISCV_ISA_EXT_SS1p12
 #define RISCV_ISA_EXT_SxAIA		RISCV_ISA_EXT_SSAIA
 #endif
 
diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
index d11d6320fb0d..2e6b90ed0d51 100644
--- a/arch/riscv/kernel/cpu.c
+++ b/arch/riscv/kernel/cpu.c
@@ -215,6 +215,11 @@ static void print_isa(struct seq_file *f, const unsigned long *isa_bitmap)
 		if (!__riscv_isa_extension_available(isa_bitmap, riscv_isa_ext[i].id))
 			continue;
 
+		/* Only show the newest implemented version of an extension */
+		if (riscv_isa_ext[i].successor_id != RISCV_ISA_EXT_INVALID &&
+		    __riscv_isa_extension_available(isa_bitmap, riscv_isa_ext[i].successor_id))
+			continue;
+
 		/* Only multi-letter extensions are split by underscores */
 		if (strnlen(riscv_isa_ext[i].name, 2) != 1)
 			seq_puts(f, "_");
diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
index c5b13f7dd482..8e10b50120e9 100644
--- a/arch/riscv/kernel/cpufeature.c
+++ b/arch/riscv/kernel/cpufeature.c
@@ -113,23 +113,29 @@ static bool riscv_isa_extension_check(int id)
 	return true;
 }
 
-#define _RISCV_ISA_EXT_DATA(_name, _id, _subset_exts, _subset_exts_size) {	\
+#define _RISCV_ISA_EXT_DATA(_name, _id, _subset_exts, _subset_exts_size, _successor) {	\
 	.name = #_name,								\
 	.property = #_name,							\
 	.id = _id,								\
 	.subset_ext_ids = _subset_exts,						\
-	.subset_ext_size = _subset_exts_size					\
+	.subset_ext_size = _subset_exts_size,					\
+	.successor_id = _successor,						\
 }
 
-#define __RISCV_ISA_EXT_DATA(_name, _id) _RISCV_ISA_EXT_DATA(_name, _id, NULL, 0)
+#define __RISCV_ISA_EXT_DATA(_name, _id) \
+	_RISCV_ISA_EXT_DATA(_name, _id, NULL, 0, RISCV_ISA_EXT_INVALID)
 
 /* Used to declare pure "lasso" extension (Zk for instance) */
 #define __RISCV_ISA_EXT_BUNDLE(_name, _bundled_exts) \
-	_RISCV_ISA_EXT_DATA(_name, RISCV_ISA_EXT_INVALID, _bundled_exts, ARRAY_SIZE(_bundled_exts))
+	_RISCV_ISA_EXT_DATA(_name, RISCV_ISA_EXT_INVALID, \
+			    _bundled_exts, ARRAY_SIZE(_bundled_exts), RISCV_ISA_EXT_INVALID)
 
 /* Used to declare extensions that are a superset of other extensions (Zvbb for instance) */
 #define __RISCV_ISA_EXT_SUPERSET(_name, _id, _sub_exts) \
-	_RISCV_ISA_EXT_DATA(_name, _id, _sub_exts, ARRAY_SIZE(_sub_exts))
+	_RISCV_ISA_EXT_DATA(_name, _id, _sub_exts, ARRAY_SIZE(_sub_exts), RISCV_ISA_EXT_INVALID)
+
+#define __RISCV_ISA_EXT_VERSION(_name, _id, _preds, _preds_size, _successor) \
+	_RISCV_ISA_EXT_DATA(_name, _id, _preds, _preds_size, _successor)
 
 static const unsigned int riscv_zk_bundled_exts[] = {
 	RISCV_ISA_EXT_ZBKB,
@@ -201,6 +207,16 @@ static const unsigned int riscv_zvbb_exts[] = {
 	RISCV_ISA_EXT_ZVKB
 };
 
+static const unsigned int riscv_sm_ext_versions[] = {
+	RISCV_ISA_EXT_SM1p11,
+	RISCV_ISA_EXT_SM1p12,
+};
+
+static const unsigned int riscv_ss_ext_versions[] = {
+	RISCV_ISA_EXT_SS1p11,
+	RISCV_ISA_EXT_SS1p12,
+};
+
 /*
  * The canonical order of ISA extension names in the ISA string is defined in
  * chapter 27 of the unprivileged specification.
@@ -299,8 +315,16 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = {
 	__RISCV_ISA_EXT_DATA(zvksh, RISCV_ISA_EXT_ZVKSH),
 	__RISCV_ISA_EXT_BUNDLE(zvksg, riscv_zvksg_bundled_exts),
 	__RISCV_ISA_EXT_DATA(zvkt, RISCV_ISA_EXT_ZVKT),
+	__RISCV_ISA_EXT_VERSION(sm1p11, RISCV_ISA_EXT_SM1p11, riscv_sm_ext_versions, 0,
+				RISCV_ISA_EXT_SM1p12),
+	__RISCV_ISA_EXT_VERSION(sm1p12, RISCV_ISA_EXT_SM1p12, riscv_sm_ext_versions, 1,
+				RISCV_ISA_EXT_INVALID),
 	__RISCV_ISA_EXT_DATA(smaia, RISCV_ISA_EXT_SMAIA),
 	__RISCV_ISA_EXT_DATA(smstateen, RISCV_ISA_EXT_SMSTATEEN),
+	__RISCV_ISA_EXT_VERSION(ss1p11, RISCV_ISA_EXT_SS1p11, riscv_ss_ext_versions, 0,
+				RISCV_ISA_EXT_SS1p12),
+	__RISCV_ISA_EXT_VERSION(ss1p12, RISCV_ISA_EXT_SS1p12, riscv_ss_ext_versions, 1,
+				RISCV_ISA_EXT_INVALID),
 	__RISCV_ISA_EXT_DATA(ssaia, RISCV_ISA_EXT_SSAIA),
 	__RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
 	__RISCV_ISA_EXT_DATA(sstc, RISCV_ISA_EXT_SSTC),
@@ -414,6 +438,14 @@ static void __init riscv_parse_isa_string(unsigned long *this_hwcap, struct risc
 				;
 
 			++ext_end;
+
+			/*
+			 * As a special case for the Sm and Ss extensions, where the version
+			 * number is important, include it in the extension name.
+			 */
+			if (ext_end - ext == 2 && tolower(ext[0]) == 's' &&
+			    (tolower(ext[1]) == 'm' || tolower(ext[1]) == 's'))
+				ext_end = isa;
 			break;
 		default:
 			/*
-- 
2.43.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH -fixes v2 3/4] riscv: Add ISA extension parsing for Sm and Ss

[Andrew Jones]

On Mon, Feb 12, 2024 at 07:37:34PM -0800, Samuel Holland wrote:
> Previously, all extension version numbers were ignored. However, the
> version number is important for these two extensions. The simplest way
> to implement this is to use a separate bitmap bit for each supported
> version, with each successive version implying all of the previous ones.
> This allows alternatives and riscv_has_extension_[un]likely() to work
> naturally.
> 
> To avoid duplicate extensions in /proc/cpuinfo, the new successor_id
> field allows hiding all but the newest implemented version of an
> extension.
> 
> Cc: <stable@vger.kernel.org> # v6.7+
> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
> ---
> 
> Changes in v2:
>  - New patch for v2
> 
>  arch/riscv/include/asm/cpufeature.h |  1 +
>  arch/riscv/include/asm/hwcap.h      |  8 ++++++
>  arch/riscv/kernel/cpu.c             |  5 ++++
>  arch/riscv/kernel/cpufeature.c      | 42 +++++++++++++++++++++++++----
>  4 files changed, 51 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/cpufeature.h b/arch/riscv/include/asm/cpufeature.h
> index 0bd11862b760..ac71384e7bc4 100644
> --- a/arch/riscv/include/asm/cpufeature.h
> +++ b/arch/riscv/include/asm/cpufeature.h
> @@ -61,6 +61,7 @@ struct riscv_isa_ext_data {
>  	const char *property;
>  	const unsigned int *subset_ext_ids;
>  	const unsigned int subset_ext_size;
> +	const unsigned int successor_id;
>  };
>  
>  extern const struct riscv_isa_ext_data riscv_isa_ext[];
> diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> index 5340f818746b..5b51aa1db15b 100644
> --- a/arch/riscv/include/asm/hwcap.h
> +++ b/arch/riscv/include/asm/hwcap.h
> @@ -80,13 +80,21 @@
>  #define RISCV_ISA_EXT_ZFA		71
>  #define RISCV_ISA_EXT_ZTSO		72
>  #define RISCV_ISA_EXT_ZACAS		73
> +#define RISCV_ISA_EXT_SM1p11		74
> +#define RISCV_ISA_EXT_SM1p12		75
> +#define RISCV_ISA_EXT_SS1p11		76
> +#define RISCV_ISA_EXT_SS1p12		77
>  
>  #define RISCV_ISA_EXT_MAX		128
>  #define RISCV_ISA_EXT_INVALID		U32_MAX
>  
>  #ifdef CONFIG_RISCV_M_MODE
> +#define RISCV_ISA_EXT_Sx1p11		RISCV_ISA_EXT_SM1p11
> +#define RISCV_ISA_EXT_Sx1p12		RISCV_ISA_EXT_SM1p12
>  #define RISCV_ISA_EXT_SxAIA		RISCV_ISA_EXT_SMAIA
>  #else
> +#define RISCV_ISA_EXT_Sx1p11		RISCV_ISA_EXT_SS1p11
> +#define RISCV_ISA_EXT_Sx1p12		RISCV_ISA_EXT_SS1p12
>  #define RISCV_ISA_EXT_SxAIA		RISCV_ISA_EXT_SSAIA
>  #endif
>  
> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> index d11d6320fb0d..2e6b90ed0d51 100644
> --- a/arch/riscv/kernel/cpu.c
> +++ b/arch/riscv/kernel/cpu.c
> @@ -215,6 +215,11 @@ static void print_isa(struct seq_file *f, const unsigned long *isa_bitmap)
>  		if (!__riscv_isa_extension_available(isa_bitmap, riscv_isa_ext[i].id))
>  			continue;
>  
> +		/* Only show the newest implemented version of an extension */
> +		if (riscv_isa_ext[i].successor_id != RISCV_ISA_EXT_INVALID &&
> +		    __riscv_isa_extension_available(isa_bitmap, riscv_isa_ext[i].successor_id))
> +			continue;

I'm not sure we need this. Expanding Ss1p12 to 'Ss1p11 Ss1p12' and then
outputting both in the ISA string doesn't seem harmful to me. Also, using
a successor field instead of supersedes field may make this logic easy,
but it'll require updating old code (changing RISCV_ISA_EXT_INVALID to the
new version extension ID) when new versions are added. A supersedes field
wouldn't require old code updates and would match the profiles spec which
have explicit 'Ss1p12 supersedes Ss1p11.' type sentences.

> +
>  		/* Only multi-letter extensions are split by underscores */
>  		if (strnlen(riscv_isa_ext[i].name, 2) != 1)
>  			seq_puts(f, "_");
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index c5b13f7dd482..8e10b50120e9 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -113,23 +113,29 @@ static bool riscv_isa_extension_check(int id)
>  	return true;
>  }
>  
> -#define _RISCV_ISA_EXT_DATA(_name, _id, _subset_exts, _subset_exts_size) {	\
> +#define _RISCV_ISA_EXT_DATA(_name, _id, _subset_exts, _subset_exts_size, _successor) {	\
>  	.name = #_name,								\
>  	.property = #_name,							\
>  	.id = _id,								\
>  	.subset_ext_ids = _subset_exts,						\
> -	.subset_ext_size = _subset_exts_size					\
> +	.subset_ext_size = _subset_exts_size,					\
> +	.successor_id = _successor,						\
>  }
>  
> -#define __RISCV_ISA_EXT_DATA(_name, _id) _RISCV_ISA_EXT_DATA(_name, _id, NULL, 0)
> +#define __RISCV_ISA_EXT_DATA(_name, _id) \
> +	_RISCV_ISA_EXT_DATA(_name, _id, NULL, 0, RISCV_ISA_EXT_INVALID)
>  
>  /* Used to declare pure "lasso" extension (Zk for instance) */
>  #define __RISCV_ISA_EXT_BUNDLE(_name, _bundled_exts) \
> -	_RISCV_ISA_EXT_DATA(_name, RISCV_ISA_EXT_INVALID, _bundled_exts, ARRAY_SIZE(_bundled_exts))
> +	_RISCV_ISA_EXT_DATA(_name, RISCV_ISA_EXT_INVALID, \
> +			    _bundled_exts, ARRAY_SIZE(_bundled_exts), RISCV_ISA_EXT_INVALID)
>  
>  /* Used to declare extensions that are a superset of other extensions (Zvbb for instance) */
>  #define __RISCV_ISA_EXT_SUPERSET(_name, _id, _sub_exts) \
> -	_RISCV_ISA_EXT_DATA(_name, _id, _sub_exts, ARRAY_SIZE(_sub_exts))
> +	_RISCV_ISA_EXT_DATA(_name, _id, _sub_exts, ARRAY_SIZE(_sub_exts), RISCV_ISA_EXT_INVALID)
> +
> +#define __RISCV_ISA_EXT_VERSION(_name, _id, _preds, _preds_size, _successor) \
> +	_RISCV_ISA_EXT_DATA(_name, _id, _preds, _preds_size, _successor)
>  
>  static const unsigned int riscv_zk_bundled_exts[] = {
>  	RISCV_ISA_EXT_ZBKB,
> @@ -201,6 +207,16 @@ static const unsigned int riscv_zvbb_exts[] = {
>  	RISCV_ISA_EXT_ZVKB
>  };
>  
> +static const unsigned int riscv_sm_ext_versions[] = {
> +	RISCV_ISA_EXT_SM1p11,
> +	RISCV_ISA_EXT_SM1p12,
> +};
> +
> +static const unsigned int riscv_ss_ext_versions[] = {
> +	RISCV_ISA_EXT_SS1p11,
> +	RISCV_ISA_EXT_SS1p12,
> +};
> +
>  /*
>   * The canonical order of ISA extension names in the ISA string is defined in
>   * chapter 27 of the unprivileged specification.
> @@ -299,8 +315,16 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = {
>  	__RISCV_ISA_EXT_DATA(zvksh, RISCV_ISA_EXT_ZVKSH),
>  	__RISCV_ISA_EXT_BUNDLE(zvksg, riscv_zvksg_bundled_exts),
>  	__RISCV_ISA_EXT_DATA(zvkt, RISCV_ISA_EXT_ZVKT),
> +	__RISCV_ISA_EXT_VERSION(sm1p11, RISCV_ISA_EXT_SM1p11, riscv_sm_ext_versions, 0,
> +				RISCV_ISA_EXT_SM1p12),
> +	__RISCV_ISA_EXT_VERSION(sm1p12, RISCV_ISA_EXT_SM1p12, riscv_sm_ext_versions, 1,
> +				RISCV_ISA_EXT_INVALID),
>  	__RISCV_ISA_EXT_DATA(smaia, RISCV_ISA_EXT_SMAIA),
>  	__RISCV_ISA_EXT_DATA(smstateen, RISCV_ISA_EXT_SMSTATEEN),
> +	__RISCV_ISA_EXT_VERSION(ss1p11, RISCV_ISA_EXT_SS1p11, riscv_ss_ext_versions, 0,
> +				RISCV_ISA_EXT_SS1p12),
> +	__RISCV_ISA_EXT_VERSION(ss1p12, RISCV_ISA_EXT_SS1p12, riscv_ss_ext_versions, 1,
> +				RISCV_ISA_EXT_INVALID),
>  	__RISCV_ISA_EXT_DATA(ssaia, RISCV_ISA_EXT_SSAIA),
>  	__RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
>  	__RISCV_ISA_EXT_DATA(sstc, RISCV_ISA_EXT_SSTC),
> @@ -414,6 +438,14 @@ static void __init riscv_parse_isa_string(unsigned long *this_hwcap, struct risc
>  				;
>  
>  			++ext_end;
> +
> +			/*
> +			 * As a special case for the Sm and Ss extensions, where the version
> +			 * number is important, include it in the extension name.
> +			 */
> +			if (ext_end - ext == 2 && tolower(ext[0]) == 's' &&
> +			    (tolower(ext[1]) == 'm' || tolower(ext[1]) == 's'))
> +				ext_end = isa;
>  			break;
>  		default:
>  			/*
> -- 
> 2.43.0
>

Thanks,
drew

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH -fixes v2 3/4] riscv: Add ISA extension parsing for Sm and Ss

[Stefan O'Rear]

On Tue, Feb 13, 2024, at 10:14 AM, Andrew Jones wrote:
> On Mon, Feb 12, 2024 at 07:37:34PM -0800, Samuel Holland wrote:
>> Previously, all extension version numbers were ignored. However, the
>> version number is important for these two extensions. The simplest way
>> to implement this is to use a separate bitmap bit for each supported
>> version, with each successive version implying all of the previous ones.
>> This allows alternatives and riscv_has_extension_[un]likely() to work
>> naturally.
>>
>> To avoid duplicate extensions in /proc/cpuinfo, the new successor_id
>> field allows hiding all but the newest implemented version of an
>> extension.
>> 
>> Cc: <stable@vger.kernel.org> # v6.7+
>> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
>> ---
>> 
>> Changes in v2:
>>  - New patch for v2
>> 
>>  arch/riscv/include/asm/cpufeature.h |  1 +
>>  arch/riscv/include/asm/hwcap.h      |  8 ++++++
>>  arch/riscv/kernel/cpu.c             |  5 ++++
>>  arch/riscv/kernel/cpufeature.c      | 42 +++++++++++++++++++++++++----
>>  4 files changed, 51 insertions(+), 5 deletions(-)
>> 
>> diff --git a/arch/riscv/include/asm/cpufeature.h b/arch/riscv/include/asm/cpufeature.h
>> index 0bd11862b760..ac71384e7bc4 100644
>> --- a/arch/riscv/include/asm/cpufeature.h
>> +++ b/arch/riscv/include/asm/cpufeature.h
>> @@ -61,6 +61,7 @@ struct riscv_isa_ext_data {
>>  	const char *property;
>>  	const unsigned int *subset_ext_ids;
>>  	const unsigned int subset_ext_size;
>> +	const unsigned int successor_id;
>>  };
>>  
>>  extern const struct riscv_isa_ext_data riscv_isa_ext[];
>> diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
>> index 5340f818746b..5b51aa1db15b 100644
>> --- a/arch/riscv/include/asm/hwcap.h
>> +++ b/arch/riscv/include/asm/hwcap.h
>> @@ -80,13 +80,21 @@
>>  #define RISCV_ISA_EXT_ZFA		71
>>  #define RISCV_ISA_EXT_ZTSO		72
>>  #define RISCV_ISA_EXT_ZACAS		73
>> +#define RISCV_ISA_EXT_SM1p11		74
>> +#define RISCV_ISA_EXT_SM1p12		75
>> +#define RISCV_ISA_EXT_SS1p11		76
>> +#define RISCV_ISA_EXT_SS1p12		77
>>  
>>  #define RISCV_ISA_EXT_MAX		128
>>  #define RISCV_ISA_EXT_INVALID		U32_MAX
>>  
>>  #ifdef CONFIG_RISCV_M_MODE
>> +#define RISCV_ISA_EXT_Sx1p11		RISCV_ISA_EXT_SM1p11
>> +#define RISCV_ISA_EXT_Sx1p12		RISCV_ISA_EXT_SM1p12
>>  #define RISCV_ISA_EXT_SxAIA		RISCV_ISA_EXT_SMAIA
>>  #else
>> +#define RISCV_ISA_EXT_Sx1p11		RISCV_ISA_EXT_SS1p11
>> +#define RISCV_ISA_EXT_Sx1p12		RISCV_ISA_EXT_SS1p12
>>  #define RISCV_ISA_EXT_SxAIA		RISCV_ISA_EXT_SSAIA
>>  #endif
>>  
>> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
>> index d11d6320fb0d..2e6b90ed0d51 100644
>> --- a/arch/riscv/kernel/cpu.c
>> +++ b/arch/riscv/kernel/cpu.c
>> @@ -215,6 +215,11 @@ static void print_isa(struct seq_file *f, const unsigned long *isa_bitmap)
>>  		if (!__riscv_isa_extension_available(isa_bitmap, riscv_isa_ext[i].id))
>>  			continue;
>>  
>> +		/* Only show the newest implemented version of an extension */
>> +		if (riscv_isa_ext[i].successor_id != RISCV_ISA_EXT_INVALID &&
>> +		    __riscv_isa_extension_available(isa_bitmap, riscv_isa_ext[i].successor_id))
>> +			continue;
>
> I'm not sure we need this. Expanding Ss1p12 to 'Ss1p11 Ss1p12' and then
> outputting both in the ISA string doesn't seem harmful to me. Also, using
> a successor field instead of supersedes field may make this logic easy,
> but it'll require updating old code (changing RISCV_ISA_EXT_INVALID to the
> new version extension ID) when new versions are added. A supersedes field
> wouldn't require old code updates and would match the profiles spec which
> have explicit 'Ss1p12 supersedes Ss1p11.' type sentences.

Seconding - suppressing implied extensions in /proc/cpuinfo will require anything
that parses /proc/cpuinfo to know about extension implication in order to
generate the complete list.

>> +
>>  		/* Only multi-letter extensions are split by underscores */
>>  		if (strnlen(riscv_isa_ext[i].name, 2) != 1)
>>  			seq_puts(f, "_");
>> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
>> index c5b13f7dd482..8e10b50120e9 100644
>> --- a/arch/riscv/kernel/cpufeature.c
>> +++ b/arch/riscv/kernel/cpufeature.c
>> @@ -113,23 +113,29 @@ static bool riscv_isa_extension_check(int id)
>>  	return true;
>>  }
>>  
>> -#define _RISCV_ISA_EXT_DATA(_name, _id, _subset_exts, _subset_exts_size) {	\
>> +#define _RISCV_ISA_EXT_DATA(_name, _id, _subset_exts, _subset_exts_size, _successor) {	\
>>  	.name = #_name,								\
>>  	.property = #_name,							\
>>  	.id = _id,								\
>>  	.subset_ext_ids = _subset_exts,						\
>> -	.subset_ext_size = _subset_exts_size					\
>> +	.subset_ext_size = _subset_exts_size,					\
>> +	.successor_id = _successor,						\
>>  }
>>  
>> -#define __RISCV_ISA_EXT_DATA(_name, _id) _RISCV_ISA_EXT_DATA(_name, _id, NULL, 0)
>> +#define __RISCV_ISA_EXT_DATA(_name, _id) \
>> +	_RISCV_ISA_EXT_DATA(_name, _id, NULL, 0, RISCV_ISA_EXT_INVALID)
>>  
>>  /* Used to declare pure "lasso" extension (Zk for instance) */
>>  #define __RISCV_ISA_EXT_BUNDLE(_name, _bundled_exts) \
>> -	_RISCV_ISA_EXT_DATA(_name, RISCV_ISA_EXT_INVALID, _bundled_exts, ARRAY_SIZE(_bundled_exts))
>> +	_RISCV_ISA_EXT_DATA(_name, RISCV_ISA_EXT_INVALID, \
>> +			    _bundled_exts, ARRAY_SIZE(_bundled_exts), RISCV_ISA_EXT_INVALID)
>>  
>>  /* Used to declare extensions that are a superset of other extensions (Zvbb for instance) */
>>  #define __RISCV_ISA_EXT_SUPERSET(_name, _id, _sub_exts) \
>> -	_RISCV_ISA_EXT_DATA(_name, _id, _sub_exts, ARRAY_SIZE(_sub_exts))
>> +	_RISCV_ISA_EXT_DATA(_name, _id, _sub_exts, ARRAY_SIZE(_sub_exts), RISCV_ISA_EXT_INVALID)
>> +
>> +#define __RISCV_ISA_EXT_VERSION(_name, _id, _preds, _preds_size, _successor) \
>> +	_RISCV_ISA_EXT_DATA(_name, _id, _preds, _preds_size, _successor)
>>  
>>  static const unsigned int riscv_zk_bundled_exts[] = {
>>  	RISCV_ISA_EXT_ZBKB,
>> @@ -201,6 +207,16 @@ static const unsigned int riscv_zvbb_exts[] = {
>>  	RISCV_ISA_EXT_ZVKB
>>  };
>>  
>> +static const unsigned int riscv_sm_ext_versions[] = {
>> +	RISCV_ISA_EXT_SM1p11,
>> +	RISCV_ISA_EXT_SM1p12,
>> +};
>> +
>> +static const unsigned int riscv_ss_ext_versions[] = {
>> +	RISCV_ISA_EXT_SS1p11,
>> +	RISCV_ISA_EXT_SS1p12,
>> +};
>> +
>>  /*
>>   * The canonical order of ISA extension names in the ISA string is defined in
>>   * chapter 27 of the unprivileged specification.
>> @@ -299,8 +315,16 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = {
>>  	__RISCV_ISA_EXT_DATA(zvksh, RISCV_ISA_EXT_ZVKSH),
>>  	__RISCV_ISA_EXT_BUNDLE(zvksg, riscv_zvksg_bundled_exts),
>>  	__RISCV_ISA_EXT_DATA(zvkt, RISCV_ISA_EXT_ZVKT),
>> +	__RISCV_ISA_EXT_VERSION(sm1p11, RISCV_ISA_EXT_SM1p11, riscv_sm_ext_versions, 0,
>> +				RISCV_ISA_EXT_SM1p12),
>> +	__RISCV_ISA_EXT_VERSION(sm1p12, RISCV_ISA_EXT_SM1p12, riscv_sm_ext_versions, 1,
>> +				RISCV_ISA_EXT_INVALID),
>>  	__RISCV_ISA_EXT_DATA(smaia, RISCV_ISA_EXT_SMAIA),
>>  	__RISCV_ISA_EXT_DATA(smstateen, RISCV_ISA_EXT_SMSTATEEN),
>> +	__RISCV_ISA_EXT_VERSION(ss1p11, RISCV_ISA_EXT_SS1p11, riscv_ss_ext_versions, 0,
>> +				RISCV_ISA_EXT_SS1p12),
>> +	__RISCV_ISA_EXT_VERSION(ss1p12, RISCV_ISA_EXT_SS1p12, riscv_ss_ext_versions, 1,
>> +				RISCV_ISA_EXT_INVALID),
>>  	__RISCV_ISA_EXT_DATA(ssaia, RISCV_ISA_EXT_SSAIA),
>>  	__RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
>>  	__RISCV_ISA_EXT_DATA(sstc, RISCV_ISA_EXT_SSTC),
>> @@ -414,6 +438,14 @@ static void __init riscv_parse_isa_string(unsigned long *this_hwcap, struct risc
>>  				;
>>  
>>  			++ext_end;
>> +
>> +			/*
>> +			 * As a special case for the Sm and Ss extensions, where the version
>> +			 * number is important, include it in the extension name.
>> +			 */
>> +			if (ext_end - ext == 2 && tolower(ext[0]) == 's' &&
>> +			    (tolower(ext[1]) == 'm' || tolower(ext[1]) == 's'))
>> +				ext_end = isa;

Per the strategy in [1] we treat every ratified version of every extension as a
unique extension; the unique aspect here is that more than one version of Sm/Ss
is defined.

[1]: https://lore.kernel.org/linux-riscv/20230608-sitting-bath-31eddc03c5a5@spud/

-s

>>  			break;
>>  		default:
>>  			/*
>> -- 
>> 2.43.0
>>
>
> Thanks,
> drew
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH -fixes v2 3/4] riscv: Add ISA extension parsing for Sm and Ss

[Samuel Holland]

On 2024-02-13 11:52 AM, Stefan O'Rear wrote:
> On Tue, Feb 13, 2024, at 10:14 AM, Andrew Jones wrote:
>> On Mon, Feb 12, 2024 at 07:37:34PM -0800, Samuel Holland wrote:
>>> Previously, all extension version numbers were ignored. However, the
>>> version number is important for these two extensions. The simplest way
>>> to implement this is to use a separate bitmap bit for each supported
>>> version, with each successive version implying all of the previous ones.
>>> This allows alternatives and riscv_has_extension_[un]likely() to work
>>> naturally.
>>>
>>> To avoid duplicate extensions in /proc/cpuinfo, the new successor_id
>>> field allows hiding all but the newest implemented version of an
>>> extension.
>>>
>>> Cc: <stable@vger.kernel.org> # v6.7+
>>> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
>>> ---
>>>
>>> Changes in v2:
>>>  - New patch for v2
>>>
>>>  arch/riscv/include/asm/cpufeature.h |  1 +
>>>  arch/riscv/include/asm/hwcap.h      |  8 ++++++
>>>  arch/riscv/kernel/cpu.c             |  5 ++++
>>>  arch/riscv/kernel/cpufeature.c      | 42 +++++++++++++++++++++++++----
>>>  4 files changed, 51 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/arch/riscv/include/asm/cpufeature.h b/arch/riscv/include/asm/cpufeature.h
>>> index 0bd11862b760..ac71384e7bc4 100644
>>> --- a/arch/riscv/include/asm/cpufeature.h
>>> +++ b/arch/riscv/include/asm/cpufeature.h
>>> @@ -61,6 +61,7 @@ struct riscv_isa_ext_data {
>>>  	const char *property;
>>>  	const unsigned int *subset_ext_ids;
>>>  	const unsigned int subset_ext_size;
>>> +	const unsigned int successor_id;
>>>  };
>>>  
>>>  extern const struct riscv_isa_ext_data riscv_isa_ext[];
>>> diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
>>> index 5340f818746b..5b51aa1db15b 100644
>>> --- a/arch/riscv/include/asm/hwcap.h
>>> +++ b/arch/riscv/include/asm/hwcap.h
>>> @@ -80,13 +80,21 @@
>>>  #define RISCV_ISA_EXT_ZFA		71
>>>  #define RISCV_ISA_EXT_ZTSO		72
>>>  #define RISCV_ISA_EXT_ZACAS		73
>>> +#define RISCV_ISA_EXT_SM1p11		74
>>> +#define RISCV_ISA_EXT_SM1p12		75
>>> +#define RISCV_ISA_EXT_SS1p11		76
>>> +#define RISCV_ISA_EXT_SS1p12		77
>>>  
>>>  #define RISCV_ISA_EXT_MAX		128
>>>  #define RISCV_ISA_EXT_INVALID		U32_MAX
>>>  
>>>  #ifdef CONFIG_RISCV_M_MODE
>>> +#define RISCV_ISA_EXT_Sx1p11		RISCV_ISA_EXT_SM1p11
>>> +#define RISCV_ISA_EXT_Sx1p12		RISCV_ISA_EXT_SM1p12
>>>  #define RISCV_ISA_EXT_SxAIA		RISCV_ISA_EXT_SMAIA
>>>  #else
>>> +#define RISCV_ISA_EXT_Sx1p11		RISCV_ISA_EXT_SS1p11
>>> +#define RISCV_ISA_EXT_Sx1p12		RISCV_ISA_EXT_SS1p12
>>>  #define RISCV_ISA_EXT_SxAIA		RISCV_ISA_EXT_SSAIA
>>>  #endif
>>>  
>>> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
>>> index d11d6320fb0d..2e6b90ed0d51 100644
>>> --- a/arch/riscv/kernel/cpu.c
>>> +++ b/arch/riscv/kernel/cpu.c
>>> @@ -215,6 +215,11 @@ static void print_isa(struct seq_file *f, const unsigned long *isa_bitmap)
>>>  		if (!__riscv_isa_extension_available(isa_bitmap, riscv_isa_ext[i].id))
>>>  			continue;
>>>  
>>> +		/* Only show the newest implemented version of an extension */
>>> +		if (riscv_isa_ext[i].successor_id != RISCV_ISA_EXT_INVALID &&
>>> +		    __riscv_isa_extension_available(isa_bitmap, riscv_isa_ext[i].successor_id))
>>> +			continue;
>>
>> I'm not sure we need this. Expanding Ss1p12 to 'Ss1p11 Ss1p12' and then
>> outputting both in the ISA string doesn't seem harmful to me. Also, using

I was thinking that parsers would be confused by seeing the same extension
multiple times, but I have no problem with it if folks disagree.

>> a successor field instead of supersedes field may make this logic easy,
>> but it'll require updating old code (changing RISCV_ISA_EXT_INVALID to the
>> new version extension ID) when new versions are added. A supersedes field
>> wouldn't require old code updates and would match the profiles spec which
>> have explicit 'Ss1p12 supersedes Ss1p11.' type sentences.
> 
> Seconding - suppressing implied extensions in /proc/cpuinfo will require anything
> that parses /proc/cpuinfo to know about extension implication in order to
> generate the complete list.

Well, from an ISA string perspective (i.e. what's shown in /proc/cpuinfo), only
including the latest version _does_ give you the complete list, because Ss1p11
and Ss1p12 aren't different extensions. I'm only expanding them to different
flags inside the kernel so we can avoid storing the version number as an integer
and doing numeric comparisons at each usage site.

So /proc/cpuinfo parsers wouldn't need to know about implication, since that's a
kernel implementation detail. They just need to know how to parse the version
number from an ISA string. And I would expect them to be able to do that anyway.

There would only be backward-compatibility concerns if parsers are doing a
string match on the whole "ss1p12", which I would consider wrong to start with.

Regards,
Samuel


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH -fixes v2 3/4] riscv: Add ISA extension parsing for Sm and Ss

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 8043 bytes --]

On Mon, Feb 12, 2024 at 07:37:34PM -0800, Samuel Holland wrote:
> Previously, all extension version numbers were ignored. However, the
> version number is important for these two extensions. The simplest way
> to implement this is to use a separate bitmap bit for each supported
> version, with each successive version implying all of the previous ones.
> This allows alternatives and riscv_has_extension_[un]likely() to work
> naturally.
> 
> To avoid duplicate extensions in /proc/cpuinfo, the new successor_id
> field allows hiding all but the newest implemented version of an
> extension.
> 
> Cc: <stable@vger.kernel.org> # v6.7+
> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
> ---
> 
> Changes in v2:
>  - New patch for v2
> 
>  arch/riscv/include/asm/cpufeature.h |  1 +
>  arch/riscv/include/asm/hwcap.h      |  8 ++++++
>  arch/riscv/kernel/cpu.c             |  5 ++++
>  arch/riscv/kernel/cpufeature.c      | 42 +++++++++++++++++++++++++----
>  4 files changed, 51 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/riscv/include/asm/cpufeature.h b/arch/riscv/include/asm/cpufeature.h
> index 0bd11862b760..ac71384e7bc4 100644
> --- a/arch/riscv/include/asm/cpufeature.h
> +++ b/arch/riscv/include/asm/cpufeature.h
> @@ -61,6 +61,7 @@ struct riscv_isa_ext_data {
>  	const char *property;
>  	const unsigned int *subset_ext_ids;
>  	const unsigned int subset_ext_size;
> +	const unsigned int successor_id;
>  };
>  
>  extern const struct riscv_isa_ext_data riscv_isa_ext[];
> diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> index 5340f818746b..5b51aa1db15b 100644
> --- a/arch/riscv/include/asm/hwcap.h
> +++ b/arch/riscv/include/asm/hwcap.h
> @@ -80,13 +80,21 @@
>  #define RISCV_ISA_EXT_ZFA		71
>  #define RISCV_ISA_EXT_ZTSO		72
>  #define RISCV_ISA_EXT_ZACAS		73
> +#define RISCV_ISA_EXT_SM1p11		74
> +#define RISCV_ISA_EXT_SM1p12		75
> +#define RISCV_ISA_EXT_SS1p11		76
> +#define RISCV_ISA_EXT_SS1p12		77
>  
>  #define RISCV_ISA_EXT_MAX		128
>  #define RISCV_ISA_EXT_INVALID		U32_MAX
>  
>  #ifdef CONFIG_RISCV_M_MODE
> +#define RISCV_ISA_EXT_Sx1p11		RISCV_ISA_EXT_SM1p11
> +#define RISCV_ISA_EXT_Sx1p12		RISCV_ISA_EXT_SM1p12
>  #define RISCV_ISA_EXT_SxAIA		RISCV_ISA_EXT_SMAIA
>  #else
> +#define RISCV_ISA_EXT_Sx1p11		RISCV_ISA_EXT_SS1p11
> +#define RISCV_ISA_EXT_Sx1p12		RISCV_ISA_EXT_SS1p12
>  #define RISCV_ISA_EXT_SxAIA		RISCV_ISA_EXT_SSAIA
>  #endif
>  
> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> index d11d6320fb0d..2e6b90ed0d51 100644
> --- a/arch/riscv/kernel/cpu.c
> +++ b/arch/riscv/kernel/cpu.c
> @@ -215,6 +215,11 @@ static void print_isa(struct seq_file *f, const unsigned long *isa_bitmap)
>  		if (!__riscv_isa_extension_available(isa_bitmap, riscv_isa_ext[i].id))
>  			continue;
>  
> +		/* Only show the newest implemented version of an extension */
> +		if (riscv_isa_ext[i].successor_id != RISCV_ISA_EXT_INVALID &&
> +		    __riscv_isa_extension_available(isa_bitmap, riscv_isa_ext[i].successor_id))
> +			continue;
> +
>  		/* Only multi-letter extensions are split by underscores */
>  		if (strnlen(riscv_isa_ext[i].name, 2) != 1)
>  			seq_puts(f, "_");
> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index c5b13f7dd482..8e10b50120e9 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -113,23 +113,29 @@ static bool riscv_isa_extension_check(int id)
>  	return true;
>  }
>  
> -#define _RISCV_ISA_EXT_DATA(_name, _id, _subset_exts, _subset_exts_size) {	\
> +#define _RISCV_ISA_EXT_DATA(_name, _id, _subset_exts, _subset_exts_size, _successor) {	\
>  	.name = #_name,								\
>  	.property = #_name,							\
>  	.id = _id,								\
>  	.subset_ext_ids = _subset_exts,						\
> -	.subset_ext_size = _subset_exts_size					\
> +	.subset_ext_size = _subset_exts_size,					\
> +	.successor_id = _successor,						\
>  }
>  
> -#define __RISCV_ISA_EXT_DATA(_name, _id) _RISCV_ISA_EXT_DATA(_name, _id, NULL, 0)
> +#define __RISCV_ISA_EXT_DATA(_name, _id) \
> +	_RISCV_ISA_EXT_DATA(_name, _id, NULL, 0, RISCV_ISA_EXT_INVALID)
>  
>  /* Used to declare pure "lasso" extension (Zk for instance) */
>  #define __RISCV_ISA_EXT_BUNDLE(_name, _bundled_exts) \
> -	_RISCV_ISA_EXT_DATA(_name, RISCV_ISA_EXT_INVALID, _bundled_exts, ARRAY_SIZE(_bundled_exts))
> +	_RISCV_ISA_EXT_DATA(_name, RISCV_ISA_EXT_INVALID, \
> +			    _bundled_exts, ARRAY_SIZE(_bundled_exts), RISCV_ISA_EXT_INVALID)
>  
>  /* Used to declare extensions that are a superset of other extensions (Zvbb for instance) */
>  #define __RISCV_ISA_EXT_SUPERSET(_name, _id, _sub_exts) \
> -	_RISCV_ISA_EXT_DATA(_name, _id, _sub_exts, ARRAY_SIZE(_sub_exts))
> +	_RISCV_ISA_EXT_DATA(_name, _id, _sub_exts, ARRAY_SIZE(_sub_exts), RISCV_ISA_EXT_INVALID)
> +
> +#define __RISCV_ISA_EXT_VERSION(_name, _id, _preds, _preds_size, _successor) \
> +	_RISCV_ISA_EXT_DATA(_name, _id, _preds, _preds_size, _successor)
>  
>  static const unsigned int riscv_zk_bundled_exts[] = {
>  	RISCV_ISA_EXT_ZBKB,
> @@ -201,6 +207,16 @@ static const unsigned int riscv_zvbb_exts[] = {
>  	RISCV_ISA_EXT_ZVKB
>  };
>  
> +static const unsigned int riscv_sm_ext_versions[] = {
> +	RISCV_ISA_EXT_SM1p11,
> +	RISCV_ISA_EXT_SM1p12,
> +};
> +
> +static const unsigned int riscv_ss_ext_versions[] = {
> +	RISCV_ISA_EXT_SS1p11,
> +	RISCV_ISA_EXT_SS1p12,
> +};
> +
>  /*
>   * The canonical order of ISA extension names in the ISA string is defined in
>   * chapter 27 of the unprivileged specification.
> @@ -299,8 +315,16 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = {
>  	__RISCV_ISA_EXT_DATA(zvksh, RISCV_ISA_EXT_ZVKSH),
>  	__RISCV_ISA_EXT_BUNDLE(zvksg, riscv_zvksg_bundled_exts),
>  	__RISCV_ISA_EXT_DATA(zvkt, RISCV_ISA_EXT_ZVKT),
> +	__RISCV_ISA_EXT_VERSION(sm1p11, RISCV_ISA_EXT_SM1p11, riscv_sm_ext_versions, 0,
> +				RISCV_ISA_EXT_SM1p12),
> +	__RISCV_ISA_EXT_VERSION(sm1p12, RISCV_ISA_EXT_SM1p12, riscv_sm_ext_versions, 1,
> +				RISCV_ISA_EXT_INVALID),
>  	__RISCV_ISA_EXT_DATA(smaia, RISCV_ISA_EXT_SMAIA),
>  	__RISCV_ISA_EXT_DATA(smstateen, RISCV_ISA_EXT_SMSTATEEN),
> +	__RISCV_ISA_EXT_VERSION(ss1p11, RISCV_ISA_EXT_SS1p11, riscv_ss_ext_versions, 0,
> +				RISCV_ISA_EXT_SS1p12),
> +	__RISCV_ISA_EXT_VERSION(ss1p12, RISCV_ISA_EXT_SS1p12, riscv_ss_ext_versions, 1,
> +				RISCV_ISA_EXT_INVALID),
>  	__RISCV_ISA_EXT_DATA(ssaia, RISCV_ISA_EXT_SSAIA),
>  	__RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
>  	__RISCV_ISA_EXT_DATA(sstc, RISCV_ISA_EXT_SSTC),
> @@ -414,6 +438,14 @@ static void __init riscv_parse_isa_string(unsigned long *this_hwcap, struct risc
>  				;
>  
>  			++ext_end;
> +
> +			/*
> +			 * As a special case for the Sm and Ss extensions, where the version
> +			 * number is important, include it in the extension name.
> +			 */
> +			if (ext_end - ext == 2 && tolower(ext[0]) == 's' &&
> +			    (tolower(ext[1]) == 'm' || tolower(ext[1]) == 's'))
> +				ext_end = isa;
>  			break;
>  		default:
>  			/*


Hmm, looking at all of this (especially this hack to the "old" parser),
I feel more like these should be promoted to a property of their own.
The "old" parser was designed to handle numbers, and here when you're
interested in the values behind the numbers (which is a first iirc), you
don't make any use of that. I don't really want to see a world where
we have every single iteration of smNpM under the sun in the property,
because there's a fair bit of churn in the isa. Granted, this applies to
all the various, the difference for me is the level of churn.
Or maybe we can still with the properties you have, but instead of
treating them like any other extension, handle these separately,
focusing on the numbering, so that only having the exact version
supported by a cpu is possible.

I'm still pretty undecided, I'd like to think about this a little bit,
but I think we can do better here.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH -fixes v2 3/4] riscv: Add ISA extension parsing for Sm and Ss

[Samuel Holland]

On 2024-02-13 12:07 PM, Conor Dooley wrote:
> On Mon, Feb 12, 2024 at 07:37:34PM -0800, Samuel Holland wrote:
>> Previously, all extension version numbers were ignored. However, the
>> version number is important for these two extensions. The simplest way
>> to implement this is to use a separate bitmap bit for each supported
>> version, with each successive version implying all of the previous ones.
>> This allows alternatives and riscv_has_extension_[un]likely() to work
>> naturally.
>>
>> To avoid duplicate extensions in /proc/cpuinfo, the new successor_id
>> field allows hiding all but the newest implemented version of an
>> extension.
>>
>> Cc: <stable@vger.kernel.org> # v6.7+
>> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
>> ---
>>
>> Changes in v2:
>>  - New patch for v2
>>
>>  arch/riscv/include/asm/cpufeature.h |  1 +
>>  arch/riscv/include/asm/hwcap.h      |  8 ++++++
>>  arch/riscv/kernel/cpu.c             |  5 ++++
>>  arch/riscv/kernel/cpufeature.c      | 42 +++++++++++++++++++++++++----
>>  4 files changed, 51 insertions(+), 5 deletions(-)
>>
>> diff --git a/arch/riscv/include/asm/cpufeature.h b/arch/riscv/include/asm/cpufeature.h
>> index 0bd11862b760..ac71384e7bc4 100644
>> --- a/arch/riscv/include/asm/cpufeature.h
>> +++ b/arch/riscv/include/asm/cpufeature.h
>> @@ -61,6 +61,7 @@ struct riscv_isa_ext_data {
>>  	const char *property;
>>  	const unsigned int *subset_ext_ids;
>>  	const unsigned int subset_ext_size;
>> +	const unsigned int successor_id;
>>  };
>>  
>>  extern const struct riscv_isa_ext_data riscv_isa_ext[];
>> diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
>> index 5340f818746b..5b51aa1db15b 100644
>> --- a/arch/riscv/include/asm/hwcap.h
>> +++ b/arch/riscv/include/asm/hwcap.h
>> @@ -80,13 +80,21 @@
>>  #define RISCV_ISA_EXT_ZFA		71
>>  #define RISCV_ISA_EXT_ZTSO		72
>>  #define RISCV_ISA_EXT_ZACAS		73
>> +#define RISCV_ISA_EXT_SM1p11		74
>> +#define RISCV_ISA_EXT_SM1p12		75
>> +#define RISCV_ISA_EXT_SS1p11		76
>> +#define RISCV_ISA_EXT_SS1p12		77
>>  
>>  #define RISCV_ISA_EXT_MAX		128
>>  #define RISCV_ISA_EXT_INVALID		U32_MAX
>>  
>>  #ifdef CONFIG_RISCV_M_MODE
>> +#define RISCV_ISA_EXT_Sx1p11		RISCV_ISA_EXT_SM1p11
>> +#define RISCV_ISA_EXT_Sx1p12		RISCV_ISA_EXT_SM1p12
>>  #define RISCV_ISA_EXT_SxAIA		RISCV_ISA_EXT_SMAIA
>>  #else
>> +#define RISCV_ISA_EXT_Sx1p11		RISCV_ISA_EXT_SS1p11
>> +#define RISCV_ISA_EXT_Sx1p12		RISCV_ISA_EXT_SS1p12
>>  #define RISCV_ISA_EXT_SxAIA		RISCV_ISA_EXT_SSAIA
>>  #endif
>>  
>> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
>> index d11d6320fb0d..2e6b90ed0d51 100644
>> --- a/arch/riscv/kernel/cpu.c
>> +++ b/arch/riscv/kernel/cpu.c
>> @@ -215,6 +215,11 @@ static void print_isa(struct seq_file *f, const unsigned long *isa_bitmap)
>>  		if (!__riscv_isa_extension_available(isa_bitmap, riscv_isa_ext[i].id))
>>  			continue;
>>  
>> +		/* Only show the newest implemented version of an extension */
>> +		if (riscv_isa_ext[i].successor_id != RISCV_ISA_EXT_INVALID &&
>> +		    __riscv_isa_extension_available(isa_bitmap, riscv_isa_ext[i].successor_id))
>> +			continue;
>> +
>>  		/* Only multi-letter extensions are split by underscores */
>>  		if (strnlen(riscv_isa_ext[i].name, 2) != 1)
>>  			seq_puts(f, "_");
>> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
>> index c5b13f7dd482..8e10b50120e9 100644
>> --- a/arch/riscv/kernel/cpufeature.c
>> +++ b/arch/riscv/kernel/cpufeature.c
>> @@ -113,23 +113,29 @@ static bool riscv_isa_extension_check(int id)
>>  	return true;
>>  }
>>  
>> -#define _RISCV_ISA_EXT_DATA(_name, _id, _subset_exts, _subset_exts_size) {	\
>> +#define _RISCV_ISA_EXT_DATA(_name, _id, _subset_exts, _subset_exts_size, _successor) {	\
>>  	.name = #_name,								\
>>  	.property = #_name,							\
>>  	.id = _id,								\
>>  	.subset_ext_ids = _subset_exts,						\
>> -	.subset_ext_size = _subset_exts_size					\
>> +	.subset_ext_size = _subset_exts_size,					\
>> +	.successor_id = _successor,						\
>>  }
>>  
>> -#define __RISCV_ISA_EXT_DATA(_name, _id) _RISCV_ISA_EXT_DATA(_name, _id, NULL, 0)
>> +#define __RISCV_ISA_EXT_DATA(_name, _id) \
>> +	_RISCV_ISA_EXT_DATA(_name, _id, NULL, 0, RISCV_ISA_EXT_INVALID)
>>  
>>  /* Used to declare pure "lasso" extension (Zk for instance) */
>>  #define __RISCV_ISA_EXT_BUNDLE(_name, _bundled_exts) \
>> -	_RISCV_ISA_EXT_DATA(_name, RISCV_ISA_EXT_INVALID, _bundled_exts, ARRAY_SIZE(_bundled_exts))
>> +	_RISCV_ISA_EXT_DATA(_name, RISCV_ISA_EXT_INVALID, \
>> +			    _bundled_exts, ARRAY_SIZE(_bundled_exts), RISCV_ISA_EXT_INVALID)
>>  
>>  /* Used to declare extensions that are a superset of other extensions (Zvbb for instance) */
>>  #define __RISCV_ISA_EXT_SUPERSET(_name, _id, _sub_exts) \
>> -	_RISCV_ISA_EXT_DATA(_name, _id, _sub_exts, ARRAY_SIZE(_sub_exts))
>> +	_RISCV_ISA_EXT_DATA(_name, _id, _sub_exts, ARRAY_SIZE(_sub_exts), RISCV_ISA_EXT_INVALID)
>> +
>> +#define __RISCV_ISA_EXT_VERSION(_name, _id, _preds, _preds_size, _successor) \
>> +	_RISCV_ISA_EXT_DATA(_name, _id, _preds, _preds_size, _successor)
>>  
>>  static const unsigned int riscv_zk_bundled_exts[] = {
>>  	RISCV_ISA_EXT_ZBKB,
>> @@ -201,6 +207,16 @@ static const unsigned int riscv_zvbb_exts[] = {
>>  	RISCV_ISA_EXT_ZVKB
>>  };
>>  
>> +static const unsigned int riscv_sm_ext_versions[] = {
>> +	RISCV_ISA_EXT_SM1p11,
>> +	RISCV_ISA_EXT_SM1p12,
>> +};
>> +
>> +static const unsigned int riscv_ss_ext_versions[] = {
>> +	RISCV_ISA_EXT_SS1p11,
>> +	RISCV_ISA_EXT_SS1p12,
>> +};
>> +
>>  /*
>>   * The canonical order of ISA extension names in the ISA string is defined in
>>   * chapter 27 of the unprivileged specification.
>> @@ -299,8 +315,16 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = {
>>  	__RISCV_ISA_EXT_DATA(zvksh, RISCV_ISA_EXT_ZVKSH),
>>  	__RISCV_ISA_EXT_BUNDLE(zvksg, riscv_zvksg_bundled_exts),
>>  	__RISCV_ISA_EXT_DATA(zvkt, RISCV_ISA_EXT_ZVKT),
>> +	__RISCV_ISA_EXT_VERSION(sm1p11, RISCV_ISA_EXT_SM1p11, riscv_sm_ext_versions, 0,
>> +				RISCV_ISA_EXT_SM1p12),
>> +	__RISCV_ISA_EXT_VERSION(sm1p12, RISCV_ISA_EXT_SM1p12, riscv_sm_ext_versions, 1,
>> +				RISCV_ISA_EXT_INVALID),
>>  	__RISCV_ISA_EXT_DATA(smaia, RISCV_ISA_EXT_SMAIA),
>>  	__RISCV_ISA_EXT_DATA(smstateen, RISCV_ISA_EXT_SMSTATEEN),
>> +	__RISCV_ISA_EXT_VERSION(ss1p11, RISCV_ISA_EXT_SS1p11, riscv_ss_ext_versions, 0,
>> +				RISCV_ISA_EXT_SS1p12),
>> +	__RISCV_ISA_EXT_VERSION(ss1p12, RISCV_ISA_EXT_SS1p12, riscv_ss_ext_versions, 1,
>> +				RISCV_ISA_EXT_INVALID),
>>  	__RISCV_ISA_EXT_DATA(ssaia, RISCV_ISA_EXT_SSAIA),
>>  	__RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
>>  	__RISCV_ISA_EXT_DATA(sstc, RISCV_ISA_EXT_SSTC),
>> @@ -414,6 +438,14 @@ static void __init riscv_parse_isa_string(unsigned long *this_hwcap, struct risc
>>  				;
>>  
>>  			++ext_end;
>> +
>> +			/*
>> +			 * As a special case for the Sm and Ss extensions, where the version
>> +			 * number is important, include it in the extension name.
>> +			 */
>> +			if (ext_end - ext == 2 && tolower(ext[0]) == 's' &&
>> +			    (tolower(ext[1]) == 'm' || tolower(ext[1]) == 's'))
>> +				ext_end = isa;
>>  			break;
>>  		default:
>>  			/*
> 
> 
> Hmm, looking at all of this (especially this hack to the "old" parser),
> I feel more like these should be promoted to a property of their own.
> The "old" parser was designed to handle numbers, and here when you're
> interested in the values behind the numbers (which is a first iirc), you
> don't make any use of that. I don't really want to see a world where

I had a version of this code that parsed the numbers and stored them as integers
in `struct riscv_isainfo`. It didn't work with of_property_match_string() as
used for riscv,isa-extensions, since that function expects the extension name to
be the full string. Either we would need to change the code to parse a version
number out of each string in the riscv,isa-extensions list (and make the binding
a bunch of regexes), or we need a separate "extension" entry (and DT binding
entry) for each supported version.

I chose the second option, and as a consequence I didn't actually need to parse
the integer value in the ISA string code path either.

> we have every single iteration of smNpM under the sun in the property,
> because there's a fair bit of churn in the isa. Granted, this applies to
> all the various, the difference for me is the level of churn.

Indeed. In fact, one thought I had while looking at this code is that we should
be ignoring any extension in the ISA string with a version < 1.0 or >= 2.0,
since those won't be compatible with what we expect.

> Or maybe we can still with the properties you have, but instead of
> treating them like any other extension, handle these separately,
> focusing on the numbering, so that only having the exact version
> supported by a cpu is possible.

Maybe I'm misunderstanding what you're saying here, but it is already the case
that the DT for a CPU would only contain the exact version of the privileged ISA
supported by that CPU.

With this implementation, the fact that the integer version gets expanded to a
series of flags is supposed to be invisible in the DT and to userspace. I
realize I don't quite succeed there: putting "ss1p13" in the ISA string should
work, but does not.

> I'm still pretty undecided, I'd like to think about this a little bit,
> but I think we can do better here.

Sure, no problem. I'm happy to implement whatever we agree on. Though one
consideration I had is that this is all in support of fixing a bug in v6.7, so I
wanted the changes to be backportable.

I suppose the easy way out for backporting is to check for RISCV_ISA_EXT_ZICBOZ
for now, and then solve the larger problem once there is some other user of the
envcfg CSR (or another Ss1p12 feature).

Regards,
Samuel


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH -fixes v2 3/4] riscv: Add ISA extension parsing for Sm and Ss

[Stefan O'Rear]

On Tue, Feb 13, 2024, at 3:22 PM, Samuel Holland wrote:
> On 2024-02-13 12:07 PM, Conor Dooley wrote:
>> On Mon, Feb 12, 2024 at 07:37:34PM -0800, Samuel Holland wrote:
>>> Previously, all extension version numbers were ignored. However, the
>>> version number is important for these two extensions. The simplest way
>>> to implement this is to use a separate bitmap bit for each supported
>>> version, with each successive version implying all of the previous ones.
>>> This allows alternatives and riscv_has_extension_[un]likely() to work
>>> naturally.
>>>
>>> To avoid duplicate extensions in /proc/cpuinfo, the new successor_id
>>> field allows hiding all but the newest implemented version of an
>>> extension.
>>>
>>> Cc: <stable@vger.kernel.org> # v6.7+
>>> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
>>> ---
>>>
>>> Changes in v2:
>>>  - New patch for v2
>>>
>>>  arch/riscv/include/asm/cpufeature.h |  1 +
>>>  arch/riscv/include/asm/hwcap.h      |  8 ++++++
>>>  arch/riscv/kernel/cpu.c             |  5 ++++
>>>  arch/riscv/kernel/cpufeature.c      | 42 +++++++++++++++++++++++++----
>>>  4 files changed, 51 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/arch/riscv/include/asm/cpufeature.h b/arch/riscv/include/asm/cpufeature.h
>>> index 0bd11862b760..ac71384e7bc4 100644
>>> --- a/arch/riscv/include/asm/cpufeature.h
>>> +++ b/arch/riscv/include/asm/cpufeature.h
>>> @@ -61,6 +61,7 @@ struct riscv_isa_ext_data {
>>>  	const char *property;
>>>  	const unsigned int *subset_ext_ids;
>>>  	const unsigned int subset_ext_size;
>>> +	const unsigned int successor_id;
>>>  };
>>>  
>>>  extern const struct riscv_isa_ext_data riscv_isa_ext[];
>>> diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
>>> index 5340f818746b..5b51aa1db15b 100644
>>> --- a/arch/riscv/include/asm/hwcap.h
>>> +++ b/arch/riscv/include/asm/hwcap.h
>>> @@ -80,13 +80,21 @@
>>>  #define RISCV_ISA_EXT_ZFA		71
>>>  #define RISCV_ISA_EXT_ZTSO		72
>>>  #define RISCV_ISA_EXT_ZACAS		73
>>> +#define RISCV_ISA_EXT_SM1p11		74
>>> +#define RISCV_ISA_EXT_SM1p12		75
>>> +#define RISCV_ISA_EXT_SS1p11		76
>>> +#define RISCV_ISA_EXT_SS1p12		77
>>>  
>>>  #define RISCV_ISA_EXT_MAX		128
>>>  #define RISCV_ISA_EXT_INVALID		U32_MAX
>>>  
>>>  #ifdef CONFIG_RISCV_M_MODE
>>> +#define RISCV_ISA_EXT_Sx1p11		RISCV_ISA_EXT_SM1p11
>>> +#define RISCV_ISA_EXT_Sx1p12		RISCV_ISA_EXT_SM1p12
>>>  #define RISCV_ISA_EXT_SxAIA		RISCV_ISA_EXT_SMAIA
>>>  #else
>>> +#define RISCV_ISA_EXT_Sx1p11		RISCV_ISA_EXT_SS1p11
>>> +#define RISCV_ISA_EXT_Sx1p12		RISCV_ISA_EXT_SS1p12
>>>  #define RISCV_ISA_EXT_SxAIA		RISCV_ISA_EXT_SSAIA
>>>  #endif
>>>  
>>> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
>>> index d11d6320fb0d..2e6b90ed0d51 100644
>>> --- a/arch/riscv/kernel/cpu.c
>>> +++ b/arch/riscv/kernel/cpu.c
>>> @@ -215,6 +215,11 @@ static void print_isa(struct seq_file *f, const unsigned long *isa_bitmap)
>>>  		if (!__riscv_isa_extension_available(isa_bitmap, riscv_isa_ext[i].id))
>>>  			continue;
>>>  
>>> +		/* Only show the newest implemented version of an extension */
>>> +		if (riscv_isa_ext[i].successor_id != RISCV_ISA_EXT_INVALID &&
>>> +		    __riscv_isa_extension_available(isa_bitmap, riscv_isa_ext[i].successor_id))
>>> +			continue;
>>> +
>>>  		/* Only multi-letter extensions are split by underscores */
>>>  		if (strnlen(riscv_isa_ext[i].name, 2) != 1)
>>>  			seq_puts(f, "_");
>>> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
>>> index c5b13f7dd482..8e10b50120e9 100644
>>> --- a/arch/riscv/kernel/cpufeature.c
>>> +++ b/arch/riscv/kernel/cpufeature.c
>>> @@ -113,23 +113,29 @@ static bool riscv_isa_extension_check(int id)
>>>  	return true;
>>>  }
>>>  
>>> -#define _RISCV_ISA_EXT_DATA(_name, _id, _subset_exts, _subset_exts_size) {	\
>>> +#define _RISCV_ISA_EXT_DATA(_name, _id, _subset_exts, _subset_exts_size, _successor) {	\
>>>  	.name = #_name,								\
>>>  	.property = #_name,							\
>>>  	.id = _id,								\
>>>  	.subset_ext_ids = _subset_exts,						\
>>> -	.subset_ext_size = _subset_exts_size					\
>>> +	.subset_ext_size = _subset_exts_size,					\
>>> +	.successor_id = _successor,						\
>>>  }
>>>  
>>> -#define __RISCV_ISA_EXT_DATA(_name, _id) _RISCV_ISA_EXT_DATA(_name, _id, NULL, 0)
>>> +#define __RISCV_ISA_EXT_DATA(_name, _id) \
>>> +	_RISCV_ISA_EXT_DATA(_name, _id, NULL, 0, RISCV_ISA_EXT_INVALID)
>>>  
>>>  /* Used to declare pure "lasso" extension (Zk for instance) */
>>>  #define __RISCV_ISA_EXT_BUNDLE(_name, _bundled_exts) \
>>> -	_RISCV_ISA_EXT_DATA(_name, RISCV_ISA_EXT_INVALID, _bundled_exts, ARRAY_SIZE(_bundled_exts))
>>> +	_RISCV_ISA_EXT_DATA(_name, RISCV_ISA_EXT_INVALID, \
>>> +			    _bundled_exts, ARRAY_SIZE(_bundled_exts), RISCV_ISA_EXT_INVALID)
>>>  
>>>  /* Used to declare extensions that are a superset of other extensions (Zvbb for instance) */
>>>  #define __RISCV_ISA_EXT_SUPERSET(_name, _id, _sub_exts) \
>>> -	_RISCV_ISA_EXT_DATA(_name, _id, _sub_exts, ARRAY_SIZE(_sub_exts))
>>> +	_RISCV_ISA_EXT_DATA(_name, _id, _sub_exts, ARRAY_SIZE(_sub_exts), RISCV_ISA_EXT_INVALID)
>>> +
>>> +#define __RISCV_ISA_EXT_VERSION(_name, _id, _preds, _preds_size, _successor) \
>>> +	_RISCV_ISA_EXT_DATA(_name, _id, _preds, _preds_size, _successor)
>>>  
>>>  static const unsigned int riscv_zk_bundled_exts[] = {
>>>  	RISCV_ISA_EXT_ZBKB,
>>> @@ -201,6 +207,16 @@ static const unsigned int riscv_zvbb_exts[] = {
>>>  	RISCV_ISA_EXT_ZVKB
>>>  };
>>>  
>>> +static const unsigned int riscv_sm_ext_versions[] = {
>>> +	RISCV_ISA_EXT_SM1p11,
>>> +	RISCV_ISA_EXT_SM1p12,
>>> +};
>>> +
>>> +static const unsigned int riscv_ss_ext_versions[] = {
>>> +	RISCV_ISA_EXT_SS1p11,
>>> +	RISCV_ISA_EXT_SS1p12,
>>> +};
>>> +
>>>  /*
>>>   * The canonical order of ISA extension names in the ISA string is defined in
>>>   * chapter 27 of the unprivileged specification.
>>> @@ -299,8 +315,16 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = {
>>>  	__RISCV_ISA_EXT_DATA(zvksh, RISCV_ISA_EXT_ZVKSH),
>>>  	__RISCV_ISA_EXT_BUNDLE(zvksg, riscv_zvksg_bundled_exts),
>>>  	__RISCV_ISA_EXT_DATA(zvkt, RISCV_ISA_EXT_ZVKT),
>>> +	__RISCV_ISA_EXT_VERSION(sm1p11, RISCV_ISA_EXT_SM1p11, riscv_sm_ext_versions, 0,
>>> +				RISCV_ISA_EXT_SM1p12),
>>> +	__RISCV_ISA_EXT_VERSION(sm1p12, RISCV_ISA_EXT_SM1p12, riscv_sm_ext_versions, 1,
>>> +				RISCV_ISA_EXT_INVALID),
>>>  	__RISCV_ISA_EXT_DATA(smaia, RISCV_ISA_EXT_SMAIA),
>>>  	__RISCV_ISA_EXT_DATA(smstateen, RISCV_ISA_EXT_SMSTATEEN),
>>> +	__RISCV_ISA_EXT_VERSION(ss1p11, RISCV_ISA_EXT_SS1p11, riscv_ss_ext_versions, 0,
>>> +				RISCV_ISA_EXT_SS1p12),
>>> +	__RISCV_ISA_EXT_VERSION(ss1p12, RISCV_ISA_EXT_SS1p12, riscv_ss_ext_versions, 1,
>>> +				RISCV_ISA_EXT_INVALID),
>>>  	__RISCV_ISA_EXT_DATA(ssaia, RISCV_ISA_EXT_SSAIA),
>>>  	__RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
>>>  	__RISCV_ISA_EXT_DATA(sstc, RISCV_ISA_EXT_SSTC),
>>> @@ -414,6 +438,14 @@ static void __init riscv_parse_isa_string(unsigned long *this_hwcap, struct risc
>>>  				;
>>>  
>>>  			++ext_end;
>>> +
>>> +			/*
>>> +			 * As a special case for the Sm and Ss extensions, where the version
>>> +			 * number is important, include it in the extension name.
>>> +			 */
>>> +			if (ext_end - ext == 2 && tolower(ext[0]) == 's' &&
>>> +			    (tolower(ext[1]) == 'm' || tolower(ext[1]) == 's'))
>>> +				ext_end = isa;
>>>  			break;
>>>  		default:
>>>  			/*
>> 
>> 
>> Hmm, looking at all of this (especially this hack to the "old" parser),
>> I feel more like these should be promoted to a property of their own.
>> The "old" parser was designed to handle numbers, and here when you're
>> interested in the values behind the numbers (which is a first iirc), you
>> don't make any use of that. I don't really want to see a world where
>
> I had a version of this code that parsed the numbers and stored them as integers
> in `struct riscv_isainfo`. It didn't work with of_property_match_string() as
> used for riscv,isa-extensions, since that function expects the extension name to
> be the full string. Either we would need to change the code to parse a version
> number out of each string in the riscv,isa-extensions list (and make the binding
> a bunch of regexes), or we need a separate "extension" entry (and DT binding
> entry) for each supported version.

Version numbers aren't real, there's no compatibility promise that we can
consistently rely on so we treat riscv,isa-extensions as simply containing
alphanumeric extensions.  This was an intentional part of simplifying riscv,isa
into riscv,isa-extensions.

> I chose the second option, and as a consequence I didn't actually need to parse
> the integer value in the ISA string code path either.
>
>> we have every single iteration of smNpM under the sun in the property,
>> because there's a fair bit of churn in the isa. Granted, this applies to
>> all the various, the difference for me is the level of churn.
>
> Indeed. In fact, one thought I had while looking at this code is that we should
> be ignoring any extension in the ISA string with a version < 1.0 or >= 2.0,
> since those won't be compatible with what we expect.

I might go further and say that we should only accept specific exact versions of
extensions other than Ss/Sm.  This could be revisited after the recent "semver
for ISA extensions" policy is tested at least once under real-world conditions.

Right now we have two ratified versions of Ss/Sm, soon to be three, and one
ratified version of all other extensions.  I hardly think this is an excessive
amount of churn.

>> Or maybe we can still with the properties you have, but instead of
>> treating them like any other extension, handle these separately,
>> focusing on the numbering, so that only having the exact version
>> supported by a cpu is possible.
>
> Maybe I'm misunderstanding what you're saying here, but it is already the case
> that the DT for a CPU would only contain the exact version of the privileged ISA
> supported by that CPU.

If privileged spec versions are boolean extensions, then you would say "ss1p11",
"ss1p12", "ss1p13" as separate/simultaneous extensions.  This is needed in order
to allow simple support checks as described in the riscv,isa-extensions cover
letter.

> With this implementation, the fact that the integer version gets expanded to a
> series of flags is supposed to be invisible in the DT and to userspace. I
> realize I don't quite succeed there: putting "ss1p13" in the ISA string should
> work, but does not.
>
>> I'm still pretty undecided, I'd like to think about this a little bit,
>> but I think we can do better here.
>
> Sure, no problem. I'm happy to implement whatever we agree on. Though one
> consideration I had is that this is all in support of fixing a bug in v6.7, so I
> wanted the changes to be backportable.
>
> I suppose the easy way out for backporting is to check for RISCV_ISA_EXT_ZICBOZ
> for now, and then solve the larger problem once there is some other user of the
> envcfg CSR (or another Ss1p12 feature).

I support that course of action.

-s

> Regards,
> Samuel
>
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH -fixes v2 3/4] riscv: Add ISA extension parsing for Sm and Ss

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 13086 bytes --]

On Tue, Feb 13, 2024 at 03:43:27PM -0500, Stefan O'Rear wrote:
> On Tue, Feb 13, 2024, at 3:22 PM, Samuel Holland wrote:
> > On 2024-02-13 12:07 PM, Conor Dooley wrote:
> >> On Mon, Feb 12, 2024 at 07:37:34PM -0800, Samuel Holland wrote:
> >>> Previously, all extension version numbers were ignored. However, the
> >>> version number is important for these two extensions. The simplest way
> >>> to implement this is to use a separate bitmap bit for each supported
> >>> version, with each successive version implying all of the previous ones.
> >>> This allows alternatives and riscv_has_extension_[un]likely() to work
> >>> naturally.
> >>>
> >>> To avoid duplicate extensions in /proc/cpuinfo, the new successor_id
> >>> field allows hiding all but the newest implemented version of an
> >>> extension.
> >>>
> >>> Cc: <stable@vger.kernel.org> # v6.7+
> >>> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
> >>> ---
> >>>
> >>> Changes in v2:
> >>>  - New patch for v2
> >>>
> >>>  arch/riscv/include/asm/cpufeature.h |  1 +
> >>>  arch/riscv/include/asm/hwcap.h      |  8 ++++++
> >>>  arch/riscv/kernel/cpu.c             |  5 ++++
> >>>  arch/riscv/kernel/cpufeature.c      | 42 +++++++++++++++++++++++++----
> >>>  4 files changed, 51 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/arch/riscv/include/asm/cpufeature.h b/arch/riscv/include/asm/cpufeature.h
> >>> index 0bd11862b760..ac71384e7bc4 100644
> >>> --- a/arch/riscv/include/asm/cpufeature.h
> >>> +++ b/arch/riscv/include/asm/cpufeature.h
> >>> @@ -61,6 +61,7 @@ struct riscv_isa_ext_data {
> >>>  	const char *property;
> >>>  	const unsigned int *subset_ext_ids;
> >>>  	const unsigned int subset_ext_size;
> >>> +	const unsigned int successor_id;
> >>>  };
> >>>  
> >>>  extern const struct riscv_isa_ext_data riscv_isa_ext[];
> >>> diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
> >>> index 5340f818746b..5b51aa1db15b 100644
> >>> --- a/arch/riscv/include/asm/hwcap.h
> >>> +++ b/arch/riscv/include/asm/hwcap.h
> >>> @@ -80,13 +80,21 @@
> >>>  #define RISCV_ISA_EXT_ZFA		71
> >>>  #define RISCV_ISA_EXT_ZTSO		72
> >>>  #define RISCV_ISA_EXT_ZACAS		73
> >>> +#define RISCV_ISA_EXT_SM1p11		74
> >>> +#define RISCV_ISA_EXT_SM1p12		75
> >>> +#define RISCV_ISA_EXT_SS1p11		76
> >>> +#define RISCV_ISA_EXT_SS1p12		77
> >>>  
> >>>  #define RISCV_ISA_EXT_MAX		128
> >>>  #define RISCV_ISA_EXT_INVALID		U32_MAX
> >>>  
> >>>  #ifdef CONFIG_RISCV_M_MODE
> >>> +#define RISCV_ISA_EXT_Sx1p11		RISCV_ISA_EXT_SM1p11
> >>> +#define RISCV_ISA_EXT_Sx1p12		RISCV_ISA_EXT_SM1p12
> >>>  #define RISCV_ISA_EXT_SxAIA		RISCV_ISA_EXT_SMAIA
> >>>  #else
> >>> +#define RISCV_ISA_EXT_Sx1p11		RISCV_ISA_EXT_SS1p11
> >>> +#define RISCV_ISA_EXT_Sx1p12		RISCV_ISA_EXT_SS1p12
> >>>  #define RISCV_ISA_EXT_SxAIA		RISCV_ISA_EXT_SSAIA
> >>>  #endif
> >>>  
> >>> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
> >>> index d11d6320fb0d..2e6b90ed0d51 100644
> >>> --- a/arch/riscv/kernel/cpu.c
> >>> +++ b/arch/riscv/kernel/cpu.c
> >>> @@ -215,6 +215,11 @@ static void print_isa(struct seq_file *f, const unsigned long *isa_bitmap)
> >>>  		if (!__riscv_isa_extension_available(isa_bitmap, riscv_isa_ext[i].id))
> >>>  			continue;
> >>>  
> >>> +		/* Only show the newest implemented version of an extension */
> >>> +		if (riscv_isa_ext[i].successor_id != RISCV_ISA_EXT_INVALID &&
> >>> +		    __riscv_isa_extension_available(isa_bitmap, riscv_isa_ext[i].successor_id))
> >>> +			continue;
> >>> +
> >>>  		/* Only multi-letter extensions are split by underscores */
> >>>  		if (strnlen(riscv_isa_ext[i].name, 2) != 1)
> >>>  			seq_puts(f, "_");
> >>> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> >>> index c5b13f7dd482..8e10b50120e9 100644
> >>> --- a/arch/riscv/kernel/cpufeature.c
> >>> +++ b/arch/riscv/kernel/cpufeature.c
> >>> @@ -113,23 +113,29 @@ static bool riscv_isa_extension_check(int id)
> >>>  	return true;
> >>>  }
> >>>  
> >>> -#define _RISCV_ISA_EXT_DATA(_name, _id, _subset_exts, _subset_exts_size) {	\
> >>> +#define _RISCV_ISA_EXT_DATA(_name, _id, _subset_exts, _subset_exts_size, _successor) {	\
> >>>  	.name = #_name,								\
> >>>  	.property = #_name,							\
> >>>  	.id = _id,								\
> >>>  	.subset_ext_ids = _subset_exts,						\
> >>> -	.subset_ext_size = _subset_exts_size					\
> >>> +	.subset_ext_size = _subset_exts_size,					\
> >>> +	.successor_id = _successor,						\
> >>>  }
> >>>  
> >>> -#define __RISCV_ISA_EXT_DATA(_name, _id) _RISCV_ISA_EXT_DATA(_name, _id, NULL, 0)
> >>> +#define __RISCV_ISA_EXT_DATA(_name, _id) \
> >>> +	_RISCV_ISA_EXT_DATA(_name, _id, NULL, 0, RISCV_ISA_EXT_INVALID)
> >>>  
> >>>  /* Used to declare pure "lasso" extension (Zk for instance) */
> >>>  #define __RISCV_ISA_EXT_BUNDLE(_name, _bundled_exts) \
> >>> -	_RISCV_ISA_EXT_DATA(_name, RISCV_ISA_EXT_INVALID, _bundled_exts, ARRAY_SIZE(_bundled_exts))
> >>> +	_RISCV_ISA_EXT_DATA(_name, RISCV_ISA_EXT_INVALID, \
> >>> +			    _bundled_exts, ARRAY_SIZE(_bundled_exts), RISCV_ISA_EXT_INVALID)
> >>>  
> >>>  /* Used to declare extensions that are a superset of other extensions (Zvbb for instance) */
> >>>  #define __RISCV_ISA_EXT_SUPERSET(_name, _id, _sub_exts) \
> >>> -	_RISCV_ISA_EXT_DATA(_name, _id, _sub_exts, ARRAY_SIZE(_sub_exts))
> >>> +	_RISCV_ISA_EXT_DATA(_name, _id, _sub_exts, ARRAY_SIZE(_sub_exts), RISCV_ISA_EXT_INVALID)
> >>> +
> >>> +#define __RISCV_ISA_EXT_VERSION(_name, _id, _preds, _preds_size, _successor) \
> >>> +	_RISCV_ISA_EXT_DATA(_name, _id, _preds, _preds_size, _successor)
> >>>  
> >>>  static const unsigned int riscv_zk_bundled_exts[] = {
> >>>  	RISCV_ISA_EXT_ZBKB,
> >>> @@ -201,6 +207,16 @@ static const unsigned int riscv_zvbb_exts[] = {
> >>>  	RISCV_ISA_EXT_ZVKB
> >>>  };
> >>>  
> >>> +static const unsigned int riscv_sm_ext_versions[] = {
> >>> +	RISCV_ISA_EXT_SM1p11,
> >>> +	RISCV_ISA_EXT_SM1p12,
> >>> +};
> >>> +
> >>> +static const unsigned int riscv_ss_ext_versions[] = {
> >>> +	RISCV_ISA_EXT_SS1p11,
> >>> +	RISCV_ISA_EXT_SS1p12,
> >>> +};
> >>> +
> >>>  /*
> >>>   * The canonical order of ISA extension names in the ISA string is defined in
> >>>   * chapter 27 of the unprivileged specification.
> >>> @@ -299,8 +315,16 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = {
> >>>  	__RISCV_ISA_EXT_DATA(zvksh, RISCV_ISA_EXT_ZVKSH),
> >>>  	__RISCV_ISA_EXT_BUNDLE(zvksg, riscv_zvksg_bundled_exts),
> >>>  	__RISCV_ISA_EXT_DATA(zvkt, RISCV_ISA_EXT_ZVKT),
> >>> +	__RISCV_ISA_EXT_VERSION(sm1p11, RISCV_ISA_EXT_SM1p11, riscv_sm_ext_versions, 0,
> >>> +				RISCV_ISA_EXT_SM1p12),
> >>> +	__RISCV_ISA_EXT_VERSION(sm1p12, RISCV_ISA_EXT_SM1p12, riscv_sm_ext_versions, 1,
> >>> +				RISCV_ISA_EXT_INVALID),
> >>>  	__RISCV_ISA_EXT_DATA(smaia, RISCV_ISA_EXT_SMAIA),
> >>>  	__RISCV_ISA_EXT_DATA(smstateen, RISCV_ISA_EXT_SMSTATEEN),
> >>> +	__RISCV_ISA_EXT_VERSION(ss1p11, RISCV_ISA_EXT_SS1p11, riscv_ss_ext_versions, 0,
> >>> +				RISCV_ISA_EXT_SS1p12),
> >>> +	__RISCV_ISA_EXT_VERSION(ss1p12, RISCV_ISA_EXT_SS1p12, riscv_ss_ext_versions, 1,
> >>> +				RISCV_ISA_EXT_INVALID),
> >>>  	__RISCV_ISA_EXT_DATA(ssaia, RISCV_ISA_EXT_SSAIA),
> >>>  	__RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
> >>>  	__RISCV_ISA_EXT_DATA(sstc, RISCV_ISA_EXT_SSTC),
> >>> @@ -414,6 +438,14 @@ static void __init riscv_parse_isa_string(unsigned long *this_hwcap, struct risc
> >>>  				;
> >>>  
> >>>  			++ext_end;
> >>> +
> >>> +			/*
> >>> +			 * As a special case for the Sm and Ss extensions, where the version
> >>> +			 * number is important, include it in the extension name.
> >>> +			 */
> >>> +			if (ext_end - ext == 2 && tolower(ext[0]) == 's' &&
> >>> +			    (tolower(ext[1]) == 'm' || tolower(ext[1]) == 's'))
> >>> +				ext_end = isa;
> >>>  			break;
> >>>  		default:
> >>>  			/*
> >> 
> >> 
> >> Hmm, looking at all of this (especially this hack to the "old" parser),
> >> I feel more like these should be promoted to a property of their own.
> >> The "old" parser was designed to handle numbers, and here when you're
> >> interested in the values behind the numbers (which is a first iirc), you
> >> don't make any use of that. I don't really want to see a world where
> >
> > I had a version of this code that parsed the numbers and stored them as integers
> > in `struct riscv_isainfo`. It didn't work with of_property_match_string() as
> > used for riscv,isa-extensions, since that function expects the extension name to
> > be the full string.

I don't think I actually want what I am about to say, but it's not as if we
are forced to parse it in that way for all properties. It's handy AF to be
able to reuse reuse that function, and that was part of my goal originally
with the property, but we are not locked into using
of_property_match_string() if there's some specific property where that's
getting in our way. That's kinda an aside though..

> > Either we would need to change the code to parse a version
> > number out of each string in the riscv,isa-extensions list (and make the binding
> > a bunch of regexes), or we need a separate "extension" entry (and DT binding
> > entry) for each supported version.
> 
> Version numbers aren't real, there's no compatibility promise that we can
> consistently rely on so we treat riscv,isa-extensions as simply containing
> alphanumeric extensions.  This was an intentional part of simplifying riscv,isa
> into riscv,isa-extensions.

You seem to recall my motivations better than I do!

> > I chose the second option, and as a consequence I didn't actually need to parse
> > the integer value in the ISA string code path either.
> >
> >> we have every single iteration of smNpM under the sun in the property,
> >> because there's a fair bit of churn in the isa. Granted, this applies to
> >> all the various, the difference for me is the level of churn.
> >
> > Indeed. In fact, one thought I had while looking at this code is that we should
> > be ignoring any extension in the ISA string with a version < 1.0 or >= 2.0,
> > since those won't be compatible with what we expect.
> 
> I might go further and say that we should only accept specific exact versions of
> extensions other than Ss/Sm.

This is what we do, right? Every property is supposed to match to
exactly the frozen or ratified spec, so they do have exactly one version
at present. The property descriptions should contain that information.

> This could be revisited after the recent "semver
> for ISA extensions" policy is tested at least once under real-world conditions.
> 
> Right now we have two ratified versions of Ss/Sm, soon to be three, and one
> ratified version of all other extensions.  I hardly think this is an excessive
> amount of churn.

Yeah, maybe it's fine. I'm just overthinking it and there isn't a
problem.

> >> Or maybe we can still with the properties you have, but instead of
> >> treating them like any other extension, handle these separately,
> >> focusing on the numbering, so that only having the exact version
> >> supported by a cpu is possible.
> >
> > Maybe I'm misunderstanding what you're saying here, but it is already the case
> > that the DT for a CPU would only contain the exact version of the privileged ISA
> > supported by that CPU.
> 
> If privileged spec versions are boolean extensions, then you would say "ss1p11",
> "ss1p12", "ss1p13" as separate/simultaneous extensions.

> This is needed in order
> to allow simple support checks as described in the riscv,isa-extensions cover
> letter.

Yes, it is explicitly a goal of riscv,isa-extensions that you can look
for a specific extension in the list if that is all you care about. If
you go and drop ss1p11 because you support ss1p12 it defeats that. I
don't know off the top of my head how to enforce ss1p12 requiring ss1p11
in json schema, but I do have an idea of where to start...

> > With this implementation, the fact that the integer version gets expanded to a
> > series of flags is supposed to be invisible in the DT and to userspace. I
> > realize I don't quite succeed there: putting "ss1p13" in the ISA string should
> > work, but does not.
> >
> >> I'm still pretty undecided, I'd like to think about this a little bit,
> >> but I think we can do better here.
> >
> > Sure, no problem. I'm happy to implement whatever we agree on. Though one
> > consideration I had is that this is all in support of fixing a bug in v6.7, so I
> > wanted the changes to be backportable.
> >
> > I suppose the easy way out for backporting is to check for RISCV_ISA_EXT_ZICBOZ
> > for now, and then solve the larger problem once there is some other user of the
> > envcfg CSR (or another Ss1p12 feature).
> 
> I support that course of action.

I saw another mail suggesting that Zicbom implied Ss1p12, I think that
should be reasonable position to take for now.

Cheers,
Conor.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH -fixes v2 3/4] riscv: Add ISA extension parsing for Sm and Ss

[Samuel Holland]

Hi Conor, Stefan,

Thanks for the discussion!

On 2024-02-13 5:15 PM, Conor Dooley wrote:
> On Tue, Feb 13, 2024 at 03:43:27PM -0500, Stefan O'Rear wrote:
>> On Tue, Feb 13, 2024, at 3:22 PM, Samuel Holland wrote:
>>> On 2024-02-13 12:07 PM, Conor Dooley wrote:
>>>> On Mon, Feb 12, 2024 at 07:37:34PM -0800, Samuel Holland wrote:
>>>>> Previously, all extension version numbers were ignored. However, the
>>>>> version number is important for these two extensions. The simplest way
>>>>> to implement this is to use a separate bitmap bit for each supported
>>>>> version, with each successive version implying all of the previous ones.
>>>>> This allows alternatives and riscv_has_extension_[un]likely() to work
>>>>> naturally.
>>>>>
>>>>> To avoid duplicate extensions in /proc/cpuinfo, the new successor_id
>>>>> field allows hiding all but the newest implemented version of an
>>>>> extension.
>>>>>
>>>>> Cc: <stable@vger.kernel.org> # v6.7+
>>>>> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
>>>>> ---
>>>>>
>>>>> Changes in v2:
>>>>>  - New patch for v2
>>>>>
>>>>>  arch/riscv/include/asm/cpufeature.h |  1 +
>>>>>  arch/riscv/include/asm/hwcap.h      |  8 ++++++
>>>>>  arch/riscv/kernel/cpu.c             |  5 ++++
>>>>>  arch/riscv/kernel/cpufeature.c      | 42 +++++++++++++++++++++++++----
>>>>>  4 files changed, 51 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/arch/riscv/include/asm/cpufeature.h b/arch/riscv/include/asm/cpufeature.h
>>>>> index 0bd11862b760..ac71384e7bc4 100644
>>>>> --- a/arch/riscv/include/asm/cpufeature.h
>>>>> +++ b/arch/riscv/include/asm/cpufeature.h
>>>>> @@ -61,6 +61,7 @@ struct riscv_isa_ext_data {
>>>>>  	const char *property;
>>>>>  	const unsigned int *subset_ext_ids;
>>>>>  	const unsigned int subset_ext_size;
>>>>> +	const unsigned int successor_id;
>>>>>  };
>>>>>  
>>>>>  extern const struct riscv_isa_ext_data riscv_isa_ext[];
>>>>> diff --git a/arch/riscv/include/asm/hwcap.h b/arch/riscv/include/asm/hwcap.h
>>>>> index 5340f818746b..5b51aa1db15b 100644
>>>>> --- a/arch/riscv/include/asm/hwcap.h
>>>>> +++ b/arch/riscv/include/asm/hwcap.h
>>>>> @@ -80,13 +80,21 @@
>>>>>  #define RISCV_ISA_EXT_ZFA		71
>>>>>  #define RISCV_ISA_EXT_ZTSO		72
>>>>>  #define RISCV_ISA_EXT_ZACAS		73
>>>>> +#define RISCV_ISA_EXT_SM1p11		74
>>>>> +#define RISCV_ISA_EXT_SM1p12		75
>>>>> +#define RISCV_ISA_EXT_SS1p11		76
>>>>> +#define RISCV_ISA_EXT_SS1p12		77
>>>>>  
>>>>>  #define RISCV_ISA_EXT_MAX		128
>>>>>  #define RISCV_ISA_EXT_INVALID		U32_MAX
>>>>>  
>>>>>  #ifdef CONFIG_RISCV_M_MODE
>>>>> +#define RISCV_ISA_EXT_Sx1p11		RISCV_ISA_EXT_SM1p11
>>>>> +#define RISCV_ISA_EXT_Sx1p12		RISCV_ISA_EXT_SM1p12
>>>>>  #define RISCV_ISA_EXT_SxAIA		RISCV_ISA_EXT_SMAIA
>>>>>  #else
>>>>> +#define RISCV_ISA_EXT_Sx1p11		RISCV_ISA_EXT_SS1p11
>>>>> +#define RISCV_ISA_EXT_Sx1p12		RISCV_ISA_EXT_SS1p12
>>>>>  #define RISCV_ISA_EXT_SxAIA		RISCV_ISA_EXT_SSAIA
>>>>>  #endif
>>>>>  
>>>>> diff --git a/arch/riscv/kernel/cpu.c b/arch/riscv/kernel/cpu.c
>>>>> index d11d6320fb0d..2e6b90ed0d51 100644
>>>>> --- a/arch/riscv/kernel/cpu.c
>>>>> +++ b/arch/riscv/kernel/cpu.c
>>>>> @@ -215,6 +215,11 @@ static void print_isa(struct seq_file *f, const unsigned long *isa_bitmap)
>>>>>  		if (!__riscv_isa_extension_available(isa_bitmap, riscv_isa_ext[i].id))
>>>>>  			continue;
>>>>>  
>>>>> +		/* Only show the newest implemented version of an extension */
>>>>> +		if (riscv_isa_ext[i].successor_id != RISCV_ISA_EXT_INVALID &&
>>>>> +		    __riscv_isa_extension_available(isa_bitmap, riscv_isa_ext[i].successor_id))
>>>>> +			continue;
>>>>> +
>>>>>  		/* Only multi-letter extensions are split by underscores */
>>>>>  		if (strnlen(riscv_isa_ext[i].name, 2) != 1)
>>>>>  			seq_puts(f, "_");
>>>>> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
>>>>> index c5b13f7dd482..8e10b50120e9 100644
>>>>> --- a/arch/riscv/kernel/cpufeature.c
>>>>> +++ b/arch/riscv/kernel/cpufeature.c
>>>>> @@ -113,23 +113,29 @@ static bool riscv_isa_extension_check(int id)
>>>>>  	return true;
>>>>>  }
>>>>>  
>>>>> -#define _RISCV_ISA_EXT_DATA(_name, _id, _subset_exts, _subset_exts_size) {	\
>>>>> +#define _RISCV_ISA_EXT_DATA(_name, _id, _subset_exts, _subset_exts_size, _successor) {	\
>>>>>  	.name = #_name,								\
>>>>>  	.property = #_name,							\
>>>>>  	.id = _id,								\
>>>>>  	.subset_ext_ids = _subset_exts,						\
>>>>> -	.subset_ext_size = _subset_exts_size					\
>>>>> +	.subset_ext_size = _subset_exts_size,					\
>>>>> +	.successor_id = _successor,						\
>>>>>  }
>>>>>  
>>>>> -#define __RISCV_ISA_EXT_DATA(_name, _id) _RISCV_ISA_EXT_DATA(_name, _id, NULL, 0)
>>>>> +#define __RISCV_ISA_EXT_DATA(_name, _id) \
>>>>> +	_RISCV_ISA_EXT_DATA(_name, _id, NULL, 0, RISCV_ISA_EXT_INVALID)
>>>>>  
>>>>>  /* Used to declare pure "lasso" extension (Zk for instance) */
>>>>>  #define __RISCV_ISA_EXT_BUNDLE(_name, _bundled_exts) \
>>>>> -	_RISCV_ISA_EXT_DATA(_name, RISCV_ISA_EXT_INVALID, _bundled_exts, ARRAY_SIZE(_bundled_exts))
>>>>> +	_RISCV_ISA_EXT_DATA(_name, RISCV_ISA_EXT_INVALID, \
>>>>> +			    _bundled_exts, ARRAY_SIZE(_bundled_exts), RISCV_ISA_EXT_INVALID)
>>>>>  
>>>>>  /* Used to declare extensions that are a superset of other extensions (Zvbb for instance) */
>>>>>  #define __RISCV_ISA_EXT_SUPERSET(_name, _id, _sub_exts) \
>>>>> -	_RISCV_ISA_EXT_DATA(_name, _id, _sub_exts, ARRAY_SIZE(_sub_exts))
>>>>> +	_RISCV_ISA_EXT_DATA(_name, _id, _sub_exts, ARRAY_SIZE(_sub_exts), RISCV_ISA_EXT_INVALID)
>>>>> +
>>>>> +#define __RISCV_ISA_EXT_VERSION(_name, _id, _preds, _preds_size, _successor) \
>>>>> +	_RISCV_ISA_EXT_DATA(_name, _id, _preds, _preds_size, _successor)
>>>>>  
>>>>>  static const unsigned int riscv_zk_bundled_exts[] = {
>>>>>  	RISCV_ISA_EXT_ZBKB,
>>>>> @@ -201,6 +207,16 @@ static const unsigned int riscv_zvbb_exts[] = {
>>>>>  	RISCV_ISA_EXT_ZVKB
>>>>>  };
>>>>>  
>>>>> +static const unsigned int riscv_sm_ext_versions[] = {
>>>>> +	RISCV_ISA_EXT_SM1p11,
>>>>> +	RISCV_ISA_EXT_SM1p12,
>>>>> +};
>>>>> +
>>>>> +static const unsigned int riscv_ss_ext_versions[] = {
>>>>> +	RISCV_ISA_EXT_SS1p11,
>>>>> +	RISCV_ISA_EXT_SS1p12,
>>>>> +};
>>>>> +
>>>>>  /*
>>>>>   * The canonical order of ISA extension names in the ISA string is defined in
>>>>>   * chapter 27 of the unprivileged specification.
>>>>> @@ -299,8 +315,16 @@ const struct riscv_isa_ext_data riscv_isa_ext[] = {
>>>>>  	__RISCV_ISA_EXT_DATA(zvksh, RISCV_ISA_EXT_ZVKSH),
>>>>>  	__RISCV_ISA_EXT_BUNDLE(zvksg, riscv_zvksg_bundled_exts),
>>>>>  	__RISCV_ISA_EXT_DATA(zvkt, RISCV_ISA_EXT_ZVKT),
>>>>> +	__RISCV_ISA_EXT_VERSION(sm1p11, RISCV_ISA_EXT_SM1p11, riscv_sm_ext_versions, 0,
>>>>> +				RISCV_ISA_EXT_SM1p12),
>>>>> +	__RISCV_ISA_EXT_VERSION(sm1p12, RISCV_ISA_EXT_SM1p12, riscv_sm_ext_versions, 1,
>>>>> +				RISCV_ISA_EXT_INVALID),
>>>>>  	__RISCV_ISA_EXT_DATA(smaia, RISCV_ISA_EXT_SMAIA),
>>>>>  	__RISCV_ISA_EXT_DATA(smstateen, RISCV_ISA_EXT_SMSTATEEN),
>>>>> +	__RISCV_ISA_EXT_VERSION(ss1p11, RISCV_ISA_EXT_SS1p11, riscv_ss_ext_versions, 0,
>>>>> +				RISCV_ISA_EXT_SS1p12),
>>>>> +	__RISCV_ISA_EXT_VERSION(ss1p12, RISCV_ISA_EXT_SS1p12, riscv_ss_ext_versions, 1,
>>>>> +				RISCV_ISA_EXT_INVALID),
>>>>>  	__RISCV_ISA_EXT_DATA(ssaia, RISCV_ISA_EXT_SSAIA),
>>>>>  	__RISCV_ISA_EXT_DATA(sscofpmf, RISCV_ISA_EXT_SSCOFPMF),
>>>>>  	__RISCV_ISA_EXT_DATA(sstc, RISCV_ISA_EXT_SSTC),
>>>>> @@ -414,6 +438,14 @@ static void __init riscv_parse_isa_string(unsigned long *this_hwcap, struct risc
>>>>>  				;
>>>>>  
>>>>>  			++ext_end;
>>>>> +
>>>>> +			/*
>>>>> +			 * As a special case for the Sm and Ss extensions, where the version
>>>>> +			 * number is important, include it in the extension name.
>>>>> +			 */
>>>>> +			if (ext_end - ext == 2 && tolower(ext[0]) == 's' &&
>>>>> +			    (tolower(ext[1]) == 'm' || tolower(ext[1]) == 's'))
>>>>> +				ext_end = isa;
>>>>>  			break;
>>>>>  		default:
>>>>>  			/*
>>>>
>>>>
>>>> Hmm, looking at all of this (especially this hack to the "old" parser),
>>>> I feel more like these should be promoted to a property of their own.
>>>> The "old" parser was designed to handle numbers, and here when you're
>>>> interested in the values behind the numbers (which is a first iirc), you
>>>> don't make any use of that. I don't really want to see a world where
>>>
>>> I had a version of this code that parsed the numbers and stored them as integers
>>> in `struct riscv_isainfo`. It didn't work with of_property_match_string() as
>>> used for riscv,isa-extensions, since that function expects the extension name to
>>> be the full string.
> 
> I don't think I actually want what I am about to say, but it's not as if we
> are forced to parse it in that way for all properties. It's handy AF to be
> able to reuse reuse that function, and that was part of my goal originally
> with the property, but we are not locked into using
> of_property_match_string() if there's some specific property where that's
> getting in our way. That's kinda an aside though..
> 
>>> Either we would need to change the code to parse a version
>>> number out of each string in the riscv,isa-extensions list (and make the binding
>>> a bunch of regexes), or we need a separate "extension" entry (and DT binding
>>> entry) for each supported version.
>>
>> Version numbers aren't real, there's no compatibility promise that we can
>> consistently rely on so we treat riscv,isa-extensions as simply containing
>> alphanumeric extensions.  This was an intentional part of simplifying riscv,isa
>> into riscv,isa-extensions.
> 
> You seem to recall my motivations better than I do!
> 
>>> I chose the second option, and as a consequence I didn't actually need to parse
>>> the integer value in the ISA string code path either.
>>>
>>>> we have every single iteration of smNpM under the sun in the property,
>>>> because there's a fair bit of churn in the isa. Granted, this applies to
>>>> all the various, the difference for me is the level of churn.
>>>
>>> Indeed. In fact, one thought I had while looking at this code is that we should
>>> be ignoring any extension in the ISA string with a version < 1.0 or >= 2.0,
>>> since those won't be compatible with what we expect.
>>
>> I might go further and say that we should only accept specific exact versions of
>> extensions other than Ss/Sm.
> 
> This is what we do, right? Every property is supposed to match to
> exactly the frozen or ratified spec, so they do have exactly one version
> at present. The property descriptions should contain that information.
> 
>> This could be revisited after the recent "semver
>> for ISA extensions" policy is tested at least once under real-world conditions.
>>
>> Right now we have two ratified versions of Ss/Sm, soon to be three, and one

My understanding (from hwprobe.rst and various ML comments) is that Linux
assumes Ss1p10. That's why I started counting with Ss1p11. It looks like Ss1p10
was never ratified, but that doesn't prevent us from referencing it in the
binding, if needed. Should we start enumerating extensions at Ss1p11 or
something else?

>> ratified version of all other extensions.  I hardly think this is an excessive
>> amount of churn.
> 
> Yeah, maybe it's fine. I'm just overthinking it and there isn't a
> problem.

My interpretation of this and the above comments is that we do actually want to
enumerate every supported "version" of the privileged ISA in the binding, and
parse them as entirely independent extensions that just happen to have
similar-looking names. Is that correct?

>>>> Or maybe we can still with the properties you have, but instead of
>>>> treating them like any other extension, handle these separately,
>>>> focusing on the numbering, so that only having the exact version
>>>> supported by a cpu is possible.
>>>
>>> Maybe I'm misunderstanding what you're saying here, but it is already the case
>>> that the DT for a CPU would only contain the exact version of the privileged ISA
>>> supported by that CPU.
>>
>> If privileged spec versions are boolean extensions, then you would say "ss1p11",
>> "ss1p12", "ss1p13" as separate/simultaneous extensions.
> 
>> This is needed in order
>> to allow simple support checks as described in the riscv,isa-extensions cover
>> letter.
> 
> Yes, it is explicitly a goal of riscv,isa-extensions that you can look
> for a specific extension in the list if that is all you care about. If
> you go and drop ss1p11 because you support ss1p12 it defeats that.

Okay, that makes sense, but that is not how the parsing code works right now,
which is probably what led me down the wrong path. :)

To have the intended semantics, we cannot parse _anything_ in
riscv,isa-extensions as a "bundled" or "superset" extension. Because to
accomplish your goal, each extension in the bundle must be listed explicitly, in
case the consumer only cares about that one extension. So it sounds like
riscv_fill_hwcap_from_ext_list() needs to ignore subset_ext_size/subset_ext_ids.

> I don't know off the top of my head how to enforce ss1p12 requiring ss1p11
> in json schema, but I do have an idea of where to start...

Yeah, this is different from normal "dependencies:" because it is a string list.

I think we need to add dependencies in the binding for the bundled extensions as
well, and maybe even between extensions like "d" and "f".

>>> With this implementation, the fact that the integer version gets expanded to a
>>> series of flags is supposed to be invisible in the DT and to userspace. I
>>> realize I don't quite succeed there: putting "ss1p13" in the ISA string should
>>> work, but does not.
>>>
>>>> I'm still pretty undecided, I'd like to think about this a little bit,
>>>> but I think we can do better here.
>>>
>>> Sure, no problem. I'm happy to implement whatever we agree on. Though one
>>> consideration I had is that this is all in support of fixing a bug in v6.7, so I
>>> wanted the changes to be backportable.
>>>
>>> I suppose the easy way out for backporting is to check for RISCV_ISA_EXT_ZICBOZ
>>> for now, and then solve the larger problem once there is some other user of the
>>> envcfg CSR (or another Ss1p12 feature).
>>
>> I support that course of action.
> 
> I saw another mail suggesting that Zicbom implied Ss1p12, I think that
> should be reasonable position to take for now.

Like I mentioned in my other email, I don't think Zicboz is sufficient to imply
Ss1p12. So I'd prefer to either stay with checking Zicboz (like v3 does); or add
a bit for specifically the existence of the the envcfg CSR, that doesn't map to
anything in the ISA string (i.e. is not listed in riscv_isa_ext). Of course, if
we start ignoring subset_ext_ids, I'll have to reconsider how to actually
implement that.

Regards,
Samuel


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH -fixes v2 3/4] riscv: Add ISA extension parsing for Sm and Ss

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 3300 bytes --]

On Sun, Feb 18, 2024 at 09:00:14AM -0600, Samuel Holland wrote:
> >>>> Or maybe we can still with the properties you have, but instead of
> >>>> treating them like any other extension, handle these separately,
> >>>> focusing on the numbering, so that only having the exact version
> >>>> supported by a cpu is possible.
> >>>
> >>> Maybe I'm misunderstanding what you're saying here, but it is already the case
> >>> that the DT for a CPU would only contain the exact version of the privileged ISA
> >>> supported by that CPU.
> >>
> >> If privileged spec versions are boolean extensions, then you would say "ss1p11",
> >> "ss1p12", "ss1p13" as separate/simultaneous extensions.
> > 
> >> This is needed in order
> >> to allow simple support checks as described in the riscv,isa-extensions cover
> >> letter.
> > 
> > Yes, it is explicitly a goal of riscv,isa-extensions that you can look
> > for a specific extension in the list if that is all you care about. If
> > you go and drop ss1p11 because you support ss1p12 it defeats that.
> 
> Okay, that makes sense, but that is not how the parsing code works right now,
> which is probably what led me down the wrong path. :)
> 
> To have the intended semantics, we cannot parse _anything_ in
> riscv,isa-extensions as a "bundled" or "superset" extension.

That's not true I don't think. You can parse as a "bundle" but...

> Because to
> accomplish your goal, each extension in the bundle must be listed explicitly, in
> case the consumer only cares about that one extension.

...as you note here, the extensions also have to be listed explicitly so
that they can be detected in isolation if that is all that a consumer
does.

> So it sounds like
> riscv_fill_hwcap_from_ext_list() needs to ignore subset_ext_size/subset_ext_ids.

Do you mean this:
if (ext->subset_ext_size) {
	for (int j = 0; j < ext->subset_ext_size; j++) {
		if (riscv_isa_extension_check(ext->subset_ext_ids[i]))
			set_bit(ext->subset_ext_ids[j], isainfo->isa);
	}
}

I think that is fine? If you find the "superset" you can enable the
individual elements. The problem would just be if someone put only the
superset in a DT (or ACPI tables) and the software looked for the
individual element only, but IIRC the kernel currently checks both the
superset and individual elements.
It would be possible to check a bundle and then skip looking for the
individual elements if the bundle was already found if the parsing is
wont to be sped up.

I think all we need to do is enforce that all individual elements are
present on a schema validation level (I have no clue what we can do
for ACPI) and no change is required in the kernel.

Am I misunderstanding what you think is the problem here?

> > I don't know off the top of my head how to enforce ss1p12 requiring ss1p11
> > in json schema, but I do have an idea of where to start...
> 
> Yeah, this is different from normal "dependencies:" because it is a string list.

I think it is actually doable, just will look sorta clunky. I meant to
go and do it this weekend, but I've been rather sick unfortunately.
Something similar is definitely doable for compatibles, so either it'll
"just work" or I may have to extend the validation tooling.

Cheers,
Conor.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 161 bytes --]

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

[PATCH -fixes v2 4/4] riscv: Save/restore envcfg CSR during CPU suspend

[Samuel Holland]

The value of the [ms]envcfg CSR is lost when entering a nonretentive
idle state, so the CSR must be rewritten when resuming the CPU.

Cc: <stable@vger.kernel.org> # v6.7+
Fixes: 43c16d51a19b ("RISC-V: Enable cbo.zero in usermode")
Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
---

Changes in v2:
 - Check for privileged ISA v1.12 instead of the specific CSR
 - Use riscv_has_extension_likely() instead of new ALTERNATIVE()s

 arch/riscv/include/asm/suspend.h | 1 +
 arch/riscv/kernel/suspend.c      | 4 ++++
 2 files changed, 5 insertions(+)

diff --git a/arch/riscv/include/asm/suspend.h b/arch/riscv/include/asm/suspend.h
index 02f87867389a..491296a335d0 100644
--- a/arch/riscv/include/asm/suspend.h
+++ b/arch/riscv/include/asm/suspend.h
@@ -14,6 +14,7 @@ struct suspend_context {
 	struct pt_regs regs;
 	/* Saved and restored by high-level functions */
 	unsigned long scratch;
+	unsigned long envcfg;
 	unsigned long tvec;
 	unsigned long ie;
 #ifdef CONFIG_MMU
diff --git a/arch/riscv/kernel/suspend.c b/arch/riscv/kernel/suspend.c
index 239509367e42..be03615486ed 100644
--- a/arch/riscv/kernel/suspend.c
+++ b/arch/riscv/kernel/suspend.c
@@ -15,6 +15,8 @@
 void suspend_save_csrs(struct suspend_context *context)
 {
 	context->scratch = csr_read(CSR_SCRATCH);
+	if (riscv_has_extension_likely(RISCV_ISA_EXT_Sx1p12))
+		context->envcfg = csr_read(CSR_ENVCFG);
 	context->tvec = csr_read(CSR_TVEC);
 	context->ie = csr_read(CSR_IE);
 
@@ -36,6 +38,8 @@ void suspend_save_csrs(struct suspend_context *context)
 void suspend_restore_csrs(struct suspend_context *context)
 {
 	csr_write(CSR_SCRATCH, context->scratch);
+	if (riscv_has_extension_likely(RISCV_ISA_EXT_Sx1p12))
+		csr_write(CSR_ENVCFG, context->envcfg);
 	csr_write(CSR_TVEC, context->tvec);
 	csr_write(CSR_IE, context->ie);
 
-- 
2.43.0


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH -fixes v2 4/4] riscv: Save/restore envcfg CSR during CPU suspend

[Andrew Jones]

On Mon, Feb 12, 2024 at 07:37:35PM -0800, Samuel Holland wrote:
> The value of the [ms]envcfg CSR is lost when entering a nonretentive
> idle state, so the CSR must be rewritten when resuming the CPU.
> 
> Cc: <stable@vger.kernel.org> # v6.7+
> Fixes: 43c16d51a19b ("RISC-V: Enable cbo.zero in usermode")
> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
> ---
> 
> Changes in v2:
>  - Check for privileged ISA v1.12 instead of the specific CSR
>  - Use riscv_has_extension_likely() instead of new ALTERNATIVE()s
> 
>  arch/riscv/include/asm/suspend.h | 1 +
>  arch/riscv/kernel/suspend.c      | 4 ++++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/arch/riscv/include/asm/suspend.h b/arch/riscv/include/asm/suspend.h
> index 02f87867389a..491296a335d0 100644
> --- a/arch/riscv/include/asm/suspend.h
> +++ b/arch/riscv/include/asm/suspend.h
> @@ -14,6 +14,7 @@ struct suspend_context {
>  	struct pt_regs regs;
>  	/* Saved and restored by high-level functions */
>  	unsigned long scratch;
> +	unsigned long envcfg;
>  	unsigned long tvec;
>  	unsigned long ie;
>  #ifdef CONFIG_MMU
> diff --git a/arch/riscv/kernel/suspend.c b/arch/riscv/kernel/suspend.c
> index 239509367e42..be03615486ed 100644
> --- a/arch/riscv/kernel/suspend.c
> +++ b/arch/riscv/kernel/suspend.c
> @@ -15,6 +15,8 @@
>  void suspend_save_csrs(struct suspend_context *context)
>  {
>  	context->scratch = csr_read(CSR_SCRATCH);
> +	if (riscv_has_extension_likely(RISCV_ISA_EXT_Sx1p12))
> +		context->envcfg = csr_read(CSR_ENVCFG);
>  	context->tvec = csr_read(CSR_TVEC);
>  	context->ie = csr_read(CSR_IE);
>  
> @@ -36,6 +38,8 @@ void suspend_save_csrs(struct suspend_context *context)
>  void suspend_restore_csrs(struct suspend_context *context)
>  {
>  	csr_write(CSR_SCRATCH, context->scratch);
> +	if (riscv_has_extension_likely(RISCV_ISA_EXT_Sx1p12))
> +		csr_write(CSR_ENVCFG, context->envcfg);
>  	csr_write(CSR_TVEC, context->tvec);
>  	csr_write(CSR_IE, context->ie);
>  
> -- 
> 2.43.0
>

We're still exposing Zicboz to userspace in hwprobe when only
RISCV_ISA_EXT_ZICBOZ is present, which will be the case for anything that
either doesn't implement 1.12, but does implement Zicboz, or maybe does
implement 1.12, but hasn't started putting Ss1p12 in its ISA string yet
(e.g. QEMU). We should either stop exposing Zicboz to userspace in those
cases (since it won't work) or rethink how we want to determine whether
or not we have envcfg CSRs.

Thanks,
drew

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH -fixes v2 4/4] riscv: Save/restore envcfg CSR during CPU suspend

[Stefan O'Rear]

On Tue, Feb 13, 2024, at 9:49 AM, Andrew Jones wrote:
> On Mon, Feb 12, 2024 at 07:37:35PM -0800, Samuel Holland wrote:
>> The value of the [ms]envcfg CSR is lost when entering a nonretentive
>> idle state, so the CSR must be rewritten when resuming the CPU.
>> 
>> Cc: <stable@vger.kernel.org> # v6.7+
>> Fixes: 43c16d51a19b ("RISC-V: Enable cbo.zero in usermode")
>> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
>> ---
>> 
>> Changes in v2:
>>  - Check for privileged ISA v1.12 instead of the specific CSR
>>  - Use riscv_has_extension_likely() instead of new ALTERNATIVE()s
>> 
>>  arch/riscv/include/asm/suspend.h | 1 +
>>  arch/riscv/kernel/suspend.c      | 4 ++++
>>  2 files changed, 5 insertions(+)
>> 
>> diff --git a/arch/riscv/include/asm/suspend.h b/arch/riscv/include/asm/suspend.h
>> index 02f87867389a..491296a335d0 100644
>> --- a/arch/riscv/include/asm/suspend.h
>> +++ b/arch/riscv/include/asm/suspend.h
>> @@ -14,6 +14,7 @@ struct suspend_context {
>>  	struct pt_regs regs;
>>  	/* Saved and restored by high-level functions */
>>  	unsigned long scratch;
>> +	unsigned long envcfg;
>>  	unsigned long tvec;
>>  	unsigned long ie;
>>  #ifdef CONFIG_MMU
>> diff --git a/arch/riscv/kernel/suspend.c b/arch/riscv/kernel/suspend.c
>> index 239509367e42..be03615486ed 100644
>> --- a/arch/riscv/kernel/suspend.c
>> +++ b/arch/riscv/kernel/suspend.c
>> @@ -15,6 +15,8 @@
>>  void suspend_save_csrs(struct suspend_context *context)
>>  {
>>  	context->scratch = csr_read(CSR_SCRATCH);
>> +	if (riscv_has_extension_likely(RISCV_ISA_EXT_Sx1p12))
>> +		context->envcfg = csr_read(CSR_ENVCFG);
>>  	context->tvec = csr_read(CSR_TVEC);
>>  	context->ie = csr_read(CSR_IE);
>>  
>> @@ -36,6 +38,8 @@ void suspend_save_csrs(struct suspend_context *context)
>>  void suspend_restore_csrs(struct suspend_context *context)
>>  {
>>  	csr_write(CSR_SCRATCH, context->scratch);
>> +	if (riscv_has_extension_likely(RISCV_ISA_EXT_Sx1p12))
>> +		csr_write(CSR_ENVCFG, context->envcfg);
>>  	csr_write(CSR_TVEC, context->tvec);
>>  	csr_write(CSR_IE, context->ie);
>>  
>> -- 
>> 2.43.0
>>
>
> We're still exposing Zicboz to userspace in hwprobe when only
> RISCV_ISA_EXT_ZICBOZ is present, which will be the case for anything that
> either doesn't implement 1.12, but does implement Zicboz, or maybe does
> implement 1.12, but hasn't started putting Ss1p12 in its ISA string yet
> (e.g. QEMU). We should either stop exposing Zicboz to userspace in those
> cases (since it won't work) or rethink how we want to determine whether
> or not we have envcfg CSRs.

opensbi treats the existence of menvcfg as sufficient evidence to prove that
the hart supports 1.12.  I wouldn't object to having Zicboz imply Ss1p12/Sm1p12.

-s

> Thanks,
> drew
>
> _______________________________________________
> linux-riscv mailing list
> linux-riscv@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-riscv

_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv

Re: [PATCH -fixes v2 4/4] riscv: Save/restore envcfg CSR during CPU suspend

[Samuel Holland]

On 2024-02-13 11:53 AM, Stefan O'Rear wrote:
> On Tue, Feb 13, 2024, at 9:49 AM, Andrew Jones wrote:
>> On Mon, Feb 12, 2024 at 07:37:35PM -0800, Samuel Holland wrote:
>>> The value of the [ms]envcfg CSR is lost when entering a nonretentive
>>> idle state, so the CSR must be rewritten when resuming the CPU.
>>>
>>> Cc: <stable@vger.kernel.org> # v6.7+
>>> Fixes: 43c16d51a19b ("RISC-V: Enable cbo.zero in usermode")
>>> Signed-off-by: Samuel Holland <samuel.holland@sifive.com>
>>> ---
>>>
>>> Changes in v2:
>>>  - Check for privileged ISA v1.12 instead of the specific CSR
>>>  - Use riscv_has_extension_likely() instead of new ALTERNATIVE()s
>>>
>>>  arch/riscv/include/asm/suspend.h | 1 +
>>>  arch/riscv/kernel/suspend.c      | 4 ++++
>>>  2 files changed, 5 insertions(+)
>>>
>>> diff --git a/arch/riscv/include/asm/suspend.h b/arch/riscv/include/asm/suspend.h
>>> index 02f87867389a..491296a335d0 100644
>>> --- a/arch/riscv/include/asm/suspend.h
>>> +++ b/arch/riscv/include/asm/suspend.h
>>> @@ -14,6 +14,7 @@ struct suspend_context {
>>>  	struct pt_regs regs;
>>>  	/* Saved and restored by high-level functions */
>>>  	unsigned long scratch;
>>> +	unsigned long envcfg;
>>>  	unsigned long tvec;
>>>  	unsigned long ie;
>>>  #ifdef CONFIG_MMU
>>> diff --git a/arch/riscv/kernel/suspend.c b/arch/riscv/kernel/suspend.c
>>> index 239509367e42..be03615486ed 100644
>>> --- a/arch/riscv/kernel/suspend.c
>>> +++ b/arch/riscv/kernel/suspend.c
>>> @@ -15,6 +15,8 @@
>>>  void suspend_save_csrs(struct suspend_context *context)
>>>  {
>>>  	context->scratch = csr_read(CSR_SCRATCH);
>>> +	if (riscv_has_extension_likely(RISCV_ISA_EXT_Sx1p12))
>>> +		context->envcfg = csr_read(CSR_ENVCFG);
>>>  	context->tvec = csr_read(CSR_TVEC);
>>>  	context->ie = csr_read(CSR_IE);
>>>  
>>> @@ -36,6 +38,8 @@ void suspend_save_csrs(struct suspend_context *context)
>>>  void suspend_restore_csrs(struct suspend_context *context)
>>>  {
>>>  	csr_write(CSR_SCRATCH, context->scratch);
>>> +	if (riscv_has_extension_likely(RISCV_ISA_EXT_Sx1p12))
>>> +		csr_write(CSR_ENVCFG, context->envcfg);
>>>  	csr_write(CSR_TVEC, context->tvec);
>>>  	csr_write(CSR_IE, context->ie);
>>>  
>>> -- 
>>> 2.43.0
>>>
>>
>> We're still exposing Zicboz to userspace in hwprobe when only
>> RISCV_ISA_EXT_ZICBOZ is present, which will be the case for anything that
>> either doesn't implement 1.12, but does implement Zicboz, or maybe does
>> implement 1.12, but hasn't started putting Ss1p12 in its ISA string yet
>> (e.g. QEMU). We should either stop exposing Zicboz to userspace in those
>> cases (since it won't work) or rethink how we want to determine whether
>> or not we have envcfg CSRs.
> 
> opensbi treats the existence of menvcfg as sufficient evidence to prove that
> the hart supports 1.12.  I wouldn't object to having Zicboz imply Ss1p12/Sm1p12.

Zicboz implies menvcfg, yes, but I don't think menvcfg is sufficient to imply
S[ms]1p12. It's entirely possible for hardware to implement menvcfg but none of
the other S[ms]1p12 features. Or it may attempt to implement S[ms]1p12, but have
some bug that prevents it from being compliant, and therefore prevents declaring
support in the devicetree/ISA string.

What I think might work best is to have a cpufeature flag for "has the envcfg
CSR" that doesn't map to anything in the devicetree/ISA string. Then Zicboz can
imply envcfg, and Ss1p12 can imply envcfg, but we avoid any possible false
implications going the other direction.

Regards,
Samuel


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv