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

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 10/28/26 6:25 PM, Baptiste Le Duc wrote:
> > > +    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.
I think that architecturally, per the RISC-V Privileged ISA 
specification, this outcome is not possible. During the execution of lw 
zero, 0(%[va]), there are only two valid spec-defined behaviors:
- Hardware A/D update (Svadu): The load instruction succeeds, and the 
hardware atomically sets the A bit (PTE_ACCESSED) in the PTE.
- Software A/D update (Svade): The load instruction triggers a Load Page 
Fault because the A bit is clear.
The specification does not allow (or at least, I don't see it.) a third 
case where the load succeeds without setting the A bit.
However, I agree that if we encounter non-compliant hardware or a buggy 
emulator where the load succeeds without setting the A bit, concluding 
that it is Svade (which strictly implies a page fault occurred) would be 
logically incorrect.
Considering this is incorrect behavior per the spec, and given that 
having Svade support by default is safe, nothing bad will happen if we 
report Svade in such a case. So I would be okay with the current 
behavour in v10 and it seems to me enough for now.
At the same time, I won't NACK the suggested approach below as it is 
harmless; I just think this scenario will never happen in practice, so 
it would essentially be dead code.
> > 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.
~ Oleksii