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

[Teddy Astie <teddy.astie@xxxxxxxxxx>]

Le 05/10/2026 à 18:02, Baptiste Le Duc a écrit :
> 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.
I second what Jan said (this is likely a firmware bug, given that the 
enumerated behavior is not consistent with the specification, as 
existence of Svadu/Svade alters observable behavior).
> 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
> it would not work on Svadu. Dirty/access tracking can restrict the RWX
> permissions of p2m entries, as x86 and Arm do.
Yet, existence of Svade alone doesn't mandate the actual behavior, it is 
also dependant on the value of ADUE bit in configuration registers.
Ideally (in cases we can) we would want to configure hardware to follow 
a consistent scheme (with the one that updates A/D with hardware i.e 
ADUE=1, as it's consistent to without Svade) and only consider the 
ADUE=0 behavior for specific needs like dirty tracking.
> > 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 v4:
> - restore svade and update the commit message accordingly.
> - rebase on top of staging.
> ---
> Changes since v3:
> - always preset A/D bits in G-stage mappings instead of keying off the DT:
>    it may not reflect what the hardware actually does.
> - change commit title/message.
> - drop svade as it is now-unused.
> ---
> Changes since v2:
> - change commit title.
> - preset A/D bits in p2m_set_pte_flags() if (!svadu || svade), instead of
> setting RISCV_ISA_EXT_svade in the riscv_isa bitmap for cases 1, 2 and 4.
> - riscv_resolve_ad_scheme() now only warns when both Svade and Svadu are
> present and SBI FWFT is missing; drop the dead 'svadu && svade' operand.
> - repeat XENLOG_WARNING on each line of the warning.
> - drop ASSERT(svade != svadu) from p2m_set_pte_flags().
> - only add the sbi_probe_extension() declaration to sbi.h.
> - rework the comment in p2m_set_pte_flags().
> ---
> 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.
> - stop presetting A/D bits unconditionally in p2m_set_permission(), do it
>    only when Svade is present.
> ---
>   xen/arch/riscv/p2m.c | 38 ++++++++------------------------------
>   1 file changed, 8 insertions(+), 30 deletions(-)
>
> diff --git a/xen/arch/riscv/p2m.c b/xen/arch/riscv/p2m.c
> index f7b380b90a..14cae7dcd7 100644
> --- 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.
>        */
> -    if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) )
> -        e->pte |= PTE_ACCESSED | PTE_DIRTY;
> +    e->pte |= PTE_ACCESSED | PTE_DIRTY;
On the code change itself, it looks good to me, as we don't make use of 
A/D bit anyway (and in case we would need to, we would need to clear 
them anyway).
>   
>       switch ( t )
>       {

Attachment: OpenPGP_signature.asc
Description: OpenPGP digital signature