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

[Catalin Marinas <catalin.marinas@arm.com>]

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().

> >> +#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.

> >> 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.

-- 
Catalin