Re: [PATCH v8 4/5] xen/riscv: probe the A/D update scheme if the DT advertises neither

[Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>]

On 10/26/26 4:14 PM, Baptiste Le Duc wrote:

>   
> +/*
> + * Probe whether hardware updates the PTE A/D bits by writing to a local
> + * variable through an alias mapping whose A and D bits are clear.
> + *
> + * Check the D bit rather than the A bit, as the A bit may be set
> + * speculatively while the D bit can't.
> + *
> + * Under Svade, the store raises a page fault instead, so use the extable to
> + * handle it.
> + */
> +static bool __init probe_hw_ad_update(void)
> +{
> +    unsigned int probe_target;
> +    vaddr_t va = FIXMAP_ADDR(FIX_MISC) + PAGE_OFFSET(&probe_target);
> +    pte_t *entry = &xen_fixmap[FIX_MISC];
> +    bool d_set = false;
> +
> +    ASSERT(!pte_is_valid(*entry));
> +
> +    /*
> +     * Any page in use will do, so alias the stack page this code runs on,
> +     * with A and D clear. Store to a variable of its own, so that nothing
> +     * the compiler relies on gets overwritten.
> +     */
> +    write_pte(entry, pte_from_mfn(maddr_to_mfn(virt_to_maddr(&probe_target)),
> +                                  PTE_VALID | PTE_READABLE | PTE_WRITABLE));
> +    flush_tlb_one_local(va);
> +
> +    asm_inline goto (
I think that generally this code should be wrapped by "#ifdef 
CONFIG_CC_HAS_ASM_GOTO_OUTPUT" but we can keep it as is here as we don't 
use output operands.
> +        "1: sw zero, 0(%[va])\n"
> +        ASM_EXTABLE(1b, %l[out])
> +        :
> +        : [va] "r" (va)
> +        : "memory"
> +        : out );> +
> +    d_set = read_pte(entry).pte & PTE_DIRTY;
> +
> + out:
> +    write_pte(entry, (pte_t){});
> +    flush_tlb_one_local(va);
> +
> +    return d_set;
> +}
> +
>   void __init riscv_fill_hwcap(void)
>   {
>       unsigned int i;
> @@ -674,6 +721,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");
> +    }
> +
>       for ( i = 0; i < req_extns_amount; i++ )
>       {
>           const struct riscv_isa_ext_data ext = required_extensions[i];
> diff --git a/xen/arch/riscv/include/asm/cpufeature.h 
> b/xen/arch/riscv/include/asm/cpufeature.h
> index 2973eb13a5..b0a23be18e 100644
> --- a/xen/arch/riscv/include/asm/cpufeature.h
> +++ b/xen/arch/riscv/include/asm/cpufeature.h
> @@ -8,13 +8,13 @@
>   #include <xen/types.h>
>   
>   /*
> - * These macros represent the logical IDs of each multi-letter RISC-V ISA
> - * extension and are used in the ISA bitmap. The logical IDs start from
> - * RISCV_ISA_EXT_BASE, which allows the 0-25 range to be reserved for single
> - * letter extensions and are used in enum riscv_isa_ext_id.
> + * Logical IDs of RISC-V ISA extensions, used as bit positions in the ISA
> + * bitmap.
>    *
> - * New extensions should just be added to the bottom, rather than added
> - * alphabetically, in order to avoid unnecessary shuffling.
> + * Single-letter extensions use IDs below RISCV_ISA_EXT_BASE and are sorted
> + * alphabetically. Multi-letter extensions start from RISCV_ISA_EXT_BASE and
> + * follow the same order as riscv_isa_ext[] (see the ordering rules described
> + * above it).
>    */
>   #define RISCV_ISA_EXT_BASE  26
>   
> @@ -41,6 +41,7 @@ enum riscv_isa_ext_id {
>       RISCV_ISA_EXT_ssaia,
>       RISCV_ISA_EXT_sstc,
>       RISCV_ISA_EXT_svade,
> +    RISCV_ISA_EXT_svadu,
>       RISCV_ISA_EXT_svpbmt,
>       RISCV_ISA_EXT_MAX
>   };
Generally, looks good to me:

Reviewed-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>

~ Oleksii