Re: [PATCH] arm64: mm: fix warning in arch_faults_on_old_pte()

From: Catalin Marinas <catalin.marinas@arm.com>
To: Yu Zhao <yuzhao@google.com>
Cc: Jia He <justin.he@arm.com>, Mark Rutland <mark.rutland@arm.com>,
	Will Deacon <will@kernel.org>, Marc Zyngier <maz@kernel.org>,
	Suzuki Poulose <Suzuki.Poulose@arm.com>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH] arm64: mm: fix warning in arch_faults_on_old_pte()
Date: Mon, 14 Jun 2021 18:07:29 +0100	[thread overview]
Message-ID: <20210614170727.GG30667@arm.com> (raw)
In-Reply-To: <20210613214728.1695340-1-yuzhao@google.com>

On Sun, Jun 13, 2021 at 03:47:28PM -0600, Yu Zhao wrote:
> cow_user_page() doesn't disable preemption, and it triggers the
> warning in arch_faults_on_old_pte() when CONFIG_PREEMPT_COUNT=y.

I'm surprised we haven't seen this warning until now. We probably
haven't exercised this path with the right config options.

> Converting the Access flag support to a system-wide feature to avoid
> reading ID_AA64MMFR1_EL1 on local CPUs when determining the h/w cap.
> 
> Note that though the Access flag support is a non-conflicting feature,
> we require all late CPUs to have it if the boot CPU does. Otherwise
> the feature won't be enabled regardless of the capabilities of late
> CPUs.
> 
> If there are h/w implementations that break this rule, they will have
> to add errata, unless they can provide justifications to switch to the
> less strict ARM64_CPUCAP_WEAK_LOCAL_CPU_FEATURE.
> 
> Signed-off-by: Yu Zhao <yuzhao@google.com>

I'm ok in principle but one small issue below.

> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index efed2830d141..afdb6e0336ed 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -1566,6 +1566,14 @@ static bool has_hw_dbm(const struct arm64_cpu_capabilities *cap,
>  	return true;
>  }
>  
> +static void cpu_enable_hw_af(struct arm64_cpu_capabilities const *cap)
> +{
> +	if (has_cpuid_feature(cap, SCOPE_LOCAL_CPU)) {
> +		u64 val = read_sysreg(tcr_el1);
> +
> +		write_sysreg(val | TCR_HA, tcr_el1);
> +	}
> +}

This needs an isb(); local_flush_tlb_all() since this bit may be cached
in the TLB. See how we did it in __cpu_enable_hw_dbm().

Alternatively you could leave the current TCR_AF bit setting as is (in
proc.S) and only add an arm64_features[] entry for AF together with the
arch_faults_on_old_pte() change but without any enable function.

I'm not sure we have mixed CPUs where only some of them support AF to
benefit from this.

-- 
Catalin

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