[PATCH v2 0/5] Add Svadu extension support

[Andrew Jones]

On Fri, May 24, 2024 at 09:38:30PM GMT, Alexandre Ghiti wrote:
> 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.

If an OS only sets A/D bits when it gets the traps, then it's redundant.
If an OS sets A/D bits and does some other stuff, then it's not.

> 
> 
> > 
> > > 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.

We also have extensions which appear to just state what to expect at boot
time (afaict). For example, svade just says "A/D bits will cause
exceptions". So, when combined with svadu (to me) it implies one should
expect exceptions at boot but have the ability to flip on hardware A/D
updating if desired. Without svade also in the DT, then (to me) it would
imply svadu is on by default. Or, for completeness, here's the table

 svade  svadu
 0      0        -- who knows... let's assume svade
 1      0        -- svade on by default and the only option
 0      1        -- svadu on by default and the only option
 1      1        -- svade on by default, svadu is an option

But I could be inferring/assuming too much!

Thanks,
drew


> 
> 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
> 
> -- 
> opensbi mailing list
> opensbi at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/opensbi