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

[Atish Patra <atish.patra@linux.dev>]

On 6/8/26 11:12 PM, sashiko-bot@kernel.org wrote:
> 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.

In this case, it is not true. Quoting the RISC-V spec.
For a RISC-V hardware platform, Smcdeleg and Ssccfg must always be 
implemented in tandem.

https://docs.riscv.org/reference/isa/v20260120/priv/smcdeleg.html

>> +            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?

Yes. Answered above.


>> +            - 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?

While it is ideal to have that, we can relax that dependency.

Smcntrpmf is strictly dependant on Smcdeleg/Ssccfg extensions 
implementation but not the other way around.

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