Re: [PATCH v3 3/6] xen/riscv: fix A/D bit handling in G-stage mappings

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 9/30/26 3:58 PM, Jan Beulich wrote:
> On 30.09.2026 15:53, Oleksii Kurochko wrote:
> > On 9/30/26 2:20 PM, Jan Beulich wrote:
> > > On 29.09.2026 18:32, Baptiste Le Duc wrote:
> > > > --- a/xen/arch/riscv/p2m.c
> > > > +++ b/xen/arch/riscv/p2m.c
> > > > @@ -586,42 +586,24 @@ static inline void p2m_clean_pte(pte_t *p, bool 
> > > > clean_cache)
> > > >    
> > > >    static void p2m_set_pte_flags(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);
> > > > +
> > > >        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.
> > > > +     * A RISC-V implementation can choose to either:
> > > > +     * 1) Update 'A' and 'D' PTE bits in hardware.
> > > > +     * 2) Generate page fault when 'A' and/or 'D' PTE bits are not set so that
> > > > +     *    software can update these 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.
> > > > +     * Xen supports both options mentioned above: unless the platform 
> > > > guarantees
> > > > +     * (1), i.e. only Svadu is present, set 'A' and 'D' so that (2) never
> > > > +     * faults.
> > > >         */
> > > > -    if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) )
> > > > +    if ( !svadu || svade )
> > > >            e->pte |= PTE_ACCESSED | PTE_DIRTY;
> > > I may have asked this already when the original conditional was introduced:
> > > What use is it to leave A and D clear, when we don't otherwise consume the
> > > bits? This way hardware has to issue more (atomic) writes, i.e. performance
> > > suffers for no gain.
> > It will be done only once, won't it? After that, if the A/D bits aren't
> > cleared, there shouldn't be any performance impact, so it will basically
> > behave the same as setting the A/D bits in software.
> >
> > The idea was that, if Svadu is available, it should be the hardware's
> > job to set the A/D bits. That way, when the A/D bits eventually start
> > being used for some purpose in Xen, we won't miss an unconditional write
> > of the A/D bits when Svadu is available.
> >
> > If you think it's enough to just have the A/D bits set unconditionally,
> > I'm okay with that, and we can go that way.
> I think starting simple (i.e. unconditional) here is the way to go. Making
> the setting of one or both flags conditional can be left to whenever that
> would become a necessity. Note that on x86 we haven't seen a need in all
> the time (for Xen's own page tables that is).
Okay, then lets set them unconditionally for now.

~ Oleksii