Re: [PATCH v3 3/3] powerpc/64s: feature: Work around inline asm issues

From: Michael Ellerman <mpe@ellerman.id.au>
To: Bill Wendling <morbo@google.com>,
	Segher Boessenkool <segher@kernel.crashing.org>
Cc: Nick Desaulniers <ndesaulniers@google.com>,
	linuxppc-dev@lists.ozlabs.org
Subject: Re: [PATCH v3 3/3] powerpc/64s: feature: Work around inline asm issues
Date: Tue, 24 Nov 2020 14:43:59 +1100	[thread overview]
Message-ID: <87zh37zaf4.fsf@mpe.ellerman.id.au> (raw)
In-Reply-To: <CAGG=3QUeXTU+8jqw40W_rhatsHCRiuTboL3enz9bpt_jaJC3TA@mail.gmail.com>

Bill Wendling <morbo@google.com> writes:
> On Mon, Nov 23, 2020 at 12:10 PM Segher Boessenkool
> <segher@kernel.crashing.org> wrote:
>> On Mon, Nov 23, 2020 at 12:01:01PM -0800, Bill Wendling wrote:
>> > On Mon, Nov 23, 2020 at 11:58 AM Segher Boessenkool
>> > <segher@kernel.crashing.org> wrote:
>> > > > On Sun, Nov 22, 2020 at 10:36 PM Segher Boessenkool
>> > > > <segher@kernel.crashing.org> wrote:
>> > > > > "true" (as a result of a comparison) in as is -1, not 1.
>> > >
>> > > On Mon, Nov 23, 2020 at 11:43:11AM -0800, Bill Wendling wrote:
>> > > > What Segher said. :-) Also, if you reverse the comparison, you'll get
>> > > > a build error.
>> > >
>> > > But that means your patch is the wrong way around?
>> > >
>> > > -       .ifgt (label##4b- label##3b)-(label##2b- label##1b);    \
>> > > -       .error "Feature section else case larger than body";    \
>> > > -       .endif;                                                 \
>> > > +       .org . - ((label##4b-label##3b) > (label##2b-label##1b)); \
>> > >
>> > > It should be a + in that last line, not a -.
>> >
>> > I said so in a follow up email.
>>
>> Yeah, and that arrived a second after I pressed "send" :-)
>>
> Michael, I apologize for the churn with these patches. I believe the
> policy is to resend the match as "v4", correct?
>
> I ran tests with the change above. It compiled with no error. If I
> switch the labels around to ".org . + ((label##2b-label##1b) >
> (label##4b-label##3b))", then it fails as expected.

I wanted to retain the nicer error reporting for gcc builds, so I did it
like this:

diff --git a/arch/powerpc/include/asm/feature-fixups.h b/arch/powerpc/include/asm/feature-fixups.h
index b0af97add751..c4ad33074df5 100644
--- a/arch/powerpc/include/asm/feature-fixups.h
+++ b/arch/powerpc/include/asm/feature-fixups.h
@@ -36,6 +36,24 @@ label##2:						\
 	.align 2;					\
 label##3:
 
+
+#ifndef CONFIG_CC_IS_CLANG
+#define CHECK_ALT_SIZE(else_size, body_size)			\
+	.ifgt (else_size) - (body_size);			\
+	.error "Feature section else case larger than body";	\
+	.endif;
+#else
+/*
+ * If we use the ifgt syntax above, clang's assembler complains about the
+ * expression being non-absolute when the code appears in an inline assembly
+ * statement.
+ * As a workaround use an .org directive that has no effect if the else case
+ * instructions are smaller than the body, but fails otherwise.
+ */
+#define CHECK_ALT_SIZE(else_size, body_size)			\
+	.org . + ((else_size) > (body_size));
+#endif
+
 #define MAKE_FTR_SECTION_ENTRY(msk, val, label, sect)		\
 label##4:							\
 	.popsection;						\
@@ -48,9 +66,7 @@ label##5:							\
 	FTR_ENTRY_OFFSET label##2b-label##5b;			\
 	FTR_ENTRY_OFFSET label##3b-label##5b;			\
 	FTR_ENTRY_OFFSET label##4b-label##5b;			\
-	.ifgt (label##4b- label##3b)-(label##2b- label##1b);	\
-	.error "Feature section else case larger than body";	\
-	.endif;							\
+	CHECK_ALT_SIZE((label##4b-label##3b), (label##2b-label##1b)); \
 	.popsection;
 
 

I've pushed a branch with all your patches applied to:

  https://github.com/linuxppc/linux/commits/next-test


Are you able to give that a quick test? It builds clean with clang for
me, but we must be using different versions of clang because my branch
already builds clean for me even without your patches.

cheers
