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

[Jessica Clarke]

On 14 May 2025, at 01:49, Samuel Holland <samuel.holland at sifive.com> wrote:
> 
> 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.

Should. But in practice these things get missed, and FreeBSD does not
have code to do that today. Is that a bug? Sure. But try arguing that
for a Linux kernel change that breaks userspace; it’s the same argument
here. I can add it, but that won’t help existing releases, and I’d like
to not have users’ systems break when they update their firmware.

Jess

> 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