[PATCH v3] lib: sbi: Enable Control Transfer Records (CTR) Ext using xstateen.
Samuel Holland
samuel.holland at sifive.com
Sun Apr 13 09:30:12 PDT 2025
Hi Atish,
On 2025-04-13 8:41 AM, Anup Patel wrote:
> On Fri, Apr 11, 2025 at 12:39 AM Atish Patra <atishp at rivosinc.com> wrote:
>>
>> On 3/31/25 5:46 PM, Samuel Holland wrote:
>>> Hi Rajnesh,
>>>
>>> On 2025-03-07 6:44 AM, Rajnesh Kanwal wrote:
>>>> The Control Transfer Records (CTR) extension provides a method to
>>>> record a limited branch history in register-accessible internal chip
>>>> storage.
>>>>
>>>> This extension is similar to Arch LBR in x86 and BRBE in ARM.
>>>> The Extension has been stable and the latest release can be found here
>>>> https://github.com/riscv/riscv-control-transfer-records/release
>>>>
>>>> Signed-off-by: Rajnesh Kanwal <rkanwal at rivosinc.com>
>>>>
>>>> ---
>>>> Changes in V3:
>>>> Removed FWFT bit. It was discussed in tech-prs group to
>>>> use trap and enable approach rather than FWFT bit. Trap
>>>> and enable approach leads to letter number of traps than
>>>> enabling and disabling FWFT bit on each CTR usage.
>>>> https://lists.riscv.org/g/tech-prs/message/1339
>>>
>>> If you want to use the trap and enable approach, then you cannot enable the
>>> mstateen0 bit by default, or you will never trap. This is also missing the code
>>> for the trap handler.
>>>
>>
>> I think you mean you can't trap in M-mode. However, there is no use case
>> for M-mode profiling.
>>
>> The PRS discussion was around how we handle CTR state for guests where
>> the earlier approach used FWFT instead of the trap/enable.
For clarity: yes, I do mean trapping in M-mode. This is not for M-mode
profiling, but to handle CTR state for (H)S-mode domains, just like HS-mode
needs trapping to handle state for VS-mode guests. The same rationale for
trapping applies at both privilege modes.
>>>>
>>>> Changes in V2:
>>>> Added FWFT bit for CTR.
>>>> ---
>>>> include/sbi/riscv_encoding.h | 13 +++++++++++++
>>>> include/sbi/sbi_hart.h | 4 ++++
>>>> lib/sbi/sbi_hart.c | 7 +++++++
>>>> 3 files changed, 24 insertions(+)
>>>>
>>>> diff --git a/include/sbi/riscv_encoding.h b/include/sbi/riscv_encoding.h
>>>> index 03c68a57..f3e2afd5 100644
>>>> --- a/include/sbi/riscv_encoding.h
>>>> +++ b/include/sbi/riscv_encoding.h
>>>> @@ -375,6 +375,17 @@
>>>> #define CSR_SSTATEEN2 0x10E
>>>> #define CSR_SSTATEEN3 0x10F
>>>>
>>>> +/* Machine-Level Control transfer records CSRs */
>>>> +#define CSR_MCTRCTL 0x34e
>>>> +
>>>> +/* Supervisor-Level Control transfer records CSRs */
>>>> +#define CSR_SCTRCTL 0x14e
>>>> +#define CSR_SCTRSTATUS 0x14f
>>>> +#define CSR_SCTRDEPTH 0x15f
>>>> +
>>>> +/* VS-Level Control transfer records CSRs */
>>>> +#define CSR_VSCTRCTL 0x24e
>>>> +
>>>> /* ===== Hypervisor-level CSRs ===== */
>>>>
>>>> /* Hypervisor Trap Setup (H-extension) */
>>>> @@ -799,6 +810,8 @@
>>>> #define SMSTATEEN0_CS (_ULL(1) << SMSTATEEN0_CS_SHIFT)
>>>> #define SMSTATEEN0_FCSR_SHIFT 1
>>>> #define SMSTATEEN0_FCSR (_ULL(1) << SMSTATEEN0_FCSR_SHIFT)
>>>> +#define SMSTATEEN0_CTR_SHIFT 54
>>>> +#define SMSTATEEN0_CTR (_ULL(1) << SMSTATEEN0_CTR_SHIFT)
>>>> #define SMSTATEEN0_CONTEXT_SHIFT 57
>>>> #define SMSTATEEN0_CONTEXT (_ULL(1) << SMSTATEEN0_CONTEXT_SHIFT)
>>>> #define SMSTATEEN0_IMSIC_SHIFT 58
>>>> diff --git a/include/sbi/sbi_hart.h b/include/sbi/sbi_hart.h
>>>> index 4c36c778..c3a7feb7 100644
>>>> --- a/include/sbi/sbi_hart.h
>>>> +++ b/include/sbi/sbi_hart.h
>>>> @@ -75,6 +75,10 @@ enum sbi_hart_extensions {
>>>> SBI_HART_EXT_ZICFISS,
>>>> /** Hart has Ssdbltrp extension */
>>>> SBI_HART_EXT_SSDBLTRP,
>>>> + /** HART has CTR M-mode CSRs */
>>>> + SBI_HART_EXT_SMCTR,
>>>> + /** HART has CTR S-mode CSRs */
>>>> + SBI_HART_EXT_SSCTR,
>>>>
>>>> /** Maximum index of Hart extension */
>>>> SBI_HART_EXT_MAX,
>>>> diff --git a/lib/sbi/sbi_hart.c b/lib/sbi/sbi_hart.c
>>>> index 8e2979b5..c343805c 100644
>>>> --- a/lib/sbi/sbi_hart.c
>>>> +++ b/lib/sbi/sbi_hart.c
>>>> @@ -105,6 +105,11 @@ static void mstatus_init(struct sbi_scratch *scratch)
>>>> else
>>>> mstateen_val &= ~(SMSTATEEN0_SVSLCT);
>>>>
>>>> + if (sbi_hart_has_extension(scratch, SBI_HART_EXT_SSCTR))
>>>> + mstateen_val |= SMSTATEEN0_CTR;
>>>> + else
>>>> + mstateen_val &= ~SMSTATEEN0_CTR;
>>>
>>> The purpose of the Smstateen extension is to prevent side channels between
>>> contexts, by blocking access to registers that are not context switched. That
>>> means any code that sets a new mstateen0 bit needs to also include code to
>>> perform the context switch, or you are creating the side channel anyway.
>>>
>>
>> yes. We need to save/restore the CTR state during domain context switch.
>> I am not aware of any use cases where we are actually running two
>> different worlds with opensbi domains that require this at this point.
Upstream OpenSBI does not have any built-in way to trigger a domain context
switch at the moment, but there are several downstream forks/users that do, for
example the OP-TEE and StandaloneMM PoCs. These may have production users, if
not already, then soon.
>> Once we have full supervisor domain extension support, these must be
>> taken care of. Can we merge this with a TODO item about domain context
>> switch issue ?
>
> Ideally, we should not leave the CTR state enabled by default even
> in absence of domain context switch. Currently, we already have
> AIA and a few other bits which are enabled by default in mstateen0
> at boot-time but those should also be enabled lazily upon first access
> fault to align with the purpose of Smstateen extension.
>
> For now, let's go ahead with this patch but separately we need to
> add infrastructure in OpenSBI to lazily enable bits in Smstateen
> upon first-access-trap.
Right, this is not the first extension missing context switch support. This
patch is adding more technical debt, but it is not really a new problem, so I
can accept pushing off the fix for now.
Regards,
Samuel
More information about the opensbi
mailing list