Re: [Xen-devel] [PATCH v2] x86: fix alternative_callN usage of ALTERNATIVE asm macro

From: "Jan Beulich" <JBeulich@suse.com>
To: "Roger Pau Monne" <roger.pau@citrix.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>,
	Wei Liu <wei.liu2@citrix.com>,
	xen-devel <xen-devel@lists.xenproject.org>
Subject: Re: [Xen-devel] [PATCH v2] x86: fix alternative_callN usage of ALTERNATIVE asm macro
Date: Thu, 23 May 2019 03:41:23 -0600	[thread overview]
Message-ID: <5CE66AC302000078002319E6@prv1-mh.provo.novell.com> (raw)
Message-ID: <20190523094123.5zJylCcKvVUkTJUhw-1BJWk-UIXHiVR3HFGs0Co-J8g@z> (raw)
In-Reply-To: <20190522164530.33147-1-roger.pau@citrix.com>

>>> On 22.05.19 at 18:45, <roger.pau@citrix.com> wrote:
> alternative_callN using inline assembly to generate the alternative
> patch sites should be using the ALTERNATIVE C preprocessor macro
> rather than the ALTERNATIVE assembly macro,

Why? See INDIRECT_{CALL,JMP}. My goal, as said on irc, would be
to eventually eliminate the redundant C macros, in favor of just using
the assembler ones.

> the more that using the
> assembly macro in an inline assembly instance causes the following
> error on llvm based toolchains:
> 
> <instantiation>:1:1: error: invalid symbol redefinition
> .L0_orig_s: call *genapic+64(%rip); .L0_orig_e: .L0_diff = (.L0_repl_e1 - 
> .L0_repl_s1) - (...

The understanding I get is that clang doesn't properly support the
\@ construct, expanding it to zero every time. That's a clang bug
imo, and hence the wording here should reflect this, rather than
suggesting the code is broken. (I seem to vaguely recall an issue
with clang instantiating a new assembly environment every time
it encounters an asm().) Without clang fixed, and with us wanting
to be able to continue to build with clang, this then voids the entire
purpose of f850619692 ("x86/alternatives: allow using assembler
macros in favor of C ones"), which irc was originally part of the
series, but went in much ahead of it.

> --- a/xen/include/asm-x86/alternative.h
> +++ b/xen/include/asm-x86/alternative.h
> @@ -202,9 +202,8 @@ extern void alternative_branches(void);
>      rettype ret_;                                                  \
>      register unsigned long r10_ asm("r10");                        \
>      register unsigned long r11_ asm("r11");                        \
> -    asm volatile (__stringify(ALTERNATIVE "call *%c[addr](%%rip)", \
> -                                          "call .",                \
> -                                          X86_FEATURE_ALWAYS)      \
> +    asm volatile (ALTERNATIVE("call *%c[addr](%%rip)", "call .",   \
> +                              X86_FEATURE_ALWAYS)                  \
>                    : ALT_CALL ## n ## _OUT, "=a" (ret_),            \
>                      "=r" (r10_), "=r" (r11_) ASM_CALL_CONSTRAINT   \
>                    : [addr] "i" (&(func)), "g" (func)               \

Okay, luckily the code change itself is simple enough, so it really
wasn't that I had to use the variant used to make things work at
all.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel