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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

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

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