[PATCH v2 0/5] Add Svadu extension support

[Alexandre Ghiti]

Hi Anup,

On 24/05/2024 18:05, Anup Patel wrote:
> Hi Alex,
>
> On Fri, May 24, 2024 at 5:01 PM Alexandre Ghiti <alex at ghiti.fr> wrote:
>> Hi Anup,
>>
>> On 14/11/2023 17:45, Anup Patel wrote:
>>> On Tue, Oct 24, 2023 at 3:42 PM Yong-Xuan Wang <yongxuan.wang at sifive.com> wrote:
>>>> This series enables Svadu extension support by configuring the menvcfg
>>>> CSR and, if available, displays the Svadu extension in the boot log.
>>>>
>>>> Additionally, we've made some programming improvements in
>>>> lib/sbi/sbi_hart.c and lib/utils/fdt/fdt_helper.c.
>>>>
>>>> ---
>>>> v2:
>>>> - Rearrange the patches to do the code refactoring first before adding
>>>>     new features
>>>> - Suggested by Anup and Atish, detect extensions from DT instead of
>>>>     menvcfg CSR
>>>> - Enable access to some extensions through menvcfg CSR if they are
>>>>     present in the device tree.
>>>>
>>>> Yong-Xuan Wang (5):
>>>>     lib: sbi: Improve the code of privilege mode and extensions detection
>>>>     lib: sbi: Refactor the code for enable extensions in menvfg CSR
>>>>     lib: sbi: Using one array to define the name of extensions
>>>>     lib: sbi: Detect extensions from the ISA string in DT
>>>>     lib: sbi: Add support for Svadu extension
>>> For backward compatibility with existing OSes, it is better to have
>>> supervisor OS explicitly enable Svadu using the upcoming SBI
>>> FWFT extension instead of enabling it unconditionally whenever
>>> Svadu extension is available.
>>
>> I find this weird because maintaining backward compatibility here means
>> "continue ignoring svadu present in the device tree".
>>
>> Why should we treat svadu differently than svpbmt? I understand FWFT
>> will fix this, but will that be available?
> Svpbmt defines a backward compatible PTE encoding so OSes
> unaware of Svpbmt work fine. Unfortunately, this is not true for
> Svadu because it changes trapping behaviour of PTE A/D bits
> in a non-backward compatible way.


Svadu-unaware kernels will protect pages so that they can set A/D bits 
when taking the trap, but that's not incompatible with Svadu. The 
setting of A or D bit in HW does not prevent this to work, so that's not 
non-backward compatible, it is just redundant.


>
>> To me, enabling svadu when present in the device tree is more a fix than
>> an issue: if enabling it breaks something, that means svadu is broken on
>> your platform so just remove that from your dt or enable the support in
>> your kernel.
> It's not broken; rather it has backward compatibility issues
> when left enabled at boot time.
>
> Unfortunately, the switch to enable Svadu for a privilege mode
> (S/VS) lies with higher privilege mode (M/HS) as-per RISC-V
> privileged spec design philosophy so we need an SBI call.


FYI, I was wondering why it needed M-mode intervention and Ved gave me 
the rationale: https://lists.riscv.org/g/tech-privileged/message/1907


>
> We do have such switches for other ISA extensions as well and
> the SBI implementation (M-mode or HS-modes) enables such
> switches whenever backward compatibility is not an issue.
>
>> In a nutshell, if asked by the dt, that means the support is present in
>> the kernel and then it expects it and should be enabled.
>>
>> Let me know what you think,
> At the moment, the presence of a XYZ extension in device tree
> or ACPI only implies that XYZ extension is present and it says
> nothing about XYZ extension being enabled.
>
> I think we should improve Linux documentation to explicitly say
> the default/boot-time enable status of each extension.


Yes, definitely, something needs to be done, not all extensions have the 
semantics, which can be counter-intuitive.

But for Svadu, enabling it by default will not break svadu-unaware OS, 
so I still think we should enable it by default.

Thanks,

Alex


>
> Regards,
> Anup
>
>
>> Thanks,
>>
>> Alex
>>
>>
>>> Regards,
>>> Anup
>>>
>>>>    include/sbi/riscv_encoding.h |   6 +-
>>>>    include/sbi/sbi_hart.h       |  15 +++
>>>>    lib/sbi/sbi_hart.c           | 226 +++++++++++++----------------------
>>>>    lib/utils/fdt/fdt_helper.c   |   5 +-
>>>>    4 files changed, 104 insertions(+), 148 deletions(-)
>>>>
>>>> --
>>>> 2.17.1
>>>>
>>>>
>>>> --
>>>> opensbi mailing list
>>>> opensbi at lists.infradead.org
>>>> http://lists.infradead.org/mailman/listinfo/opensbi