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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.10.2026 13:57, Baptiste Le Duc wrote:
> On 2026-10-13 12:36 +0200, Jan Beulich wrote:
>> On 13.10.2026 12:07, Baptiste Le Duc wrote:
>>> @@ -620,6 +623,98 @@ static void __init compute_guest_isa(void)
>>>          panic("Failed to build guest \"riscv,isa\" string\n");
>>>  }
>>>  
>>> +static bool __init probe_hw_ad_update(void)
>>> +{
>>> +    bool updated = false;
>>> +    unsigned int slot = FIX_MISC;
>>> +    vaddr_t va = FIXMAP_ADDR(slot);
>>> +    struct page_info *pg;
>>> +    pte_t *entry, pte;
>>> +    mfn_t mfn;
>>> +    unsigned long tmp;
>>> +
>>> +    pg = alloc_domheap_page(NULL, 0);
>>> +    if ( !pg )
>>> +        return false;
>>
>> Why would you need a fresh page here? Can't you create a secondary mapping
>> of a page already in use (e.g. one from Xen's .data or .bss section, or the
>> stack page you're running on)?
> You're right, I will map the current stack page.
>>
>>> +    mfn = page_to_mfn(pg);
>>> +
>>> +    arch_pmap_map(FIX_MISC, mfn);
>>> +    entry = &xen_fixmap[slot];
>>> +    pte = pte_from_mfn(mfn, PTE_VALID | PTE_READABLE);
>>> +    write_pte(entry, pte);
>>> +
>>> +    flush_tlb_one_local(va);
>>> +
>>> +    asm_inline volatile (
>>> +        "1: ld %[tmp], 0(%[va])\n"
>>> +        "   li %[updated], 1\n"
>>
>> If you mapped the stack page, you could preset a variable there to 1, load
>> from the alias mapping of that variable, and then avoid the LI altogether.
> Yes mapping the stack page appears to be the best way.
>>
>>> +        "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.
> Right, I will do that.
>>
>>> +
>>> +    /* Neither advertised: trust the probe. */
>>> +    __set_bit(has_hw_update ? RISCV_ISA_EXT_svadu : RISCV_ISA_EXT_svade,
>>> +              riscv_isa);
>>
>> Hmm, this feels fragile. Other schemes are possible, e.g. A being updated
>> by hardware but D being left to software. Are such truly precluded by the
>> spec, including older versions thereof? Furthermore, probe_hw_ad_update()
>> can (right now) return false for unrelated reasons.
> Since 1.10 the spec lists exactly two schemes, and each one covers A and
> D together: either a clear A (or a clear D on a store) raises a page
> fault, or hardware sets the corresponding bit(s). A-bit being set by
> hardware while D is left to software isn't one of them. Before 1.10, the
> hardware must set the two bits. Therefore, there is not other schemes
> possible.
>> As a consequence, patch 2 imo also can't leave the A/D pre-setting unaltered.
>> It perhaps needs doing unless Svadu is positively identified as present.
> I agree it is more robust to preset A/D bits unless Svadu is truly
> identified, i.e. advertised in the DTS (without Svade) and confirmed by
> probe_hw_ad_update(). The worst case then becomes the probe failing for
> an unrelated reason on hardware which does update A/D: we'd preset A/D
> needlessly, which is harmless, as hardware only writes the PTE when a
> bit is clear.
> 
> To sum up, and to avoid unnecessary respins, would you be fine with:
> 1) Keeping the probe as it is, with your remarks addressed (map stack page).

(... and verify A bit was set).

> 2) Presetting A/D bits unless Svadu is truly identified as above.

Yes. Assuming Oleksii also agrees.

>>> --- a/xen/arch/riscv/include/asm/cpufeature.h
>>> +++ b/xen/arch/riscv/include/asm/cpufeature.h
>>> @@ -42,6 +42,7 @@ enum riscv_isa_ext_id {
>>>      RISCV_ISA_EXT_sstc,
>>>      RISCV_ISA_EXT_svade,
>>>      RISCV_ISA_EXT_svpbmt,
>>> +    RISCV_ISA_EXT_svadu,
>>
>> Isn't there some sort of sorting in effect here?
> No, as it is mentioned in the docstring that:
>     * New extensions should just be added to the bottom, rather than added
>     * alphabetically, in order to avoid unnecessary shuffling.

I see. Yet I wonder: What shuffling would we need to be afraid of? Always
inserting at the bottom increases the risk of patch conflicts. Furthermore
single-letter extensions can't be inserted at the bottom anyway.

Jan