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

[Yicong Yang]

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.

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.