Re: [PATCH v5 10/10] arm64: atomics: Use K constraint when toolchain appears to support it

From: Will Deacon <will@kernel.org>
To: Andrew Murray <andrew.murray@arm.com>
Cc: mark.rutland@arm.com, peterz@infradead.org,
	catalin.marinas@arm.com, ndesaulniers@google.com,
	Ard.Biesheuvel@arm.com, natechancellor@gmail.com,
	robin.murphy@arm.com, linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v5 10/10] arm64: atomics: Use K constraint when toolchain appears to support it
Date: Fri, 30 Aug 2019 08:52:20 +0100	[thread overview]
Message-ID: <20190830075220.6xyyctvgd2ssrsjf@willie-the-truck> (raw)
In-Reply-To: <20190830000803.GR14582@e119886-lin.cambridge.arm.com>

On Fri, Aug 30, 2019 at 01:08:03AM +0100, Andrew Murray wrote:
> On Thu, Aug 29, 2019 at 05:54:58PM +0100, Will Deacon wrote:
> > On Thu, Aug 29, 2019 at 04:48:34PM +0100, Will Deacon wrote:
> > > diff --git a/arch/arm64/include/asm/atomic_ll_sc.h b/arch/arm64/include/asm/atomic_ll_sc.h
> > > index 95091f72228b..7fa042f5444e 100644
> > > --- a/arch/arm64/include/asm/atomic_ll_sc.h
> > > +++ b/arch/arm64/include/asm/atomic_ll_sc.h
> > > @@ -23,6 +23,10 @@ asm_ops "\n"								\
> > >  #define __LL_SC_FALLBACK(asm_ops) asm_ops
> > >  #endif
> > >  
> > > +#ifndef CONFIG_CC_HAS_K_CONSTRAINT
> > > +#define K
> > > +#endif
> > 
> > Bah, I need to use something like __stringify when the constraint is used
> > in order for this to get expanded properly. Updated diff below.
> 
> I don't think the changes in your updated diff are required. We successfully
> combine 'asm_op' with the remainder of the assembly string without using
>  __stringify, and this is no different to how the original patch combined
> 'constraint' with "r".

It's a hack: __stringify expands its arguments, so I figured I may as well
use that rather than do it manually with an extra macro.

> You can verify this by looking at the preprocessed .i files generated with
> something like:
> 
> make ARCH=arm64 CROSS_COMPILE=aarch64-linux-gnu- drivers/spi/spi-rockchip.i
> 
> I see no difference (with GCC 7.3.1) between the original approach and your
> use of __stringify. Incidentally you end up with "K" "r" instead of "Kr" but
> it seems to have the desired effect (e.g. supress/emit out of range errors).
> I have a couple of macros that resolves this to "Kr" but I don't think it's
> necessary.
> 
> Did you find that it didn't work without your changes? I found it hard to
> reproduce the out-of-range errors until I made the following change, I could
> then easily see the effect of changing the constraint:
> 
>         : "=&r" (result), "=&r" (tmp), "+Q" (v->counter)                \
> -       : #constraint "r" (i));                                         \
> +       : #constraint) "r" (4294967295));                                               \
>  }

Without the __stringify I get a compilation failure when building
kernel/panic.o because it tries to cmpxchg a 32-bit variable with -1
(PANIC_CPU_INVALID). Looking at panic.s, I see that constraint parameter
isn't being expanded. For example if I do:

  #ifndef CONFIG_CC_HAS_K_CONSTRAINT
  #define INVALID_CONSTRAINT
  #else
  #define INVALID_CONSTRAINT	K
  #endif

and then pass INVALID_CONSTRAINT to the generator macros, we'll end up
with INVALID_CONSTRAINT in the .s file and gas will barf.

The reason I didn't see this initially is because my silly testcase had
a typo and was using atomic_add instead of atomic_and.

Will

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