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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 29.09.2026 18:32, Baptiste Le Duc wrote:
> @@ -500,6 +502,45 @@ static void __init riscv_fill_hwcap_from_isa_string(void)
>      }
>  }
>  
> +/*
> + * Svade and Svadu extensions represent two schemes for managing the PTE A/D
> + * bits. When the PTE A/D bits need to be set, the Svade extension indicates
> + * that a page fault will be raised. In contrast, the Svadu extension 
> supports
> + * hardware updating of the PTE A/D bits.
> + *
> + * There are 4 possible combinations of these extensions in the device tree.
> + * The default hardware behavior for each is:
> + *
> + * 1) Neither Svade nor Svadu present in DT => It is technically unknown
> + *    whether the platform uses Svade or Svadu. Xen should be prepared to
> + *    handle either hardware updating of the PTE A/D bits or page faults when
> + *    they need updating.
> + *
> + * 2) Only Svade present in DT => Xen must assume Svade to be always enabled.
> + *
> + * 3) Only Svadu present in DT => Xen must assume Svadu to be always enabled.
> + *
> + * 4) Both Svade and Svadu present in DT => Xen must assume Svadu is turned 
> off
> + *    at boot time, so it presets the A/D bits. To use Svadu, the supervisor
> + *    must explicitly enable it using the SBI FWFT extension.
> + *
> + * The Svade extension is mandatory and the Svadu extension is optional in 
> the
> + * RVA23 profile. Platforms wanting to take advantage of Svadu can choose
> + * option 3. Platforms aware of the profile can choose option 4, and Xen 
> won't
> + * get the benefit of Svadu until the SBI FWFT extension is available.
> + */
> +static void __init riscv_resolve_ad_scheme(void)
> +{
> +    bool svade = riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade);
> +    bool svadu = riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svadu);
> +
> +    if ( svadu && svade )
> +        if ( sbi_probe_extension(SBI_EXT_FWFT) <= 0 )

Such successive if()s want folding.

> +            printk(XENLOG_WARNING "RISC-V: Both Svade and Svadu detected, 
> but SBI FWFT is missing.\n"
> +                XENLOG_WARNING "RISC-V: Defaulting to software A/D updates 
> (Svade).\n"
> +                XENLOG_WARNING "RISC-V: To force hardware A/D updates 
> (Svadu), remove 'svade' from DT.\n");

Nit: Indentation, and no full stop at the end of log messages please.

Overall - what use is this? Nothing will be logged if FWFT is present, yet
there's no real use of the extension. I.e. even in that case you default
to svade. Further, removing svadu from DT doesn't alter hardware behavior.
How can that be a useful suggestion?

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

Jan