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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 10/27/26 12:05 PM, Baptiste Le Duc wrote:

[...]

> +/*
> + * 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.
> + */
> +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));
> +
> +    /*
> +     * Any mapped page will do, as the alias is read-only, so alias the first
> +     * page of the Xen image, with A and D clear.
> +     */
> +    write_pte(entry, pte_from_mfn(maddr_to_mfn(virt_to_maddr(_start)),
> +                                  PTE_VALID | PTE_READABLE));
fixmap.h asks for xen_fixmap[] to be accessed directly only when
set_fixmap() is unusable. It is unusable here, as pt_update_entry() 
always sets A and D, but I think the comment should say so. Otherwise 
someone may "clean this up" by switching to set_fixmap(), after which 
the probe silently always reports Svadu, and on a Svade platform G-stage 
PTEs are then created with A/D clear.
The current comment also reads as if the page had to be mapped already,
while what makes using the Xen image correct is that A and D belong to
the PTE, not to the page, so those set in the Xen image mapping don't
matter. Something like:

    /*
     * 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.
     */


I am okay to go with how the comment is written now and apply my 
suggestion only in the case if v10 will be needed (and likely it will 
look at the comment about fence below)
> +    flush_tlb_one_local(va);
> +
> +    asm_inline goto (
> +        "1: lw zero, 0(%[va])\n"
> +        ASM_EXTABLE(1b, %l[out])
> +        :
> +        : [va] "r" (va)
> +        : "memory"
> +        : out );
> +
> +    a_set = read_pte(entry).pte & PTE_ACCESSED;
I think a fence is needed between the load and reading the PTE back. The
spec only orders the A bit update against accesses to the same virtual
page:

  "Furthermore, the PTE update must appear in the global memory order no
  later than the explicit memory access, or any subsequent explicit 
memory access to that virtual page by the local hart."
read_pte() accesses a different virtual page, and RVWMO doesn't order 
two loads to different addresses, so on a hart which updates A/D in 
hardware read_pte() may still observe A clear. That fails safe (Svade 
gets recorded and A/D are preset), but the message below then asks to 
add the wrong extension to the DT, and an out-of-order core like the 
P550 is exactly where this could happen. Ordering the load before the 
PTE read is enough, as the A bit update is in turn ordered before the load:
    /* Order the load, hence its A bit update, before reading the PTE. */
    RISCV_FENCE(r, r);

    a_set = read_pte(entry).pte & PTE_ACCESSED;

> +
> + out:
> +    write_pte(entry, (pte_t){});
> +    flush_tlb_one_local(va);
> +
> +    return a_set;
> +}
> +
[...]

I gave it a try on QEMU 9.0 on top of current staging (virt with
aia=aplic-imsic, all with svpbmt=on):
 - default (svadu): the probe doesn't run;
 - svadu=off (hardware updating, neither in the DT): "probed svadu";
 - svadu=off,svade=on with "_svade" dropped from riscv,isa: the load
   faults, the extable fixup works, "probed svade";
 - svadu=off,svade=on: the probe doesn't run.
All of them reach "All set up" so:
 Tested-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>

~ Oleksii