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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

Jan