[PATCH v3] lib: sbi: Enable Control Transfer Records (CTR) Ext using xstateen.

[Atish Patra]

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 ?

> 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),
> 
>