[PATCH] lib: sbi: Only enable TM bit in scounteren

[Samuel Holland]

Hi Jess,

On 2025-05-13 7:35 PM, Jessica Clarke wrote:
> On 14 May 2025, at 01:25, Atish Patra <atishp at rivosinc.com> wrote:
> 
>> The S-mode should disable Cycle and instruction counter for user space
>> to avoid side channel attacks. The Linux kernel already does this so that
>> any random user space code shouldn't be able to monitor cycle/instruction
>> without higher privilege mode involvement.
>>
>> Remove the CY/IR bits in scountern in OpenSBI.
> 
> Isn’t this a breaking change? S-mode OSes that are happy to allow
> U-mode to read the counters are now broken, such as FreeBSD. BBL always
> set scounteren to all ones (with bits above 2 being pointless without
> programming mhpmeventX), and OpenSBI did that until it switched to this
> behaviour of CY|TM|IR. So whether or not it was explicit in the
> specification what these were initialised to, S-mode OSes exist that
> assumed CY, TM and IR at least were available. Please do not break
> things; treat the firmware<->S-mode interface in the same way as
> kernel<->userspace, i.e. don’t break userspace/S-mode.
> 
> OSes that want to opt out can. And if you really want to push for this,
> you need to wait for OSes to be changed to explicitly initialise
> scounteren and only do it once old versions are sufficiently rare. But
> you can’t just go doing this with no warning / deprecation period.

This shouldn't be a breaking change for any OS that supports SMP. While the
architectural state of the boot hart upon S-mode entry is not well defined, the
SBI HSM specification explicitly states that "All other registers remain in an
undefined state." (This wording needs to be modified in SBI 3.0 to account for
FWFT defining the reset value of most features, but the point stands.) So it
would be inappropriate for an OS to rely on the preexisting scounteren CSR value
on any secondary hart. Thus, any SMP OS must have code to initialize scounteren
anyway.

Regards,
Samuel

> NAK.
> 
> Jess
> 
>> Signed-off-by: Atish Patra <atishp at rivosinc.com>
>> ---
>> To: anup Patel <anup at brainfault.org>
>> Cc: opensbi at lists.infradead.org
>> ---
>> lib/sbi/sbi_hart.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/lib/sbi/sbi_hart.c b/lib/sbi/sbi_hart.c
>> index fc4925b57625..177f356b7b77 100644
>> --- a/lib/sbi/sbi_hart.c
>> +++ b/lib/sbi/sbi_hart.c
>> @@ -49,10 +49,10 @@ static void mstatus_init(struct sbi_scratch *scratch)
>>
>> csr_write(CSR_MSTATUS, mstatus_val);
>>
>> - /* Disable user mode usage of all perf counters except default ones (CY, TM, IR) */
>> + /* Disable user mode usage of all perf counters except TM */
>> if (misa_extension('S') &&
>>    sbi_hart_priv_version(scratch) >= SBI_HART_PRIV_VER_1_10)
>> - csr_write(CSR_SCOUNTEREN, 7);
>> + csr_write(CSR_SCOUNTEREN, 0x02);
>>
>> /**
>> * OpenSBI doesn't use any PMU counters in M-mode.
>>
>> ---
>> base-commit: 316daaf1c299c29ac46e52145f65521f48ec63b5
>> change-id: 20250512-fix_scounteren-8b7b682bb864
>> --
>> Regards,
>> Atish patra
>>
>>
>> -- 
>> opensbi mailing list
>> opensbi at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/opensbi
> 
>