Re: [PATCH v9 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 10:25:53+01:00, Oleksii Kurochko wrote:
> [ ... 27 lines skipped ... ]
> 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)
I will use that in v10 thanks.
> [ ... 11 lines skipped ... ]
> > +    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);
Good catch thanks, will apply that in v10.
> [ ... 12 lines skipped ... ]
> [...]
> 
> 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>
Thanks for having tested.

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