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

[Oleksii Kurochko <oleksii.kurochko@gmail.com>]

On 10/14/26 11:09 AM, Jan Beulich wrote:
> 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.

Option 1 looks okay to me.

Also, it seems then we could simplify resolve_ad_scheme() to:

/*
  * p2m_set_pte_flags() presets the A/D bits unless only Svadu is 
advertised,
  * as per the DT binding that is the only case in which hardware 
updating is
  * guaranteed to be enabled (by firmware, via menvcfg.ADUE). Check that it
  * really is, and fall back to presetting A/D otherwise: a hart which 
faults
  * on clear A/D bits implements Svade, whether or not it says so.
  */
static void __init resolve_ad_scheme(void)
{
     if ( !riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svadu) ||
          riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) )
         return;

     if ( !probe_hw_ad_update() )
     {
         printk(XENLOG_WARNING
                "Svadu is advertised but A/D bits aren't updated by 
hardware, presetting them\n");
         __set_bit(RISCV_ISA_EXT_svade, riscv_isa);
     }
}

~ Oleksii