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

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

On Thu, Oct 24, 2024 at 10:45:51PM +0800, 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:
> >>>> +#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.

It's more of a theoretical case - walk_pmd_range() for example checks
should_clear_pmd_young() followed by !pmd_young(). Between these two
checks, should_clear_pmd_young() becomes false but the pmd may have been
accessed by a CPU without HAFT. We'd miss this. However, such race is
benign I think, only used for page aging so it shouldn't matter.

The other thing with your approach is the cost of checking (load, mask,
compare) vs just a static branch. Given that it's only done for pmds,
it's probably lost in the noise but you could check it to be sure.

-- 
Catalin