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

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

On 2026-10-13 12:36 +0200, Jan Beulich wrote:
> On 13.10.2026 12:07, Baptiste Le Duc wrote:
> > @@ -620,6 +623,98 @@ static void __init compute_guest_isa(void)
> >          panic("Failed to build guest \"riscv,isa\" string\n");
> >  }
> >  
> > +static bool __init probe_hw_ad_update(void)
> > +{
> > +    bool updated = false;
> > +    unsigned int slot = FIX_MISC;
> > +    vaddr_t va = FIXMAP_ADDR(slot);
> > +    struct page_info *pg;
> > +    pte_t *entry, pte;
> > +    mfn_t mfn;
> > +    unsigned long tmp;
> > +
> > +    pg = alloc_domheap_page(NULL, 0);
> > +    if ( !pg )
> > +        return false;
> 
> Why would you need a fresh page here? Can't you create a secondary mapping
> of a page already in use (e.g. one from Xen's .data or .bss section, or the
> stack page you're running on)?
You're right, I will map the current stack page.
> 
> > +    mfn = page_to_mfn(pg);
> > +
> > +    arch_pmap_map(FIX_MISC, mfn);
> > +    entry = &xen_fixmap[slot];
> > +    pte = pte_from_mfn(mfn, PTE_VALID | PTE_READABLE);
> > +    write_pte(entry, pte);
> > +
> > +    flush_tlb_one_local(va);
> > +
> > +    asm_inline volatile (
> > +        "1: ld %[tmp], 0(%[va])\n"
> > +        "   li %[updated], 1\n"
> 
> If you mapped the stack page, you could preset a variable there to 1, load
> from the alias mapping of that variable, and then avoid the LI altogether.
Yes mapping the stack page appears to be the best way.
> 
> > +        "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.
Right, I will do that.
> 
> > +
> > +    /* Neither advertised: trust the probe. */
> > +    __set_bit(has_hw_update ? RISCV_ISA_EXT_svadu : RISCV_ISA_EXT_svade,
> > +              riscv_isa);
> 
> Hmm, this feels fragile. Other schemes are possible, e.g. A being updated
> by hardware but D being left to software. Are such truly precluded by the
> spec, including older versions thereof? Furthermore, probe_hw_ad_update()
> can (right now) return false for unrelated reasons.
Since 1.10 the spec lists exactly two schemes, and each one covers A and
D together: either a clear A (or a clear D on a store) raises a page
fault, or hardware sets the corresponding bit(s). A-bit being set by
hardware while D is left to software isn't one of them. Before 1.10, the
hardware must set the two bits. Therefore, there is not other schemes
possible.
> As a consequence, patch 2 imo also can't leave the A/D pre-setting unaltered.
> It perhaps needs doing unless Svadu is positively identified as present.
I agree it is more robust to preset A/D bits unless Svadu is truly
identified, i.e. advertised in the DTS (without Svade) and confirmed by
probe_hw_ad_update(). The worst case then becomes the probe failing for
an unrelated reason on hardware which does update A/D: we'd preset A/D
needlessly, which is harmless, as hardware only writes the PTE when a
bit is clear.

To sum up, and to avoid unnecessary respins, would you be fine with:
1) Keeping the probe as it is, with your remarks addressed (map stack page).
2) Presetting A/D bits unless Svadu is truly identified as above.
> 
> > --- a/xen/arch/riscv/include/asm/cpufeature.h
> > +++ b/xen/arch/riscv/include/asm/cpufeature.h
> > @@ -42,6 +42,7 @@ enum riscv_isa_ext_id {
> >      RISCV_ISA_EXT_sstc,
> >      RISCV_ISA_EXT_svade,
> >      RISCV_ISA_EXT_svpbmt,
> > +    RISCV_ISA_EXT_svadu,
> 
> Isn't there some sort of sorting in effect here?
No, as it is mentioned in the docstring that:
    * New extensions should just be added to the bottom, rather than added
    * alphabetically, in order to avoid unnecessary shuffling.

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