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

[Clément Léger]

On 22/04/2024 17:23, Samuel Holland wrote:
> 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.

Acked, I actually confused myself between suspend/resume & stop/start...
Thanks, I'll definitely looked into that.

Clément

> 
> Regards,
> Samuel
>