[PATCH] lib: sbi: Fix version dependency for Sscofpmf

[Samuel Holland]

On 6/16/22 11:56 PM, Anup Patel wrote:
> On Fri, Jun 17, 2022 at 9:58 AM Samuel Holland <samuel at sholland.org> wrote:
>>
>> Hi Anup,
>>
>> On 6/16/22 11:12 PM, Anup Patel wrote:
>>> On Mon, Jun 13, 2022 at 6:08 AM Samuel Holland <samuel at sholland.org> wrote:
>>>>
>>>> mcounteren was added in the privileged spec v1.10 and mcountinhibit was
>>>> added in v1.11. Sscofpmf does not depend on anything in v1.12, so the
>>>> minimum privileged spec version should have been v1.11.
>>>>
>>>> Fixes: d4b563c881d6 ("lib: sbi: Remove MCOUNTEREN and SCOUNTEREN hart features")
>>>> Signed-off-by: Samuel Holland <samuel at sholland.org>
>>>
>>> I think there is some confusion here.
>>>
>>> The mcounteren and mcountinhibit are not part of the Sscofpmf extension and
>>> OpenSBI PMU implementation does support platforms without Sscofpmf.
>>
>> I am aware of that. I am referencing the comment on the previous line, and the
>> code that was changed by the referenced commit. The code, as originally written,
>> attempted to optimize out the Sscofpmf check based on the presence of those two
>> CSRs.
>>
>> When the hart features were refactored, the CSR feature was replaced by the
>> wrong version, i.e. the behavior was accidentally changed by the refactoring.
>>
>> This patch only attempts to restore the behavior from before the refactoring. If
>> that behavior was wrong to start with, that is a different matter.
> 
> I agree there is nothing in Sscofpmf which prevents a Priv v1.11 HART from
> implementing it so this now becomes a strange choice on the OpenSBI side.
> 
> IMO, the previous behaviour of detecting and using Sscofpmf irrespective
> of the Priv spec version was wrong because Sscofpmf is defined assuming
> Priv v1.12 spec. In fact, that's why Sscofpmf defines bits for filtering events
> in VS/VU modes.
> 
> Is there a case where we would need Sscofpmf for a Priv v1.11 HART ?

I was attempting to add C906 PMU support as an errata on top of the standard
extension, which is when I noticed this check. (C906 supports v1.11). However, I
realized C906's custom overflow handling is different enough from Sscofpmf that
I should implement it separately.

So I do not know if a v1.11+Sscofpmf case does or should exist. Maybe the check
is correct now, in which case the comment is a bit misleading.

Regards,
Samuel

>>>> ---
>>>>
>>>>  lib/sbi/sbi_hart.c | 2 +-
>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/lib/sbi/sbi_hart.c b/lib/sbi/sbi_hart.c
>>>> index de86fee..5bc6d52 100644
>>>> --- a/lib/sbi/sbi_hart.c
>>>> +++ b/lib/sbi/sbi_hart.c
>>>> @@ -645,7 +645,7 @@ __mhpm_skip:
>>>>                 hfeatures->priv_version = SBI_HART_PRIV_VER_1_12;
>>>>
>>>>         /* Counter overflow/filtering is not useful without mcounter/inhibit */
>>>> -       if (hfeatures->priv_version >= SBI_HART_PRIV_VER_1_12) {
>>>> +       if (hfeatures->priv_version >= SBI_HART_PRIV_VER_1_11) {
>>>>                 /* Detect if hart supports sscofpmf */
>>>>                 csr_read_allowed(CSR_SCOUNTOVF, (unsigned long)&trap);
>>>>                 if (!trap.cause)
>>>> --
>>>> 2.35.1
>>>>
>>>>
>>>> --
>>>> opensbi mailing list
>>>> opensbi at lists.infradead.org
>>>> http://lists.infradead.org/mailman/listinfo/opensbi
>>