Re: [PATCH v2 1/6] xen/riscv: fix Svade/Svadu A/D bit handling

[Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>]

On 2026-09-22 17:29 +0200, Oleksii Kurochko wrote:
> 
> 
> On 9/10/26 11:34 AM, Baptiste Le Duc wrote:
> > p2m_set_permission() only presets the PTE A/D bits when the Svade extension
> > is present in the device tree. This causes an unhandled page fault when
> > neither Svade nor Svadu is present (the platform's actual behaviour is then
> > unknown), and when both are present in the device tree.
> 
> When both are present, RISCV_ISA_EXT_svade is set, so the current code 
> does preset the A/D bits and no fault happens. The only broken case is 
> when neither extension is present, so shouldn't "both present" be dropped?
Yes i agree, I'll fix that in v3.
> 
> > 
> > Move the Svade/Svadu resolution out of p2m_set_permission() and into a new
> > riscv_resolve_ad_scheme(), called once from riscv_fill_hwcap(). For each of
> > the four possible Svade/Svadu combinations (inspired by [1]), it decides
> > whether software has to preset the A/D bits and, if so, sets
> > RISCV_ISA_EXT_svade to record that decision:
> > - neither present: assume Svade, since assuming Svade is harmless on real
> >    Svadu hardware, while assuming Svadu on real Svade hardware risks an
> >    unhandled page fault
> > - only Svade present: assume Svade
> > - only Svadu present: leave A/D management to hardware
> > - both present: Svade wins until Xen supports the SBI FWFT call needed to
> >    enable hardware updating of A/D bits, so assume Svade and warn that
> >    dropping 'svade' from the DT is the only way to get Svadu.
> > 
> > [1] https://lwn.net/Articles/980016/
> > 
> > Fixes: ff14053983b0 ("xen/riscv: Implement p2m_pte_from_mfn() and support 
> > PBMT configuration")
> > Assisted-by: Claude:claude-opus-5
> > Signed-off-by: Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>
> > ---
> > Changes since v1:
> > - change commit title
> > - expose RISCV_ISA_EXT_svadu so the two extensions can be told apart.
> > - move the Svade/Svadu resolution to a new riscv_resolve_ad_scheme(),
> >    called once from riscv_fill_hwcap().
> > - expose sbi_probe_extension() (was static) to probe for SBI FWFT.
> 
> sbi_probe_extension() is already non-static in staging, only the prototype
> is missing. What base is this patch against?
> 
> > - stop presetting A/D bits unconditionally in p2m_set_permission(), do it
> >    only when Svade is present.
> 
> What is the gain from not presetting them? Presetting A/D is correct 
> with both Svade and Svadu: with Svadu it just saves the hardware an 
> atomic PTE update on first access. Xen doesn't consume G-stage A/D bits 
> (no dirty tracking, no demand paging), and pt.c already presets A/D 
> unconditionally for Xen's own mappings. Always setting PTE_ACCESSED | 
> PTE_DIRTY in p2m_set_permission() fixes the bug in one line, with no 
> need for the resolver, the new ISA bit, FWFT probing or the ASSERT. 
> Handling A/D differently only makes sense once Xen actually wants that 
> information, and at that point FWFT support and a fault handler are 
> needed anyway.
I agree that presetting them is the right way, it is also the way linux
is working. If Jan agree, I will to in that way in v3.

Then, it seems there is no need to register Svade/Svadu at all in
cpufeature.c, am I right?
> > ---
> >   xen/arch/riscv/cpufeature.c             | 59 
> > +++++++++++++++++++++++++++++++++
> >   xen/arch/riscv/include/asm/cpufeature.h |  1 +
> >   xen/arch/riscv/include/asm/sbi.h        |  8 +++++
> >   xen/arch/riscv/p2m.c                    | 47 ++++++++++----------------
> >   4 files changed, 86 insertions(+), 29 deletions(-)
> > 
> > diff --git a/xen/arch/riscv/cpufeature.c b/xen/arch/riscv/cpufeature.c
> > index 92235fdfd5..19454544a7 100644
> > --- a/xen/arch/riscv/cpufeature.c
> > +++ b/xen/arch/riscv/cpufeature.c
> > @@ -18,6 +18,7 @@
> >   
> >   #include <asm/cpufeature.h>
> >   #include <asm/csr.h>
> > +#include <asm/sbi.h>
> >   
> >   #ifdef CONFIG_ACPI
> >   # error "cpufeature.c functions should be updated to support ACPI"
> > @@ -468,6 +469,62 @@ static bool __init has_isa_extensions_property(void)
> >       return false;
> >   }
> >   
> > +/*
> > + * Svade and Svadu extensions represent two schemes for managing the PTE 
> > A/D
> > + * bits. When the PTE A/D bits need to be set, the Svade extension 
> > indicates
> > + * that a page fault will be raised. In contrast, the Svadu extension 
> > supports
> > + * hardware updating of the PTE A/D bits.
> > + *
> > + * There are 4 possible combinations of these extensions in the device 
> > tree.
> > + * The default hardware behavior for each is:
> > + *
> > + * 1) Neither Svade nor Svadu present in DT => It is technically unknown
> > + *    whether the platform uses Svade or Svadu. Xen should be prepared to
> > + *    handle either hardware updating of the PTE A/D bits or page faults 
> > when
> > + *    they need updating. In that case, Xen assumes Svade because it's
> > + *    harmless if the platform is actually Svadu, while assuming Svadu on 
> > real
> > + *    Svade hardware risks an unhandled page fault.
> > + *
> > + * 2) Only Svade present in DT => Xen must assume Svade to be always 
> > enabled.
> > + *
> > + * 3) Only Svadu present in DT => Xen must assume Svadu to be always 
> > enabled.
> > + *
> > + * 4) Both Svade and Svadu present in DT => Xen must assume Svadu is 
> > turned off
> > + *    at boot time by setting A/D bits. To use Svadu, the supervisor must
> > + *    explicitly enable it using the SBI FWFT extension.
> > + *
> > + * The Svade extension is mandatory and the Svadu extension is optional in 
> > the
> > + * RVA23 profile. Platforms wanting to take advantage of Svadu can choose
> > + * option 3. Platforms aware of the profile can choose option 4, and Xen 
> > won't
> > + * get the benefit of Svadu until the SBI FWFT extension is available.
> > + *
> > + * In other words, hardware manages the A/D bits on its own only in case 
> > 3, in
> > + * all the other cases software has to preset them. Instead of open coding 
> > this
> > + * in every A/D bits user, RISCV_ISA_EXT_svade is used to mean "software is
> > + * responsible for the A/D bits" and is set here for the cases 1, 2 and 4.
> > + */
> > +static void __init riscv_resolve_ad_scheme(void)
> > +{
> > +    bool svade = riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade);
> > +    bool svadu = riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svadu);
> 
> svadu is always false here: the patch doesn't add 
> RISCV_ISA_EXT_ENTRY(svadu, NONE) to riscv_isa_ext[], so match_isa_ext()
> never sets this bit. Cases 3 and 4 are dead code. Am I missing something?
> 
> > +
> > +    /* Case 3: leave the A/D bits management to hardware. */
> > +    if ( svadu && !svade )
> > +        return;
> > +
> > +    /* Case 4 */
> > +    if ( svadu && svade ){
> > +        if ( !sbi_probe_extension(SBI_EXT_FWFT) ){
> > +          printk(XENLOG_WARNING "RISC-V: Both Svade and Svadu detected, 
> > but SBI FWFT is missing.\n"
> > +                  "RISC-V: Defaulting to software A/D updates (Svade).\n"
> > +                  "RISC-V: To force hardware A/D updates (Svadu), remove 
> > 'svade' from DT.\n");
> > +        }
> > +    }
> 
> sbi_probe_extension() returns a negative errno on SBI failure, so
> !sbi_probe_extension() is false in that case and an error is treated as
> "FWFT present". The existing callers check "> 0", so this should be
> "<= 0".
> 
> > +
> > +    /* Cases 1, 2: Xen assume Svade to be enabled */
> 
> s/assume/assumes.
> 
> > +    __set_bit(RISCV_ISA_EXT_svade, riscv_isa);
> 
> In case 4 RISCV_ISA_EXT_svadu stays set, so both bits are set and the
> ASSERT() in p2m_set_permission() fires (once svadu is actually parsed).
> This contradicts the "mutually exclusive" statement there.
> 
> > +}
> > +
> >   bool riscv_isa_extension_available(const unsigned long *isa_bitmap,
> >                                      enum riscv_isa_ext_id id)
> >   {
> > @@ -513,6 +570,8 @@ void __init riscv_fill_hwcap(void)
> >           __set_bit(RISCV_ISA_EXT_sstc, riscv_isa);
> >       }
> >   
> > +    riscv_resolve_ad_scheme();
> > +
> >       for ( i = 0; i < req_extns_amount; i++ )
> >       {
> >           const struct riscv_isa_ext_data ext = required_extensions[i];
> > diff --git a/xen/arch/riscv/include/asm/cpufeature.h 
> > b/xen/arch/riscv/include/asm/cpufeature.h
> > index 0c48d57a03..74200ce7c9 100644
> > --- a/xen/arch/riscv/include/asm/cpufeature.h
> > +++ b/xen/arch/riscv/include/asm/cpufeature.h
> > @@ -41,6 +41,7 @@ enum riscv_isa_ext_id {
> >       RISCV_ISA_EXT_sstc,
> >       RISCV_ISA_EXT_svade,
> >       RISCV_ISA_EXT_svpbmt,
> > +    RISCV_ISA_EXT_svadu,
> 
> Please keep the same order as riscv_isa_ext[], i.e. between svade and
> svpbmt, and add the matching riscv_isa_ext[] entry there as well.
> 
> >       RISCV_ISA_EXT_MAX
> >   };
> >   
> > diff --git a/xen/arch/riscv/include/asm/sbi.h 
> > b/xen/arch/riscv/include/asm/sbi.h
> > index 1952868e96..4f13e8c7a0 100644
> > --- a/xen/arch/riscv/include/asm/sbi.h
> > +++ b/xen/arch/riscv/include/asm/sbi.h
> > @@ -30,6 +30,7 @@
> >   #define SBI_EXT_BASE                    0x10
> >   #define SBI_EXT_RFENCE                  0x52464E43
> >   #define SBI_EXT_TIME                    0x54494D45
> > +#define SBI_EXT_FWFT                    0x46574654
> >   
> >   /* SBI function IDs for BASE extension */
> >   #define SBI_EXT_BASE_GET_SPEC_VERSION   0x0
> > @@ -138,6 +139,13 @@ int sbi_remote_hfence_gvma(const cpumask_t *cpu_mask, 
> > vaddr_t start,
> >   int sbi_remote_hfence_gvma_vmid(const cpumask_t *cpu_mask, vaddr_t start,
> >                                   size_t size, unsigned long vmid);
> >   
> > +/**
> > + * Check if an SBI extension ID is supported or not.
> > + * @extid: The extension ID to be probed.
> > + *
> > + * @return: 1 or an extension specific nonzero value if yes, 0 otherwise.
> > + */
> 
> This is incorrect: on failure the function returns a negative errno, not
> 0. Also, the rest of the file uses /* */ and not kernel-doc /**.
> 
> > +int sbi_probe_extension(long extid);
> 
> A blank line is missing before the next comment block.
> 
> >   /*
> >    * Initialize SBI library
> >    *
> > diff --git a/xen/arch/riscv/p2m.c b/xen/arch/riscv/p2m.c
> > index 1cea86512c..22ad4a2aee 100644
> > --- a/xen/arch/riscv/p2m.c
> > +++ b/xen/arch/riscv/p2m.c
> > @@ -586,42 +586,31 @@ static inline void p2m_clean_pte(pte_t *p, bool 
> > clean_cache)
> >   
> >   static void p2m_set_permission(pte_t *e, p2m_type_t t)
> >   {
> > +    bool svade = riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade);
> > +    bool svadu = riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svadu);
> 
> This runs for every p2m PTE. The second test_bit() exists only for the
> ASSERT().
> 
> > +
> >       e->pte &= ~PTE_ACCESS_MASK;
> >   
> >       e->pte |= PTE_USER;
> >   
> >       /*
> > -     * Two schemes to manage the A and D bits are defined:
> > -     *   • The Svade extension: when a virtual page is accessed and the A 
> > bit
> > -     *     is clear, or is written and the D bit is clear, a page-fault
> > -     *     exception is raised.
> > -     *   • When the Svade extension is not implemented, the following 
> > scheme
> > -     *     applies.
> > -     *     When a virtual page is accessed and the A bit is clear, the PTE 
> > is
> > -     *     updated to set the A bit. When the virtual page is written and 
> > the
> > -     *     D bit is clear, the PTE is updated to set the D bit. When 
> > G-stage
> > -     *     address translation is in use and is not Bare, the G-stage 
> > virtual
> > -     *     pages may be accessed or written by implicit accesses to 
> > VS-level
> > -     *     memory management data structures, such as page tables.
> > -     * Thereby to avoid a page-fault in case of Svade is available, it is
> > -     * necessary to set A and D bits.
> > -     *
> > -     * TODO: For now, it’s fine to simply set the A/D bits, since OpenSBI
> > -     *       delegates page faults to a lower privilege mode and so OpenSBI
> > -     *       isn't expect to handle page-faults occured in lower modes.
> > -     *       By setting the A/D bits here, page faults that would otherwise
> > -     *       be generated due to unset A/D bits will not occur in Xen.
> > -     *
> > -     *       Currently, Xen on RISC-V does not make use of the information
> > -     *       that could be obtained from handling such page faults, which
> > -     *       could otherwise be useful for several use cases such as demand
> > -     *       paging, cache-flushing optimizations, memory access 
> > tracking,etc.
> > +     * riscv_fill_hwcap() sets either RISCV_ISA_EXT_svade or
> > +     * RISCV_ISA_EXT_svadu (mutually exclusive) depending on the 
> > Svade/Svadu
> > +     * device tree combination (see riscv_resolve_ad_scheme()):
> 
> This isn't true, see the comment on riscv_resolve_ad_scheme(): in case 4 
> both bits end up set.
> 
> > +     * - RISCV_ISA_EXT_svade means that software is responsible for the A/D
> > +     *   bits.
> > +     * - RISCV_ISA_EXT_svadu means the hardware is responsible for the A/D
> > +     *   bits.
> >        *
> > -     *       To support the more general case and the optimizations 
> > mentioned
> > -     *       above, it would be better to stop setting the A/D bits here 
> > and
> > -     *       instead handle page faults that occur due to unset A/D bits.
> > +     * Currently, when RISCV_ISA_EXT_svade is set, Xen doesn't track A/D
> > +     * bits, so it does not make use of the information that could be
> > +     * obtained from handling the resulting page faults, which could
> > +     * otherwise be useful for several use cases such as demand paging,
> > +     * cache-flushing optimizations, memory access tracking, etc. To avoid
> > +     * such a page fault, Xen presets the A and D bits instead.
> >        */
> > -    if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) )
> > +    ASSERT(svade != svadu); /* exactly one of svade/svadu must be set by 
> > riscv_fill_hwcap() */
> 
> The line is over 80 columns, and the trailing comment just repeats the 
> block comment above.
> 
> > +    if ( svade )
> >           e->pte |= PTE_ACCESSED | PTE_DIRTY;
> >   
> >       switch ( t )
> > 
> 
> ~ Oleksii
> 
> 
>