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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.10.2026 15:24, Andrew Cooper wrote:
> On 28/10/2026 1:05 pm, Baptiste Le Duc wrote:
>> diff --git a/xen/arch/riscv/cpufeature.c b/xen/arch/riscv/cpufeature.c
>> index 3d5bbd2228..83d3bc5ddc 100644
>> --- a/xen/arch/riscv/cpufeature.c
>> +++ b/xen/arch/riscv/cpufeature.c
>> @@ -630,6 +632,53 @@ const char *get_guest_isa_str(void)
>>      return guest_isa_str;
>>  }
>>  
>> +/*
>> + * Probe whether hardware updates the PTE A/D bits by reading through an 
>> alias
>> + * mapping whose A and D bits are clear. Observing A set, even by a 
>> speculative
>> + * access, implies Svadu, as Svade only allows A to be set by software.
>> + *
>> + * Under Svade, the read raises a page fault instead, so use the extable to
>> + * handle it.
> 
> This is an internal detail, which can be covered by an internal comment.
> 
>> + */
>> +static bool __init probe_hw_ad_update(void)
>> +{
>> +    vaddr_t va = FIXMAP_ADDR(FIX_MISC);
>> +    pte_t *entry = &xen_fixmap[FIX_MISC];
>> +    bool a_set = false;
>> +
>> +    ASSERT(!pte_is_valid(*entry));
>> +
>> +    /*
>> +     * 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.
> 
> The function comment is mostly ok, but this one is misleading, not least
> because it talks about corruption despite this being a read operation. 

I think though that the intent of the comment isn't limited to the read, but
also to possible (bogus) further use of that PTE.

> All you need to say here is:
> 
> /* Opencode set_fixmap() to intentionally leave A/D clear. */

Imo the seemingly arbitrary choice of which page to map can do with mentioning.

>> @@ -674,6 +723,28 @@ void __init riscv_fill_hwcap(void)
>>                 "Zihintpause unavailable: cpu_relax() gives the CPU no hint; 
>> "
>>                 "wire up this platform's pause equivalent in cpu_relax()\n");
>>  
>> +    /*
>> +     * The privileged spec allows exactly two schemes for updating the PTE A
>> +     * and D bits:
>> +     *  1) hardware sets them on access and on write;
>> +     *  2) accessing a page whose A and/or D bit is clear raises a page 
>> fault,
>> +     *     so that software can set them (ratified as Svade).
>> +     *
>> +     * Every implementation uses one of them, but may predate the extensions
>> +     * and advertise neither. In that case, trust the hardware probe.
>> +     */
>> +    if ( !riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svade) &&
>> +         !riscv_isa_extension_available(NULL, RISCV_ISA_EXT_svadu) )
>> +    {
>> +        bool hw_update = probe_hw_ad_update();
>> +
>> +        __set_bit(hw_update ? RISCV_ISA_EXT_svadu : RISCV_ISA_EXT_svade,
>> +                  riscv_isa);
>> +        printk(XENLOG_WARNING
>> +               "Device tree advertises neither Svade nor Svadu, probed %s: 
>> add it to \"riscv,isa\"\n",
>> +               hw_update ? "svadu" : "svade");
> 
> This is overly verbose, including details that absolutely shouldn't be
> the boot log.
> 
> "Neither Svade nor Svadu in Device Tree - probed as %s\n"

Some shortening is certainly worthwhile here, but omitting the hint what the
user can do about the situation is imo unhelpful.

Jan