Re: [PATCH v6 1/4] xen/riscv: probe the PTE A/D bits update scheme at boot

[Jan Beulich <jbeulich@xxxxxxxx>]

On 14.10.2026 11:00, Baptiste Le Duc wrote:
>>>>> +        "2:\n"
>>>>> +        ASM_EXTABLE(1b, 2b)
>>>>> +        : [tmp] "=&r" (tmp), [updated] "+r" (updated)
>>>>> +        : [va] "r" (va)
>>>>> +        : "memory" );
>>>>> +
>>>>> +    arch_pmap_unmap(slot);
>>>>> +    free_domheap_page(pg);
>>>>> +
>>>>> +    return updated;
>>>>> +}
>>>>
>>>> Imo you'd better check that A also was actually set in the PTE.
>>>
>>> If I check that A was set, can I drop the "updated" variable? The A bit
>>> would tell us more than whether the load succeeded.
>>
>> Hmm, I'm inclined to suggest to check both.
> Then I'd rather probe D than A, since D, unlike A, cannot be set
> speculatively (spec: "updates to the D bit, resulting from an explicit store,
> must be exact (i.e., non-speculative)"). The result would
> then be:
> 
>       return updated && d_set;
> 
> with a warning (or panic()?) on updated && !d_set, which the spec
> doesn't allow (spec: "the access must not be performed before the PTE update 
> is
> globally visible").
> 
> Furthermore, if I understood your earlier suggestion correctly, you'd
> like the A/D bits to be preset in p2m_set_pte_flags() unless Svadu is
> in use:
> 
>     if ( !riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svadu) )
>         e->pte |= PTE_ACCESSED | PTE_DIRTY;
> 
> However, when both Svade and Svadu are advertised, Svade is in effect
> until Svadu is enabled via FWFT (perfectly legit), so this condition
> would skip the preset and the first access would fault.

In which case it would need to be

    if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) ||
         !riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svadu) )
        e->pte |= PTE_ACCESSED | PTE_DIRTY;

albeit I have to admit that permitting two contradicting extensions to
be exposed at the same time is irritating.

> I see two ways to address this:
> 
> 1) Keep checking Svade:
> 
>     if ( riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) )
>         e->pte |= PTE_ACCESSED | PTE_DIRTY;
> 
>     After resolve_ad_scheme(), the Svade bit is set in every case where
>     software has to preset the A/D bits. The only imprecise case is both
>     extensions advertised with hardware updating already enabled by
>     firmware, where presetting A/D is unnecessary but harmless.
> 
> 2) Record the probe result in a global, e.g.
> 
>     static bool __ro_after_init hw_ad_update;
> 
>     and use:
> 
>     if ( !hw_ad_update )
>         e->pte |= PTE_ACCESSED | PTE_DIRTY;
> 
>     This is exact in all cases, at the cost of a new global that mostly
>     duplicates what resolve_ad_scheme() already records in the ISA bitmap.
> 
> I discussed with Oleksii and we are fine with either. What do you prefer?

As to 1), see above. For 2), while maybe okay, I think we're better off
not deriving too much from probing. Whatever can be (reliably) derived from
what the platform description says should be preferred imo.

Jan