Re: [PATCH v5 1/3] xen/riscv: always preset A/D bits in G-stage PTEs

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 10/6/26 5:20 PM, Jan Beulich wrote:
> On 05.10.2026 18:00, Baptiste Le Duc wrote:
> > A RISC-V implementation can manage the PTE A/D bits in one of two ways:
> >      1) Update the 'A' and 'D' PTE bits in hardware (ratified as Svadu).
> >      2) Generate a page fault when 'A' and/or 'D' is clear, so that software
> >         can set them (ratified as Svade).
> >
> > p2m_set_pte_flags() presets A/D in G-stage PTEs only when the device tree
> > advertises Svade.
According to the code it seems like it is setting A/D bits always 
independently on what is DT file.  Baptiste, am I missing something?
 Platforms that use scheme (2) without advertising Svade,
> > such as the HiFive Premier P550, then hit guest-page faults that Xen does
> > not handle.
> Is that a quirk then (to avoid the word "bug")? Since it's all DT that
> conveys available ISA information, why can't the DT for those systems
> simply be fixed? Depending on the answer here I might question the
> presence of the Fixes: tag below.
I think it can at some point. Let me clarify why "at some point"...

So how the things are working now:

OpenSBI detects the available ISA extensions in two steps:
1. It parses the CPU nodes in the DTB. If a node has the 
riscv,isa-extensions property, OpenSBI uses it; otherwise it falls back 
to riscv,isa. riscv,isa-extensions describes the same extensions as 
riscv,isa, but as a list of strings, which makes it easier to parse. 
From this, OpenSBI builds a per-hart bitmap of available extensions 
(basically what we do in Xen). (Platform-specific code can also add 
extensions to the bitmap at this step.)
2. It then probes a small, fixed set of extensions (Sstc, Smaia, 
Sscofpmf, Smstateen, Ssstateen, Smcntrpmf, Sdtrig) by reading a CSR that 
the extension introduces and checking whether the access traps. This is 
similar to what we do for Sstc in Xen.
Step 2 only adds extensions to the bitmap. It does not check or clear 
the extensions that came from the DT; the only exception is Zicntr, 
which is detected purely by probing. So for extensions such as 
Svade/Svadu, which have no CSR to probe, the DT is the only source of truth.
Now, what OpenSBI does with Svade/Svadu specifically. First, it disables 
hardware A/D updating by default, i.e. it clears menvcfg.ADUE:
Now, what OpenSBI does with Svade/Svadu specifically. First, it disables 
hardware A/D updating by default, i.e. it clears menvcfg.ADUE:
  /* Disable HW A/D updating by default */
  menvcfg_val &= ~ENVCFG_ADUE;

Then it sets ADUE again, depending on which extensions are listed in the 
DTB:
  /*
   * Assume only Svadu is supported when it is the only extension
   * present in the ISA string. Svade is assumed when neither are
   * present. When both are present we must default to Svade (see
   * the zero reset value of FWFT.PTE_AD_HW_UPDATING).
   */
  if (!sbi_hart_has_extension(scratch, SBI_HART_EXT_SVADE))
        __set_menvcfg_ext(SBI_HART_EXT_SVADU, ENVCFG_ADUE);

So all OpenSBI does is set or clear menvcfg.ADUE based on what is 
mentined in DTB.
So, to answer your original question: yes, this can be fixed just by 
updating the DTS. OpenSBI will then configure menvcfg.ADUE according to 
the extensions listed there. Note that the logic above is from OpenSBI 
v1.9. Versions v1.5–v1.8 behave the same when the DT lists only svadu, 
only svade, or both. When neither is listed, they leave ADUE unchanged 
instead of clearing it. Versions before v1.5 don't handle Svade/Svadu at 
all.
But IIRC such approach was suggested in one of vX and it was rejected 
because we can't in Xen be sure that passed DTB is correct and 
corresponds to how real h/w is configured as specifically with ADUE bit 
in menvcfg we can't read it in HS-mode as an exception will occur as it 
is M-mode register. So we have to trust that OpenSBI (or other firmware 
will configure the A/D update scheme according to what is mentioned in DTS).
So then basically the question is do we want to rely on that how OpenSBI 
configures h/w?
It seems like it could be fine. For example, without support for the 
FWFT extension, we can't access or change the menvcfg bit anyway. The 
FWFT extension was ratified in 2025, so we have to trust OpenSBI anyway 
(or introduce our own extension or port the FWFT extension to an earlier 
OpenSBI version) before FWFT is started to be available.
With this explanation would you (Jan) be okay to go with just providing 
proper DTS?
> > Always preset A/D in G-stage PTEs so scheme (2) never faults. This is
> > harmless with (1): software just does what hardware would have done on
> > first access.
> >
> > Drop the TODO suggesting to handle A/D faults: only Svade raises them, so
> This contradicts the earlier paragraph, where you say that page faults can
> also occur without Svade.
>
> > it would not work on Svadu. Dirty/access tracking can restrict the RWX
> > permissions of p2m entries, as x86 and Arm do.
> I'm also not convinced that this is what the TODO was after. Oleksii would
> be able to clarify, I suppose.
I think the TODO below in the code should be added back, as it indicates 
that something needs to be changed if (or when) we decide to support 
tracking of the A/D bits.
~ Oleksii

> > 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>
> >
> > --- a/xen/arch/riscv/p2m.c
> > +++ b/xen/arch/riscv/p2m.c
> > @@ -591,38 +591,16 @@ static void p2m_set_pte_flags(pte_t *e, p2m_type_t t)
> >       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.
> > +     * A RISC-V implementation can either:
> > +     * 1) Update the 'A' and 'D' PTE bits in hardware.
> > +     * 2) Generate a page fault when 'A' and/or 'D' is clear, so that
> > +     *    software can set them.
> >        *
> > -     * 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.
> > -     *
> > -     *       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.
> > +     * Xen supports both, so set 'A' and 'D' up front to avoid the faults
> > +     * of (2). This is harmless with (1): software just does what hardware
> > +     * would have done on first access.
> "Xen supports both" to me means it suitably handles the page faults resulting
> with Svade. May I suggest "To support both, set ..."?
>
> >        */
> > -    if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) )
> > -        e->pte |= PTE_ACCESSED | PTE_DIRTY;
> > +    e->pte |= PTE_ACCESSED | PTE_DIRTY;
> >   
> >       switch ( t )
> >       {