Re: [PATCH 1/2] riscv: track effective hardware PTE A/D updates

[Michael Ellerman]

On 19/5/2026 13:19, Yunhui Cui wrote:
> Separate Svadu capability discovery from the host's effective ADUE
> state. Enable SBI FWFT PTE A/D hardware updating on each online CPU
> through CPUHP when both Svade and Svadu are present, use the resulting
> runtime state for arch_has_hw_pte_young(), and fall back to
> software-managed A/D updates when enabling the feature fails.
>
> Platforms with Svadu but without Svade are treated as always using
> hardware PTE A/D updates. Expose the runtime state through an inline
> getter so hot MM paths avoid an out-of-line function call.

I'm not sure what you mean here. The current code doesn't use an 
out-of-line function call AFAICS? More comments below ...

> Signed-off-by: Yunhui Cui <cuiyunhui@xxxxxxxxxxxxx>
> Reviewed-by: Qingwei Hu <qingwei.hu@xxxxxxxxxxxxx>
> ---
>   arch/riscv/include/asm/cpufeature.h |  6 +++
>   arch/riscv/include/asm/pgtable.h    |  8 ++--
>   arch/riscv/kernel/cpufeature.c      | 73 ++++++++++++++++++++++++++---
>   3 files changed, 77 insertions(+), 10 deletions(-)
>
> diff --git a/arch/riscv/include/asm/cpufeature.h b/arch/riscv/include/asm/cpufeature.h
> index 739fcc84bf7b2..877d71a1ea755 100644
> --- a/arch/riscv/include/asm/cpufeature.h
> +++ b/arch/riscv/include/asm/cpufeature.h
> @@ -128,6 +128,12 @@ struct riscv_isa_ext_data {
>   extern const struct riscv_isa_ext_data riscv_isa_ext[];
>   extern const size_t riscv_isa_ext_count;
>   extern bool riscv_isa_fallback;
> +extern bool riscv_hw_pte_ad_updating_enabled;
> +
> +static __always_inline bool riscv_has_hw_pte_ad_updating(void)
> +{
> +	return READ_ONCE(riscv_hw_pte_ad_updating_enabled);
> +}
>   
>   unsigned long riscv_isa_extension_base(const unsigned long *isa_bitmap);
>   static __always_inline bool riscv_cpu_has_extension_likely(int cpu, const unsigned long ext)
> diff --git a/arch/riscv/include/asm/pgtable.h b/arch/riscv/include/asm/pgtable.h
> index a1a7c6520a095..20663a466cf6c 100644
> --- a/arch/riscv/include/asm/pgtable.h
> +++ b/arch/riscv/include/asm/pgtable.h
> @@ -732,14 +732,14 @@ static inline pgprot_t pgprot_writecombine(pgprot_t _prot)
>   #define pgprot_dmacoherent pgprot_writecombine
>   
>   /*
> - * Both Svade and Svadu control the hardware behavior when the PTE A/D bits need to be set. By
> - * default the M-mode firmware enables the hardware updating scheme when only Svadu is present in
> - * DT.
> + * Both Svade and Svadu control the hardware behavior when the PTE A/D bits
> + * need to be set. The core MM code only cares whether hardware updating of
> + * the accessed/dirty state is currently active.
>    */
>   #define arch_has_hw_pte_young arch_has_hw_pte_young
>   static inline bool arch_has_hw_pte_young(void)
>   {
> -	return riscv_has_extension_unlikely(RISCV_ISA_EXT_SVADU);
> +	return riscv_has_hw_pte_ad_updating();
>   }

riscv_has_extension_unlikely() uses an asm alternative, ie. it's patched 
at boot so there's no runtime cost. But now you've changed it to just 
test a bool.

I'm not sure arch_has_hw_pte_young() is a particularly hot path, but 
seems like you could use a static key, so that the code is patched to 
avoid the runtime test?

> diff --git a/arch/riscv/kernel/cpufeature.c b/arch/riscv/kernel/cpufeature.c
> index f46aa5602d74d..e46b2d2b49eed 100644
> --- a/arch/riscv/kernel/cpufeature.c
> +++ b/arch/riscv/kernel/cpufeature.c
> @@ -35,6 +35,8 @@
>   static bool any_cpu_has_zicboz;
>   static bool any_cpu_has_zicbop;
>   static bool any_cpu_has_zicbom;
> +bool riscv_hw_pte_ad_updating_enabled __read_mostly;
> +EXPORT_SYMBOL_GPL(riscv_hw_pte_ad_updating_enabled);
>   
>   unsigned long elf_hwcap __read_mostly;
>   
> @@ -287,15 +289,74 @@ static int riscv_ext_zvfbfwma_validate(const struct riscv_isa_ext_data *data,
...
>   
> +static int __init riscv_hw_pte_ad_updating_init(void)
> +{
> +	bool has_svade, has_svadu;
> +	int state;
> +
> +	has_svade = riscv_has_extension_unlikely(RISCV_ISA_EXT_SVADE);
> +	has_svadu = riscv_has_extension_unlikely(RISCV_ISA_EXT_SVADU);
> +
> +	if (!has_svadu)
> +		return 0;
> +
> +	if (!has_svade) {
> +		riscv_set_hw_pte_ad_updating(true);
> +		pr_info("riscv: hardware PTE A/D updating enabled\n");
> +		return 0;

This block is identical to the tail of the function. I'd probably use 
"goto enable", with an "enable" label below.

> +	}
> +
> +	state = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN,
> +				  "riscv/pte-ad:starting",
> +				  riscv_hw_pte_ad_updating_starting,
> +				  riscv_hw_pte_ad_updating_dying);
> +	if (state < 0) {
> +		pr_info("riscv: leave PTE A/D updates software-managed (%d)\n",
> +			state);
> +		return 0;
> +	}
> +
> +	/*
> +	 * A successful CPUHP_AP_ONLINE_DYN registration means the startup
> +	 * callback has already succeeded on all online CPUs.
> +	 */

enable:
> +	riscv_set_hw_pte_ad_updating(true);
> +	pr_info("riscv: hardware PTE A/D updating enabled\n");

pr_info() might be a bit verbose for this. I think printk(KERN_DEBUG ..) 
would be better, that way the message is always available in dmesg but 
isn't sent to the console

>   	return 0;
>   }
> +arch_initcall(riscv_hw_pte_ad_updating_init);

cheers