[PATCH 2/3] lib: sbi: Zicfilp/Zicfiss detection and elp cfi state reflect back in status

[Deepak Gupta]

On Wed, Aug 21, 2024 at 07:32:35PM -0500, Samuel Holland wrote:
>Hi Deepak,
>
>On 2024-08-21 6:55 PM, Deepak Gupta wrote:
>> This patch adds support for zicfilp / zicfiss detection in sbi_hart.c
>> If zicfilp and zicfiss are detected, this patch turns on menvcfg.LPE and
>> menvcfg.SSE
>>
>> Zicfilp records status of hart's ELP state in *status csr. Missing landing pad
>> sets MPELP in mstatus. When SBI is redirecting back to S/VS/HS, SPELP is
>> set in sstatus/vsstatus.
>>
>> Signed-off-by: Deepak Gupta <debug at rivosinc.com>
>> ---
>>  lib/sbi/sbi_hart.c | 28 ++++++++++++++++++++++++++++
>>  lib/sbi/sbi_trap.c | 16 ++++++++++++++++
>>  2 files changed, 44 insertions(+)
>>
>> diff --git a/lib/sbi/sbi_hart.c b/lib/sbi/sbi_hart.c
>> index c366701..67a2e42 100644
>> --- a/lib/sbi/sbi_hart.c
>> +++ b/lib/sbi/sbi_hart.c
>> @@ -148,6 +148,16 @@ static void mstatus_init(struct sbi_scratch *scratch)
>>  		if (sbi_hart_has_extension(scratch, SBI_HART_EXT_SVADE))
>>  			menvcfg_val &= ~ENVCFG_ADUE;
>>
>> +		/*
>> +		 * By default allow shadow stack opreations in S/HS mode
>> +		 * don't enable landing pad because supervisor may keep faulting
>> +		 * due to missing landing pad. Open up a SBI interface to enable
>> +		 * landing pad
>> +		 */
>> +		if (sbi_hart_has_extension(scratch, SBI_HART_EXT_ZICFISS)) {
>> +			menvcfg_val |= ENVCFG_SSE;
>> +		}
>
>This violates the default value provided for the SHADOW_STACK feature in the SBI
>FWFT extension specification[1].
>
>[1]:
>https://github.com/riscv-non-isa/riscv-sbi-doc/blob/master/src/ext-firmware-features.adoc#user-content-table_fw_features_attribute_values
>

aah right. I remember now.
Kernel can't have shadow stack enabled from the get go.
I mean technically it can but someone (loader, grub, etc) will have to
allocate shadow for kernel. Although that's not how linux kernel is today.

So we can't have it enabled by default else shadow stack enabled kernel will
start tripping.

Will fix it.

