[PATCH v3] lib: sbi: Enable Control Transfer Records (CTR) Ext using xstateen.
Atish Patra
atishp at rivosinc.com
Mon Apr 14 14:08:49 PDT 2025
On 4/13/25 6: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.
>>
>>>>
>>>> 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.
>>
>> 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.
>
Yes. I will send a patch for lazily enabling smstateen bits in opensbi &
kvm.
> 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.
>
> Reviewed-by: Anup Patel <anup at brainfault.org>
>
> Applied this patch to the riscv/opensbi repo.
>
> Thanks,
> Anup
>
>
>
>>
>>> Regards,
>>> Samuel
>>>
>>>> +
>>>> csr_write(CSR_MSTATEEN0, mstateen_val);
>>>> #if __riscv_xlen == 32
>>>> csr_write(CSR_MSTATEEN0H, mstateen_val >> 32);
>>>> @@ -688,6 +693,8 @@ const struct sbi_hart_ext_data sbi_hart_ext[] = {
>>>> __SBI_HART_EXT_DATA(zicfilp, SBI_HART_EXT_ZICFILP),
>>>> __SBI_HART_EXT_DATA(zicfiss, SBI_HART_EXT_ZICFISS),
>>>> __SBI_HART_EXT_DATA(ssdbltrp, SBI_HART_EXT_SSDBLTRP),
>>>> + __SBI_HART_EXT_DATA(smctr, SBI_HART_EXT_SMCTR),
>>>> + __SBI_HART_EXT_DATA(ssctr, SBI_HART_EXT_SSCTR),
>>>> };
>>>>
>>>> _Static_assert(SBI_HART_EXT_MAX == array_size(sbi_hart_ext),
>>>
>>>
>>
>>
>> --
>> opensbi mailing list
>> opensbi at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/opensbi
>
More information about the opensbi
mailing list