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

[Jan Beulich <jbeulich@xxxxxxxx>]

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)?

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

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

> +/*
> + * The privileged spec allows exactly two schemes for the PTE A and D bits:
> + *  1) hardware sets them on access and on write.
> + *  2) accessing a page whose A and/or D bit is clear raises a page fault,
> + *     so that software can set them (ratified as Svade).
> + *
> + * Hence an implementation without Svade uses the hardware scheme (1), and 
> one
> + * that faults on clear A/D bits implements Svade, whether or not it says so.
> + *
> + * Svadu isn't itself a scheme: it adds menvcfg.ADUE, selecting between the
> + * two for S-mode and G-stage translation, and henvcfg.ADUE, doing the same
> + * for VS-stage translation (read-only zero when menvcfg.ADUE is zero).
> + *
> + * Rather than trusting the DTS alone, probe the scheme in effect with a read
> + * through a PTE whose A bit is clear, and check it against what the DTS
> + * advertises, as described by the DT binding. Panic on a mismatch Xen cannot
> + * run with, and record the probed scheme when the DTS advertises neither.
> + */
> +static void __init 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);
> +    bool has_hw_update = probe_hw_ad_update();
> +
> +    /* Both advertised: Svade applies until FWFT enables hardware updating. 
> */
> +    if ( svade && svadu )
> +    {
> +        if ( !has_hw_update )
> +            printk(XENLOG_WARNING
> +                   "Svade and Svadu are both advertised, but software 
> updates A/D bits. "
> +                   "To use hardware updating, drop Svade from the DTS or use 
> FWFT (not implemented yet)\n");
> +        return;
> +    }
> +
> +    /* Only Svadu advertised: firmware must have enabled hardware updating. 
> */
> +    if ( svadu )
> +    {
> +        if ( !has_hw_update )
> +            panic("Svadu is advertised but A/D bits are not updated by 
> hardware\n");
> +        return;
> +    }
> +
> +    /* Only Svade advertised: the hart must not update A/D bits. */
> +    if ( svade )
> +    {
> +        ASSERT(!has_hw_update);
> +        return;
> +    }
> +
> +    /* 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.

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.

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

Jan