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

From: Bill Wendling <morbo@google.com>
To: Michael Ellerman <mpe@ellerman.id.au>
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 21:13:59 -0800	[thread overview]
Message-ID: <CAGG=3QUSF4UwcZQHhFE-PW6As7GVJknsyGkgVMENDXghABzy5A@mail.gmail.com> (raw)
In-Reply-To: <87zh37zaf4.fsf@mpe.ellerman.id.au>

On Mon, Nov 23, 2020 at 7:44 PM Michael Ellerman <mpe@ellerman.id.au> wrote:
> 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
>
This works for me. Thanks!

> 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.
>
You may need to set LLVM_IAS=1 to get the behavior I'm seeing. That
turns on clang's integrated assembler, which I think is disabled by
default.

Note that with clang's integrated assembler, arch/powerpc/boot/util.S
fails to compile. Alan Modra mentioned that he sent you a patch to
"modernize" the file so that clang can compile it.

-bw