From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Jones Date: Mon, 27 May 2024 14:22:16 +0200 Subject: [PATCH v2 0/5] Add Svadu extension support In-Reply-To: References: <20231024101145.32000-1-yongxuan.wang@sifive.com> <20c80bc9-e5a0-4173-9546-061eff7add43@ghiti.fr> Message-ID: <20240527-ad16eb90d7feab51227e17ed@orel> List-Id: To: opensbi@lists.infradead.org MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit 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 wrote: > > > Hi Anup, > > > > > > On 14/11/2023 17:45, Anup Patel wrote: > > > > On Tue, Oct 24, 2023 at 3:42?PM Yong-Xuan Wang 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