[PATCH v3 3/5] arm64: Add support for FEAT_HAFT

[Yicong Yang]

On 2024/10/24 22:45, Yicong Yang wrote:
> On 2024/10/23 20:36, Catalin Marinas wrote:
>> On Wed, Oct 23, 2024 at 06:30:18PM +0800, Yicong Yang wrote:
>>> On 2024/10/23 2:30, Catalin Marinas wrote:
>>>> On Tue, Oct 22, 2024 at 05:27:32PM +0800, Yicong Yang wrote:
>>>>> diff --git a/arch/arm64/include/asm/pgalloc.h b/arch/arm64/include/asm/pgalloc.h
>>>>> index 8ff5f2a2579e..bc1051d65125 100644
>>>>> --- a/arch/arm64/include/asm/pgalloc.h
>>>>> +++ b/arch/arm64/include/asm/pgalloc.h
>>>>> @@ -30,7 +30,7 @@ static inline void pud_populate(struct mm_struct *mm, pud_t *pudp, pmd_t *pmdp)
>>>>>  {
>>>>>  	pudval_t pudval = PUD_TYPE_TABLE;
>>>>>  
>>>>> -	pudval |= (mm == &init_mm) ? PUD_TABLE_UXN : PUD_TABLE_PXN;
>>>>> +	pudval |= (mm == &init_mm) ? PUD_TABLE_AF | PUD_TABLE_UXN : PUD_TABLE_PXN;
>>>>>  	__pud_populate(pudp, __pa(pmdp), pudval);
>>>>>  }
>>>>
>>>> Why not set the table AF for the task entries? I haven't checked the
>>>> core code but normally when we map a pte it's mapped as young. While for
>>>> table AF we wouldn't get a fault, I would have thought the core code
>>>> follows the same logic.
>>>
>>> I may need to check. If I understand it correctly, for most case (e.g.
>>> a read fault) we should make pte young if the hardware AF update is
>>> not supported. Otherwsie hardware will help to update.
>>
>> On arm64 at least, _PROT_DEFAULT has PTE_AF set. So all accessible
>> entries in protection_map[] will have it set. I'm not sure how the core
>> code clears PTE_AF in the table entries. I'd have thought it goes
>> together with some pte_mkold().
>>
> 
> you're right. Checked that x86 will set the AF bit for the table entry [1][2].
> Will set AF for task entries.
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/include/asm/pgalloc.h#n84
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/x86/include/asm/pgtable_types.h#n221
> 
>>>>> +#ifdef CONFIG_ARM64_HAFT
>>>>> +	{
>>>>> +		.desc = "Hardware managed Access Flag for Table Descriptor",
>>>>> +		.type = ARM64_CPUCAP_WEAK_LOCAL_CPU_FEATURE,
>>>>
>>>> I'd actually use ARM64_CPUCAP_SYSTEM_FEATURE here. We use something
>>>> similar for HW DBM but there we get a fault and set the pte dirty. You
>>>> combined it with a system_support_haft() that checks the sanitised regs
>>>> but I'd rather have a static branch check via cpus_have_cap(). Even with
>>>> your approach we can have a race with a late CPU hot-plugged that
>>>> doesn't have the feature in the middle of some core code walking the
>>>> page tables.
>>>>
>>>> With a system feature type, late CPUs not having the feature won't be
>>>> brought online (if feature enabled) but in general I don't have much
>>>> sympathy for SoC vendors combining CPUs with incompatible features ;).
>>>
>>> ok. If we make it a system feature, we can using cpus_have_cap() then and
>>> drop the system_support_haft() which is checking with the sanitised registers.
>>> It's fine for me.
>>>
>>> Will ask to not refuse online a CPU due to mismatch of this feature in [1],
>>> hope we have an agreement :)
>>>
>>> [1] https://lore.kernel.org/linux-arm-kernel/20240820161822.GC28750@willie-the-truck/
>>
>> I initially thought this would work but I don't feel easy about having
>> should_clear_pmd_young() change its polarity at runtime while user space
>> is running. If that's not a problem, we can go with your current
>> approach.
> 
> this should be ok as I image. after online a CPU without HAFT the system won't
> advertise HAFT support but we don't disable the HAFT update on the supported
> CPUs, the ongoing page aging process can still use the updated table AF information
> and later process will fallback to use the PTE's AF bit. efficiency maybe reduced
> but the function should be correct.
> 

But we may have a chance to hit the VM_WARN_ON() in pmdp_test_and_clear_young() introduced in Patch 5/5:

CPUx                                         CPUy (without HAFT)
if (should_clear_pmd_yound())
                                             online and make system_support_haft() == false;
  pmdp_test_and_clear_young()
    VM_WARN_ON(pmd_table(READ_ONCE(*pmdp)) && !system_support_haft());

we may need to drop the VM_WARN_ON() with current approach or make this feature
ARM64_CPUCAP_SYSTEM_FEATURE.

> the user setting will be changed after online a CPU without HAFT. the user will
> know this unless read /sys/kernel/mm/lru_gen/enabled again (LRU_GEN_NONLEAF_YOUNG
> bit 0x4 will be cleared after online a CPU without HAFT since system would
> not declare non-leaf PMD young support).
> 
>>>>> diff --git a/arch/arm64/mm/proc.S b/arch/arm64/mm/proc.S
>>>>> index ccbae4525891..4a58b9b36eb6 100644
>>>>> --- a/arch/arm64/mm/proc.S
>>>>> +++ b/arch/arm64/mm/proc.S
>>>>> @@ -495,9 +495,15 @@ alternative_else_nop_endif
>>>>>  	 * via capabilities.
>>>>>  	 */
>>>>>  	mrs	x9, ID_AA64MMFR1_EL1
>>>>> -	and	x9, x9, ID_AA64MMFR1_EL1_HAFDBS_MASK
>>>>> +	ubfx	x9, x9, ID_AA64MMFR1_EL1_HAFDBS_SHIFT, #4
>>>>>  	cbz	x9, 1f
>>>>>  	orr	tcr, tcr, #TCR_HA		// hardware Access flag update
>>>>> +
>>>>> +#ifdef CONFIG_ARM64_HAFT
>>>>> +	cmp	x9, ID_AA64MMFR1_EL1_HAFDBS_HAFT
>>>>> +	b.lt	1f
>>>>> +	orr	tcr2, tcr2, TCR2_EL1x_HAFT
>>>>> +#endif /* CONFIG_ARM64_HAFT */
>>>>
>>>> I think we can skip the ID check here and always set the HAFT bit. We do
>>>> something similar with MTE (not for TCR_HA though, don't remember why).
>>>
>>> Thanks for the reference to MTE. Will see and have a test. But a check
>>> here may seem more reasonable as we usually detect a feature first
>>> then enable it?
>>
>> The behaviour of these RES0 bits is that we can write them and if the
>> feature is present, it will be enabled, otherwise it won't have any
>> effect, so it's not necessary to check the ID bits, the result would be
>> the same. We do this in other places as well.
>>
>> Of course, we need to check the presence of TCR2_EL1, otherwise it would
>> undef. Just a bit less code since we want the feature on anyway.
> 
> sure. will drop the check and set the HAFT bit unconditionally.
> 
> Thanks.
> 
> 
> .
>