Re: [PATCH v6 09/21] dt-bindings: riscv: add Counter delegation ISA extensions description

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Incorrect dependency for smcdeleg: depends on sscsrind instead of smcsrind.
- [Medium] The ssccfg extension is over-constrained by strictly requiring smcntrpmf.
- [Low] Contradictory description for the smcdeleg extension.
--

commit 53fb261803de68f9caaa685d63588c9cac6c0f5c
Author: Atish Patra <atishp@rivosinc.com>

dt-bindings: riscv: add Counter delegation ISA extensions description

This commit adds device tree binding descriptions for the Smcdeleg and
Ssccfg RISC-V ISA extensions. It also specifies the conditional dependencies
required for these extensions to pass schema validation.

> diff --git a/Documentation/devicetree/bindings/riscv/extensions.yaml b/Documentation/devicetree/bindings/riscv/extensions.yaml
> index ece3edccee42c..2845e8e2999a7 100644
> --- a/Documentation/devicetree/bindings/riscv/extensions.yaml
> +++ b/Documentation/devicetree/bindings/riscv/extensions.yaml
> @@ -181,6 +181,13 @@ properties:
>              changes to interrupts as frozen at commit ccbddab ("Merge pull
>              request #42 from riscv/jhauser-2023-RC4") of riscv-aia.
>  
> +        - const: smcdeleg
> +          description: |
> +            The standard Smcdeleg supervisor-level extension for the machine mode

[Severity: Low]
This isn't a bug, but should this say machine-level instead of
supervisor-level? Since it has an Sm prefix and is meant for machine mode,
calling it a supervisor-level extension might be a copy-paste typo.

> +            to delegate the hpmcounters to supervisor mode so that they are
> +            directly accessible in the supervisor mode as ratified in the
> +            20240213 version of the privileged ISA specification.

[ ... ]

> @@ -1135,6 +1150,36 @@ properties:
>              allOf:
>                - const: zilsd
>                - const: zca
> +      # Smcdeleg depends on Sscsrind, Zihpm, Zicntr
> +      - if:
> +          contains:
> +            const: smcdeleg
> +        then:
> +          allOf:
> +            - contains:
> +                const: sscsrind

[Severity: Medium]
Does smcdeleg actually depend on sscsrind here?

Since smcdeleg is a machine-mode extension that uses M-mode indirect CSRs to
delegate counters, shouldn't it depend on the smcsrind extension instead of
sscsrind? If a valid hardware configuration implements smcsrind and smcdeleg
without sscsrind, would this schema incorrectly reject it?

> +            - contains:
> +                const: zihpm
> +            - contains:
> +                const: zicntr
> +      # Ssccfg depends on Smcdeleg, Sscsrind, Zihpm, Zicntr, Sscofpmf, Smcntrpmf
> +      - if:
> +          contains:
> +            const: ssccfg
> +        then:
> +          allOf:
> +            - contains:
> +                const: smcdeleg
> +            - contains:
> +                const: sscsrind
> +            - contains:
> +                const: sscofpmf
> +            - contains:
> +                const: smcntrpmf

[Severity: Medium]
Is smcntrpmf a strict requirement for ssccfg?

The ssccfg extension provides supervisor-mode access to delegated counters,
but the kernel implementation seems to only conditionally expose
cyclecfg/instretcfg CSRs if smcntrpmf is present. If ssccfg can exist
independently, will requiring smcntrpmf in this binding incorrectly fail
validation for valid hardware that omits it?

> +            - contains:
> +                const: zihpm
> +            - contains:
> +                const: zicntr

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260608-counter_delegation-v6-0-285b72ed65a9@meta.com?part=9