Re: [PATCH 1/5] xen/riscv: always set A/D bits at boot time

[Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>]

On 2026-08-28 12:59 +0200, Oleksii Kurochko wrote:
> 
> 
> On 8/27/26 5:33 PM, Baptiste Le Duc wrote:
> > Always set the PTE A/D bits at boot time to avoid an unhandled page fault
> > on platforms that implement neither Svade nor Svadu, and on platforms that
> > declare both in the device tree.
> > 
> > Rewrite the comment to enumerate the four possible Svade/Svadu combinations
> > (inspired by [1]) and set A/D unconditionally, which is correct in all four
> > cases until Svadu is fully supported (full support requires the SBI FWFT
> > call to enable hardware updating of A/D bits).
> > 
> > [1] https://lwn.net/Articles/980016/
> > 
> > Assisted-by: Claude:claude-opus-5
> > Signed-off-by: Baptiste Le Duc <baptiste.le-duc@xxxxxxxxxx>
> > ---
> >   xen/arch/riscv/p2m.c | 70 ++++++++++++++++++++++++++------------------
> >   1 file changed, 42 insertions(+), 28 deletions(-)
> > 
> > diff --git a/xen/arch/riscv/p2m.c b/xen/arch/riscv/p2m.c
> > index 1cea86512c..11dc289f0f 100644
> > --- a/xen/arch/riscv/p2m.c
> > +++ b/xen/arch/riscv/p2m.c
> > @@ -591,38 +591,52 @@ static void p2m_set_permission(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.
> > +     * 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.
> >        *
> > -     * 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.
> > +     * There are 4 possible combinations of these extensions in the device
> > +     * tree. The default hardware behavior for each is:
> >        *
> > -     *       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.
> > +     * 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. To support both, Xen always sets the 
> > 'A' and
> > +     *    'D' PTE bits at boot time.
> >        *
> > -     *       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.
> > +     * 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 by setting 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
> > +     * Linux won't get the benefit of Svadu until the SBI FWFT extension is
> > +     * available.
> 
> I have a feeling that the DT-binding-related comment should not be 
> present here, as it explains when Svadu or Svade should be considered 
> enabled or disabled. We should perform this kind of detection in 
> riscv_fill_hwcap() [cpufeature.c]. Then, in p2m_set_permission(), we 
> should use riscv_isa_extension_available() to determine which extension 
> is available and, based on that, set the A and D bits.
> 
> At this point, I think the original comment was better, as it simply 
> explained what Svade and Svadu are and, therefore, provided a better 
> explanation of why the A and D bits should or should not be set.
> 
> So, my suggestion is the following:
> 
> +/*
> + * 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. To support both, Xen always sets the 'A' and 
> 'D' PTE
> + *    bits at boot time.
> + *
> + * 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 by setting 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.
> + *
> + * In other words, hardware manages the A/D bits on its own only in case 3;
> + * in all the other cases software has to preset them. Instead of open 
> coding
> + * this in every A/D bits user, RISCV_ISA_EXT_svade is used to mean 
> "software
> + * is responsible for the A/D bits" and is set here for the cases 1, 2 
> and 4.
> + */
> +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);
> +
> +    /* Case 3: leave the A/D bits management to hardware. */
> +    if ( svadu && !svade )
> +        return;
> +
> +    /* Cases 1, 2 and 4: Xen has to preset the A/D bits. */
> +    __set_bit(RISCV_ISA_EXT_svade, riscv_isa);
> +}
> +
>   void __init riscv_fill_hwcap(void)
>   {
>       unsigned int i;
> @@ -513,6 +560,8 @@ void __init riscv_fill_hwcap(void)
>           __set_bit(RISCV_ISA_EXT_sstc, riscv_isa);
>       }
> 
> +    riscv_resolve_ad_scheme();
> +
> 
> And then ...
> 
> 
> > +     *
> > +     * 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.
> > +     */
> > +
> > +    /*
> > +     * Preset unconditionally for all 4 cases above, harmless when Svadu
> > +     * manages the bits (case 3). Skipping it for case 3 requires SBI FWFT
> > +     * which is not yet supported.
> >        */
> > -    if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) )
> > -        e->pte |= PTE_ACCESSED | PTE_DIRTY;
> > +    e->pte |= PTE_ACCESSED | PTE_DIRTY;
> 
> ... we could restore the check and the comment we originally had in 

