[RFC PATCH 3/4] lib: sbi: implement SBI_FWFT_DOUBLE_TRAP_ENABLE

[Samuel Holland]

Hi Clément,

On 2024-04-22 7:30 AM, Clément Léger wrote:
> On 22/04/2024 11:15, Clément Léger wrote:
>> On 19/04/2024 00:31, Samuel Holland wrote:
>>> On 2024-04-18 8:54 AM, Clément Léger wrote:
>>>> Add support for double trap firmware feature
>>>>
>>>> Signed-off-by: Clément Léger <cleger at rivosinc.com>
>>>> ---
>>>>  lib/sbi/sbi_fwft.c | 53 +++++++++++++++++++++++++++++++++++++++++++++-
>>>>  1 file changed, 52 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/lib/sbi/sbi_fwft.c b/lib/sbi/sbi_fwft.c
>>>> index 78fbd45..965d47a 100644
>>>> --- a/lib/sbi/sbi_fwft.c
>>>> +++ b/lib/sbi/sbi_fwft.c
>>>> @@ -34,7 +34,7 @@ static unsigned long fwft_ptr_offset;
>>>>  
>>>>  #define MIS_DELEG (1UL << CAUSE_MISALIGNED_LOAD | 1UL << CAUSE_MISALIGNED_STORE)
>>>>  
>>>> -#define SUPPORTED_FEATURE_COUNT	3
>>>> +#define SUPPORTED_FEATURE_COUNT	4
>>>>  
>>>>  struct fwft_config;
>>>>  
>>>> @@ -149,6 +149,51 @@ static int sbi_get_adue(struct fwft_config *conf, unsigned long *value)
>>>>  	return SBI_OK;
>>>>  }
>>>>  
>>>> +#if __riscv_xlen == 32
>>>> +# define CSR_MENVCFG_DBLTRP	CSR_MENVCFGH
>>>> +# define DBLTRP_DTE	(ENVCFG_DTE >> 32)
>>>> +#else
>>>> +# define CSR_MENVCFG_DBLTRP	CSR_MENVCFG
>>>> +# define DBLTRP_DTE	ENVCFG_DTE
>>>> +#endif
>>>> +
>>>> +static int sbi_double_trap_supported(struct fwft_config *conf)
>>>> +{
>>>> +	if (!sbi_hart_has_extension(sbi_scratch_thishart_ptr(),
>>>> +				    SBI_HART_EXT_SSDBLTRP))
>>>> +		return SBI_ENOTSUPP;
>>>> +
>>>> +	return SBI_OK;
>>>> +}
>>>> +
>>>> +static int sbi_set_double_trap_enable(struct fwft_config *conf,
>>>> +				      unsigned long value)
>>>> +{
>>>> +	struct sbi_scratch *scratch = sbi_scratch_thishart_ptr();
>>>> +
>>>> +	if (!sbi_hart_has_extension(scratch, SBI_HART_EXT_SSDBLTRP))
>>>> +		return SBI_ENOTSUPP;
>>>> +
>>>> +	if (value)
>>>> +		csr_set(CSR_MENVCFG_DBLTRP, DBLTRP_DTE);
>>>> +	else
>>>> +		csr_clear(CSR_MENVCFG_DBLTRP, DBLTRP_DTE);
>>>
>>> This configuration will be lost after hart non-retentive suspend. Probably
>>> mstatus_init() should only be called during boot and the registers
>>> saved/restored across hart suspend. Looking at your branch, the other FWFT
>>> features have the same problem.
>>
>> Hi Samuel, Yes indeed (And I think you already mentioned that on some
>> other series/meeting, sorry for that). I'll modify it on the fwft branch.
> 
> I actually believe that we said it was going the responsibility of
> S-mode software to request the features at suspend/resume time.
> Typically, on Linux, the delegation request is done using a cpuhotplug
> callback which will be called each time the CPU is suspended/resumed.

It would be S-mode software's responsibility to configure hart-local features
after HSM hart stop/start, which corresponds to Linux CPU hotplug. Above I am
referring to HSM hart suspend, which corresponds to Linux cpuidle. It doesn't
make sense to lose the firmware context in the hart suspend case, especially
considering that hart suspend is latency sensitive.

Regards,
Samuel