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

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

On 2026-10-28 14:24 +0000, Andrew Cooper wrote:
> 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))
I didn't use that, as it would require adding
```
    /* Override macros from asm/page.h to make them work with mfn_t */
    #undef virt_to_mfn
    #define virt_to_mfn(v) _mfn(__virt_to_mfn(v))
```
as other archs do. But if you think it's worth it for a single usage, then
I could do it.

> 
> > +                                  PTE_VALID | PTE_READABLE));
> > +    flush_tlb_one_local(va);
> > +
> 
> /* Read from the alias, handling pagefault */
Will apply that in v11.
> 
> > +    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. */
It doesn't explain the fence, but mine doesn't either. If you think it
doesn't matter, then I will use what you propose in v11.
> 
> > +    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.
I don't know if such a case is allowed by the spec, but afaiu, and we
talked a lot about that with Jan B and Oleksii, if we don't fault then
we should report Svade.
> 
> 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.
I'm waiting for Jan B's and Oleksii's opinion on that point.
> 
> 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"
I agree with Jan on the point that it doesn't give any hint to the user.
What do you think of this shorter version:
"Svade/Svadu missing from \"riscv,isa\" - probed as %s\n"

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