Yes it makes sense as we now manually force the svade extension in 1, 2
and 4 cases.

> p2m_set_permission(), but probably with some updates, something along 
> the following lines:
> 
> /*
>   * Xen has to preset the A/D bits unless the hardware is known to update
>   * them on its own. riscv_fill_hwcap() folds all the Svade/Svadu device
>   * tree combinations into RISCV_ISA_EXT_svade, which then means that
>   * software is responsible for the A/D bits" (see
>   * riscv_resolve_ad_scheme()).
>   */
> 
> I have another comment regarding:
> 
>  > +    /*
>  > +     * Preset unconditionally for all 4 cases above, harmless when Svadu
>  > +     * manages the bits (case 3). Skipping it for case 3 requires 
> SBI FWFT
>  > +     * which is not yet supported.
>  >        */
>  > -    if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) )
>  > -        e->pte |= PTE_ACCESSED | PTE_DIRTY;
>  > +    e->pte |= PTE_ACCESSED | PTE_DIRTY;
> 
> I am not sure that this comment is correct. In case 3, we should not 
> need to use the SBI FWFT extension. Case 3 means that Xen must assume 
> that Svadu is enabled. Therefore, it is the responsibility of OpenSBI, 
> or the pre-bootloader that loads OpenSBI, to enable it. If it fails to 
> do so, then OpenSBI or the pre-bootloader is not complying with the DT 
> binding documentation and it should be fixed in first place.

You right, thanks
> 
> As further evidence, this is what OpenSBI already does [1]:
> /*
>   * Assume only Svadu is supported when it is the only extension
>   * present in the ISA string. Svade is assumed when neither are
>   * present. When both are present we must default to Svade (see
>   * the zero reset value of FWFT.PTE_AD_HW_UPDATING).
>   */
> if (!sbi_hart_has_extension(scratch, SBI_HART_EXT_SVADE))
>      __set_menvcfg_ext(SBI_HART_EXT_SVADU, ENVCFG_ADUE);
> 
> Therefore, in case 3, the original check is still valid, and there is no 
> need for Xen to support the SBI FWFT extension for this case. I think 
> the original check should therefore be kept as it was:

Yes agree, I'll change that in v2.

> if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) )
>      e->pte |= PTE_ACCESSED | PTE_DIRTY;
> 
> The SBI FWFT extension is only required for case 4. If both Svade and 
> Svadu are present in the DT, Svade is selected by default. To use Svadu 
> instead, SBI FWFT is required to set the ADUE bit in menvcfg, which is 
> only accessible from M-mode.
> 
> Since SBI FWFT is relatively new and may not be supported by older 
> OpenSBI versions, another option is to have OpenSBI hard-code ADUE=1. 
> Alternatively, the DTS could specify only one of Svade or Svadu in the 
> riscv,isa property. In that case, upstream OpenSBI can handle the 
> configuration automatically. So specifically for our case (Svadu and 
> Svade things) we don't need SBI FWFT at all.

So if I understood correclty, you want to not let the option to change
ADUE bits in case 4 right? Therefore, I think we should document that
somewhere to clearly indicates that if someone want to use Svadu, he
should remove `svade` in the riscv,isa DT property.
> 
> [1] 
> https://github.com/riscv-software-src/opensbi/blob/master/lib/sbi/sbi_hart.c#L171
> 
Thanks for this very clear review.

> ~ Oleksii
> 
> 
> 
> 
>