Re: [PATCH v6 1/4] xen/riscv: probe the PTE A/D bits update scheme at boot

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 10/14/26 11:09 AM, Jan Beulich wrote:
> On 14.10.2026 11:00, Baptiste Le Duc wrote:
> > > > > > +        "2:\n"
> > > > > > +        ASM_EXTABLE(1b, 2b)
> > > > > > +        : [tmp] "=&r" (tmp), [updated] "+r" (updated)
> > > > > > +        : [va] "r" (va)
> > > > > > +        : "memory" );
> > > > > > +
> > > > > > +    arch_pmap_unmap(slot);
> > > > > > +    free_domheap_page(pg);
> > > > > > +
> > > > > > +    return updated;
> > > > > > +}
> > > > > Imo you'd better check that A also was actually set in the PTE.
> > > > If I check that A was set, can I drop the "updated" variable? The A bit
> > > > would tell us more than whether the load succeeded.
> > > Hmm, I'm inclined to suggest to check both.
> > Then I'd rather probe D than A, since D, unlike A, cannot be set
> > speculatively (spec: "updates to the D bit, resulting from an explicit store,
> > must be exact (i.e., non-speculative)"). The result would
> > then be:
> >
> >        return updated && d_set;
> >
> > with a warning (or panic()?) on updated && !d_set, which the spec
> > doesn't allow (spec: "the access must not be performed before the PTE update is
> > globally visible").
> >
> > Furthermore, if I understood your earlier suggestion correctly, you'd
> > like the A/D bits to be preset in p2m_set_pte_flags() unless Svadu is
> > in use:
> >
> >      if ( !riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svadu) )
> >          e->pte |= PTE_ACCESSED | PTE_DIRTY;
> >
> > However, when both Svade and Svadu are advertised, Svade is in effect
> > until Svadu is enabled via FWFT (perfectly legit), so this condition
> > would skip the preset and the first access would fault.
> In which case it would need to be
>
>      if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) ||
>           !riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svadu) )
>          e->pte |= PTE_ACCESSED | PTE_DIRTY;
>
> albeit I have to admit that permitting two contradicting extensions to
> be exposed at the same time is irritating.
>
> > I see two ways to address this:
> >
> > 1) Keep checking Svade:
> >
> >      if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) )
> >          e->pte |= PTE_ACCESSED | PTE_DIRTY;
> >
> >      After resolve_ad_scheme(), the Svade bit is set in every case where
> >      software has to preset the A/D bits. The only imprecise case is both
> >      extensions advertised with hardware updating already enabled by
> >      firmware, where presetting A/D is unnecessary but harmless.
> >
> > 2) Record the probe result in a global, e.g.
> >
> >      static bool __ro_after_init hw_ad_update;
> >
> >      and use:
> >
> >      if ( !hw_ad_update )
> >          e->pte |= PTE_ACCESSED | PTE_DIRTY;
> >
> >      This is exact in all cases, at the cost of a new global that mostly
> >      duplicates what resolve_ad_scheme() already records in the ISA bitmap.
> >
> > I discussed with Oleksii and we are fine with either. What do you prefer?
> As to 1), see above. For 2), while maybe okay, I think we're better off
> not deriving too much from probing. Whatever can be (reliably) derived from
> what the platform description says should be preferred imo.
Option 1 looks okay to me.

Also, it seems then we could simplify resolve_ad_scheme() to:

/*
 * p2m_set_pte_flags() presets the A/D bits unless only Svadu is 
advertised,
 * as per the DT binding that is the only case in which hardware 
updating is
 * guaranteed to be enabled (by firmware, via menvcfg.ADUE). Check that it
 * really is, and fall back to presetting A/D otherwise: a hart which 
faults
 * on clear A/D bits implements Svade, whether or not it says so.
 */
static void __init resolve_ad_scheme(void)
{
    if ( !riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svadu) ||
         riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) )
        return;

    if ( !probe_hw_ad_update() )
    {
        printk(XENLOG_WARNING
               "Svadu is advertised but A/D bits aren't updated by 
hardware, presetting them\n");
        __set_bit(RISCV_ISA_EXT_svade, riscv_isa);
    }
}

~ Oleksii