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 18:12 +0200, Oleksii Kurochko wrote:
> 
> 
> On 8/28/26 3:58 PM, Baptiste Le Duc wrote:
> > 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.
> 
> Yes, that is exactly correct. Without SBI FWFT support, Xen cannot 
> toggle menvcfg.ADUE in Case 4. Thus, the only viable workaround to use 
> Svadu is to remove 'svade' from the riscv,isa DT property (Case 3), 
> which prompts OpenSBI to enable ADUE=1 at boot time.
> 
> I agree document that somewhere will make this behavior/intention clear!
> 
> Not insisting on that:
> I also think it would be a good idea to add an early printk() warning in 
> the detection logic when both Svade and Svadu are present but SBI FWFT 
> is missing, guiding users to drop 'svade' from their DT if they want to 
> leverage Svadu. Something like:
> 
> if ( svade && svadu )
> {
>      /* Assuming sbi_fwft_is_supported() or similar probe is available */
>      if ( !sbi_probe_extension(SBI_EXT_FWFT) )
>      {
>          printk(XENLOG_WARNING
>                 "RISC-V: Both Svade and Svadu detected, but SBI FWFT is 
> missing.\n"
>                 "RISC-V: Defaulting to software A/D updates (Svade).\n"
>                 "RISC-V: To force hardware A/D updates (Svadu), remove 
> 'svade' from DT.\n");
>      }
> }
> 
> somewhere in the function (riscv_resolve_ad_scheme) I suggested above.

Agree, it'll be better. I'm going to apply that in v2
> 
> >>
> >> [1]
> >> https://github.com/riscv-software-src/opensbi/blob/master/lib/sbi/sbi_hart.c#L171
> >>
> > Thanks for this very clear review.
> 
> Welcome.
> 
> ~ Oleksii
> 
> 
>