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

[Samuel Holland <samuel.holland@sifive.com>]

Hi Conor,

On 2026-05-19 10:37 AM, Conor Dooley wrote:
> On Tue, May 19, 2026 at 10:05:21PM +1000, Michael Ellerman wrote:
>> 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@bytedance.com>
>>> Reviewed-by: Qingwei Hu <qingwei.hu@bytedance.com>
>>> ---
>>>   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.
> 
> Is this code correct though? On DT systems, !svade && !svadu means we
> don't actually know if it is hardware or software managed, so printing
> that it's hardware managed may not be correct.
> 
> I don't understand the mm code enough to know if arch_has_hw_pte_young()
> returning true is problematic too, but it probably is?

This block is for !svade && svadu (Svadu is present; we didn't return above), so
I think this block and Michael's comment are correct.

Regards,
Samuel


_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv