Re: [PATCH v10 1/1] xen/riscv: probe the A/D update scheme if the DT advertises neither

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 28/10/2026 1:05 pm, Baptiste Le Duc wrote:
> diff --git a/xen/arch/riscv/cpufeature.c b/xen/arch/riscv/cpufeature.c
> index 3d5bbd2228..83d3bc5ddc 100644
> --- a/xen/arch/riscv/cpufeature.c
> +++ b/xen/arch/riscv/cpufeature.c
> @@ -630,6 +632,53 @@ const char *get_guest_isa_str(void)
>      return guest_isa_str;
>  }
>  
> +/*
> + * Probe whether hardware updates the PTE A/D bits by reading through an 
> alias
> + * mapping whose A and D bits are clear. Observing A set, even by a 
> speculative
> + * access, implies Svadu, as Svade only allows A to be set by software.
> + *
> + * Under Svade, the read raises a page fault instead, so use the extable to
> + * handle it.

This is an internal detail, which can be covered by an internal comment.

> + */
> +static bool __init probe_hw_ad_update(void)
> +{
> +    vaddr_t va = FIXMAP_ADDR(FIX_MISC);
> +    pte_t *entry = &xen_fixmap[FIX_MISC];
> +    bool a_set = false;
> +
> +    ASSERT(!pte_is_valid(*entry));
> +
> +    /*
> +     * set_fixmap() can't be used, as pt_update_entry() always sets A and D.
> +     *
> +     * A and D belong to the PTE, not to the page, so a new PTE with A and D
> +     * clear can point to any RAM page, whatever its other mappings. Use the
> +     * first page of the Xen image, and map it read-only so that Xen can't be
> +     * corrupted through this alias.

The function comment is mostly ok, but this one is misleading, not least
because it talks about corruption despite this being a read operation. 
All you need to say here is:

/* Opencode set_fixmap() to intentionally leave A/D clear. */


> +     */
> +    write_pte(entry, pte_from_mfn(maddr_to_mfn(virt_to_maddr(_start)),

pte_from_mfn(virt_to_mfn(_start))

> +                                  PTE_VALID | PTE_READABLE));
> +    flush_tlb_one_local(va);
> +

/* Read from the alias, handling pagefault */

> +    asm_inline goto (
> +        "1: lw zero, 0(%[va])\n"
> +        ASM_EXTABLE(1b, %l[out])
> +        :
> +        : [va] "r" (va)
> +        : "memory"
> +        : out );
> +
> +    /* Order the load, and so its A bit update, before reading the PTE */

/* Check to see if hardware set A. */

> +    RISCV_FENCE(r, r);
> +    a_set = read_pte(entry).pte & PTE_ACCESSED;
> +
> + out:

It's better to name this label 'fault' because that's what happened to
use it.

What happens if you neither fault, and A doesn't get set?  Right now
you'll report Svade despite there being no fault.

You might or might not care to cover this case; that's up to the RISC-V
maintainers as to how likely it is to encounter in real life.

But, if you do want to handle it, then you want something like

    asm goto (...);

    if ( 0 )
    {
    fault:
        got_fault = true;
    }

    ...

    if ( !got_fault && !a_set )
        panic("Neither Svade nor Svadu !?!\n");


The if (0) is a bit ugly but it's the only way to avoid needing a second
label.  

> +    write_pte(entry, (pte_t){});
> +    flush_tlb_one_local(va);
> +
> +    return a_set;
> +}
> +
>  void __init riscv_fill_hwcap(void)
>  {
>      unsigned int i;
> @@ -674,6 +723,28 @@ void __init riscv_fill_hwcap(void)
>                 "Zihintpause unavailable: cpu_relax() gives the CPU no hint; "
>                 "wire up this platform's pause equivalent in cpu_relax()\n");
>  
> +    /*
> +     * The privileged spec allows exactly two schemes for updating 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).
> +     *
> +     * Every implementation uses one of them, but may predate the extensions
> +     * and advertise neither. In that case, trust the hardware probe.
> +     */
> +    if ( !riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) &&
> +         !riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svadu) )
> +    {
> +        bool hw_update = probe_hw_ad_update();
> +
> +        __set_bit(hw_update ? RISCV_ISA_EXT_svadu : RISCV_ISA_EXT_svade,
> +                  riscv_isa);
> +        printk(XENLOG_WARNING
> +               "Device tree advertises neither Svade nor Svadu, probed %s: 
> add it to \"riscv,isa\"\n",
> +               hw_update ? "svadu" : "svade");

This is overly verbose, including details that absolutely shouldn't be
the boot log.

"Neither Svade nor Svadu in Device Tree - probed as %s\n"

~Andrew