Re: [PATCH 2/2] arm64: Move the LSE gas support detection to Kconfig

From: Will Deacon <will@kernel.org>
To: Catalin Marinas <catalin.marinas@arm.com>
Cc: Vladimir Murzin <vladimir.murzin@arm.com>,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 2/2] arm64: Move the LSE gas support detection to Kconfig
Date: Fri, 10 Jan 2020 15:45:43 +0000	[thread overview]
Message-ID: <20200110154542.GB24322@willie-the-truck> (raw)
In-Reply-To: <20200110153013.GH8786@arrakis.emea.arm.com>

On Fri, Jan 10, 2020 at 03:30:14PM +0000, Catalin Marinas wrote:
> On Fri, Jan 10, 2020 at 12:08:26PM +0000, Will Deacon wrote:
> > On Fri, Jan 10, 2020 at 11:54:38AM +0000, Vladimir Murzin wrote:
> > > On 1/9/20 5:49 PM, Catalin Marinas wrote:
> > > > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> > > > index 04cf64e9f0c9..2595c2886d3f 100644
> > > > --- a/arch/arm64/kernel/cpufeature.c
> > > > +++ b/arch/arm64/kernel/cpufeature.c
> > > > @@ -1291,7 +1291,7 @@ static const struct arm64_cpu_capabilities arm64_features[] = {
> > > >  		.cpu_enable = cpu_enable_pan,
> > > >  	},
> > > >  #endif /* CONFIG_ARM64_PAN */
> > > > -#if defined(CONFIG_AS_LSE) && defined(CONFIG_ARM64_LSE_ATOMICS)
> > > > +#ifdef CONFIG_ARM64_LSE_ATOMICS
> > > >  	{
> > > >  		.desc = "LSE atomic instructions",
> > > >  		.capability = ARM64_HAS_LSE_ATOMICS,
> > > > @@ -1302,7 +1302,7 @@ static const struct arm64_cpu_capabilities arm64_features[] = {
> > > >  		.sign = FTR_UNSIGNED,
> > > >  		.min_field_value = 2,
> > > >  	},
> > > > -#endif /* CONFIG_AS_LSE && CONFIG_ARM64_LSE_ATOMICS */
> > > > +#endif /* CONFIG_ARM64_LSE_ATOMICS */
> > > >  	{
> > > >  		.desc = "Software prefetching using PRFM",
> > > >  		.capability = ARM64_HAS_NO_HW_PREFETCH,
> > > > 
> > > 
> > > I was not lucky with the similar patch [1], anyway
> > > 
> > > Reviewed-by: Vladimir Murzin <vladimir.murzin@arm.com>
> > > 
> > > 
> > > [1] https://www.spinics.net/lists/linux-crypto/msg36059.html
> > 
> > It's the loss of the warning that I object to, since I think it's a useful
> > diagnostic to have. Is there some way we can keep that, but using the new
> > kbuild logic?
> 
> With the kbuild logic, you can't select CONFIG_ARM64_LSE_ATOMICS because
> CONFIG_AS_HAS_LSE is n (when gas doesn't support LSE). I consider this a
> good indication to the user trying to enable it without the need for a
> warning.
> 
> The alternative is to let the user state their preference with a config
> option without any dependencies:
> 
> config ARM64_WANT_LSE_ATOMICS
> 	bool "..."
> 	depends on JUMP_LABEL
> 	default y
> 
> config ARM64_LSE_ATOMICS
> 	def_bool CONFIG_ARM64_WANT_LSE_ATOMICS
> 	depends on CONFIG_AS_HAS_LSE
> 
> and in the Makefile, warn if CONFIG_ARM64_WANT_LSE_ATOMICS &&
> !CONFIG_ARM64_LSE_ATOMICS.
> 
> You can even get the warning directly from kbuild if you select
> ARM64_LSE_ATOMICS from ARM64_WANT_* (unmet dependency).
> 
> I personally don't think it's worth the hassle. We don't warn if the
> compiler doesn't support jump labels, why would we do this for LSE.

The thing I dislike about it is that if somebody sends you a .config with
LSE enabled, and your compiler doesn't support it, then it silently get
disabled. There are two differences with jump labels:

  1. Most compilers support jump labels
  2. LSE atomics also depend on the CPU features before they get enabled
     at runtime.

Mainly because of (2), I think silently disabling LSE at build time is
really confusing when you're trying to figure out why it's not in use.

Will

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