>> +
>>  		csr_write(CSR_MENVCFG, menvcfg_val);
>>  #if __riscv_xlen == 32
>>  		csr_write(CSR_MENVCFGH, menvcfg_val >> 32);
>> @@ -680,6 +690,8 @@ const struct sbi_hart_ext_data sbi_hart_ext[] = {
>>  	__SBI_HART_EXT_DATA(ssccfg, SBI_HART_EXT_SSCCFG),
>>  	__SBI_HART_EXT_DATA(svade, SBI_HART_EXT_SVADE),
>>  	__SBI_HART_EXT_DATA(svadu, SBI_HART_EXT_SVADU),
>> +	__SBI_HART_EXT_DATA(zicfilp, SBI_HART_EXT_ZICFILP),
>> +	__SBI_HART_EXT_DATA(zicfiss, SBI_HART_EXT_ZICFISS),
>>  };
>>
>>  _Static_assert(SBI_HART_EXT_MAX == array_size(sbi_hart_ext),
>> @@ -776,6 +788,7 @@ static int hart_detect_features(struct sbi_scratch *scratch)
>>  	unsigned long val, oldval;
>>  	bool has_zicntr = false;
>>  	int rc;
>> +	bool ssp_exist, elp_exist;
>>
>>  	/* If hart features already detected then do nothing */
>>  	if (hfeatures->detected)
>> @@ -933,6 +946,21 @@ __pmp_skip:
>>  	/* Save trap based detection of Zicntr */
>>  	has_zicntr = sbi_hart_has_extension(scratch, SBI_HART_EXT_ZICNTR);
>>
>> +	if (hfeatures->priv_version >= SBI_HART_PRIV_VER_1_12) {
>> +		val = csr_read_allowed(CSR_SSP, (unsigned long)&trap);
>> +		ssp_exist = trap.cause == 0;
>
>Would it be simpler to try setting menvcfg.SSE here, since that would never
>cause a trap?

Moot point now.
>
>> +		if (ssp_exist)
>> +			__sbi_hart_update_extension(hfeatures,
>> +					SBI_HART_EXT_ZICFISS, true);
>> +
>> +		csr_set(CSR_MSTATUS, MSTATUS_MPELP);
>> +		val = csr_read_clear(CSR_MSTATUS, MSTATUS_MPELP);
>> +		elp_exist = val & MSTATUS_MPELP;
>> +		if (elp_exist)
>> +			__sbi_hart_update_extension(hfeatures,
>> +					SBI_HART_EXT_ZICFILP, true);
>> +	}
>> +
>>  	/* Let platform populate extensions */
>>  	rc = sbi_platform_extensions_init(sbi_platform_thishart_ptr(),
>>  					  hfeatures);
>> diff --git a/lib/sbi/sbi_trap.c b/lib/sbi/sbi_trap.c
>> index b4f3a17..2273b3a 100644
>> --- a/lib/sbi/sbi_trap.c
>> +++ b/lib/sbi/sbi_trap.c
>> @@ -103,6 +103,7 @@ int sbi_trap_redirect(struct sbi_trap_regs *regs,
>>  		      const struct sbi_trap_info *trap)
>>  {
>>  	ulong hstatus, vsstatus, prev_mode;
>> +	bool elp = false;
>>  #if __riscv_xlen == 32
>>  	bool prev_virt = (regs->mstatusH & MSTATUSH_MPV) ? true : false;
>>  #else
>> @@ -116,6 +117,13 @@ int sbi_trap_redirect(struct sbi_trap_regs *regs,
>>  	if (prev_mode != PRV_S && prev_mode != PRV_U)
>>  		return SBI_ENOTSUPP;
>>
>> +	/* If extension has support for CFI, clear MPELP because redirecting to VS or (H)S */
>> +	if (sbi_hart_has_extension(sbi_scratch_thishart_ptr(), SBI_HART_EXT_ZICFILP)) {
>> +		elp = (regs->mstatus & MSTATUS_MPELP)? true: false;
>
>The ternary expression here is unnecessary. The left side is already truthy/falsy.

Yeah I just followed style for `prev_virt` earlier in this function.
I can change this one.

>
>Regards,
>Samuel
>
>> +		/* Since redirecting, clear mpelp unconditionally */
>> +		regs->mstatus &= ~MSTATUS_MPELP;
>> +	}
>> +
>>  	/* If exceptions came from VS/VU-mode, redirect to VS-mode if
>>  	 * delegated in hedeleg
>>  	 */
>> @@ -169,6 +177,10 @@ int sbi_trap_redirect(struct sbi_trap_regs *regs,
>>  		/* Get VS-mode SSTATUS CSR */
>>  		vsstatus = csr_read(CSR_VSSTATUS);
>>
>> +		/*if elp was set, set it back in vsstatus */
>> +		if (elp)
>> +			vsstatus |= MSTATUS_SPELP;
>> +
>>  		/* Set SPP for VS-mode */
>>  		vsstatus &= ~SSTATUS_SPP;
>>  		if (prev_mode == PRV_S)
>> @@ -209,6 +221,10 @@ int sbi_trap_redirect(struct sbi_trap_regs *regs,
>>
>>  		/* Clear SIE for S-mode */
>>  		regs->mstatus &= ~MSTATUS_SIE;
>> +
>> +		/* if elp was set, set it back in mstatus */
>> +		if (elp)
>> +			regs->mstatus |= MSTATUS_SPELP;
>>  	}
>>
>>  	return 0;
>