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

Roger Pau Monne posted 1 patch 4 years, 10 months ago
Failed in applying to current master (apply log)
xen/include/asm-x86/alternative.h | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
[Xen-devel] [PATCH v2] x86: fix alternative_callN usage of ALTERNATIVE asm macro
Posted by Roger Pau Monne 4 years, 10 months ago
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, 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) - (...
^
<instantiation>:1:37: error: invalid symbol redefinition
.L0_orig_s: call *genapic+64(%rip); .L0_orig_e: .L0_diff = (.L0_repl_e1 - .L0_repl_s1) - (...
                                    ^
<instantiation>:1:60: error: invalid reassignment of non-absolute variable '.L0_diff'
.L0_orig_s: call *genapic+64(%rip); .L0_orig_e: .L0_diff = (.L0_repl_e1 - .L0_repl_s1) - (...
                                                           ^
<inline asm>:1:2: note: while in macro instantiation
        ALTERNATIVE "call *genapic+64(%rip)", "call .", X86_FEATURE_LM
        ^
<instantiation>:1:156: error: invalid symbol redefinition
  ...- (.L0_orig_e - .L0_orig_s); mknops ((-(.L0_diff > 0)) * .L0_diff); .L0_orig_p:
                                                                         ^
<instantiation>:18:5: error: invalid symbol redefinition
    .L0_repl_s1: call .; .L0_repl_e1:
    ^
<instantiation>:18:26: error: invalid symbol redefinition
    .L0_repl_s1: call .; .L0_repl_e1:
                         ^
<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) - (...
^
<instantiation>:1:37: error: invalid symbol redefinition
.L0_orig_s: call *genapic+64(%rip); .L0_orig_e: .L0_diff = (.L0_repl_e1 - .L0_repl_s1) - (...
                                    ^
<instantiation>:1:60: error: invalid reassignment of non-absolute variable '.L0_diff'
.L0_orig_s: call *genapic+64(%rip); .L0_orig_e: .L0_diff = (.L0_repl_e1 - .L0_repl_s1) - (...
                                                           ^
<inline asm>:1:2: note: while in macro instantiation
        ALTERNATIVE "call *genapic+64(%rip)", "call .", X86_FEATURE_LM
        ^
<instantiation>:1:156: error: invalid symbol redefinition
  ...- (.L0_orig_e - .L0_orig_s); mknops ((-(.L0_diff > 0)) * .L0_diff); .L0_orig_p:
                                                                         ^
<instantiation>:18:5: error: invalid symbol redefinition
    .L0_repl_s1: call .; .L0_repl_e1:
    ^
<instantiation>:18:26: error: invalid symbol redefinition
    .L0_repl_s1: call .; .L0_repl_e1:
                         ^
<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) - (...
^
<instantiation>:1:37: error: invalid symbol redefinition
.L0_orig_s: call *genapic+64(%rip); .L0_orig_e: .L0_diff = (.L0_repl_e1 - .L0_repl_s1) - (...
                                    ^
<instantiation>:1:60: error: invalid reassignment of non-absolute variable '.L0_diff'
.L0_orig_s: call *genapic+64(%rip); .L0_orig_e: .L0_diff = (.L0_repl_e1 - .L0_repl_s1) - (...
                                                           ^
<inline asm>:1:2: note: while in macro instantiation
        ALTERNATIVE "call *genapic+64(%rip)", "call .", X86_FEATURE_LM
        ^
<instantiation>:1:156: error: invalid symbol redefinition
  ...- (.L0_orig_e - .L0_orig_s); mknops ((-(.L0_diff > 0)) * .L0_diff); .L0_orig_p:
                                                                         ^
<instantiation>:18:5: error: invalid symbol redefinition
    .L0_repl_s1: call .; .L0_repl_e1:
    ^
<instantiation>:18:26: error: invalid symbol redefinition
    .L0_repl_s1: call .; .L0_repl_e1:
                         ^

Fixes: 67d01cdb5 ("x86: infrastructure to allow converting certain indirect calls to direct ones")
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: Wei Liu <wei.liu2@citrix.com>
---
Changes since v1:
 - Fix subject and commit message.
---
 xen/include/asm-x86/alternative.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/xen/include/asm-x86/alternative.h b/xen/include/asm-x86/alternative.h
index 63d0a450ba..92e3581bc2 100644
--- 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)               \
-- 
2.20.1 (Apple Git-117)


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] x86: fix alternative_callN usage of ALTERNATIVE asm macro
Posted by Jan Beulich 4 years, 10 months ago
>>> 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
Re: [Xen-devel] [PATCH v2] x86: fix alternative_callN usage of ALTERNATIVE asm macro
Posted by Roger Pau Monné 4 years, 10 months ago
On Thu, May 23, 2019 at 03:41:23AM -0600, Jan Beulich wrote:
> >>> 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.

Using the current assembly macros for inline asm alternatives would
regress the build on llvm based toolchains. If that's indeed the path
forward I will have to look into making those work in inline assembly
instances.

> > 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.

Yes, that's my understanding also. I've already filled a bug report:

https://bugs.llvm.org/show_bug.cgi?id=42034

> 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().)

IIRC I've fixed that one upstream quite some time ago, and should be
fixed in versions >= 6.

> 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.

I can look into workarounds to this, the one that comes to mind is
using .altmacro and LOCAL in order to create unique labels in the
macro. I can test if such approach would work if the plan is to only
rely on the assembly alternative code.

> 
> > --- 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.

Since the only change requested is related to the commit message,
would you be OK to update the commit message to:

---8<---
x86: remove alternative_callN usage of ALTERNATIVE asm macro

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, the more that using the
assembly macro in an inline assembly instance triggers the following
bug 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) - (...
^
<instantiation>:1:37: error: invalid symbol redefinition
.L0_orig_s: call *genapic+64(%rip); .L0_orig_e: .L0_diff = (.L0_repl_e1 - .L0_repl_s1) - (...
                                    ^
<instantiation>:1:60: error: invalid reassignment of non-absolute variable '.L0_diff'
.L0_orig_s: call *genapic+64(%rip); .L0_orig_e: .L0_diff = (.L0_repl_e1 - .L0_repl_s1) - (...
                                                           ^
<inline asm>:1:2: note: while in macro instantiation
        ALTERNATIVE "call *genapic+64(%rip)", "call .", X86_FEATURE_LM
        ^
<instantiation>:1:156: error: invalid symbol redefinition
  ...- (.L0_orig_e - .L0_orig_s); mknops ((-(.L0_diff > 0)) * .L0_diff); .L0_orig_p:
                                                                         ^
<instantiation>:18:5: error: invalid symbol redefinition
    .L0_repl_s1: call .; .L0_repl_e1:
    ^
<instantiation>:18:26: error: invalid symbol redefinition
    .L0_repl_s1: call .; .L0_repl_e1:
                         ^
<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) - (...
^
<instantiation>:1:37: error: invalid symbol redefinition
.L0_orig_s: call *genapic+64(%rip); .L0_orig_e: .L0_diff = (.L0_repl_e1 - .L0_repl_s1) - (...
                                    ^
<instantiation>:1:60: error: invalid reassignment of non-absolute variable '.L0_diff'
.L0_orig_s: call *genapic+64(%rip); .L0_orig_e: .L0_diff = (.L0_repl_e1 - .L0_repl_s1) - (...
                                                           ^
<inline asm>:1:2: note: while in macro instantiation
        ALTERNATIVE "call *genapic+64(%rip)", "call .", X86_FEATURE_LM
        ^
<instantiation>:1:156: error: invalid symbol redefinition
  ...- (.L0_orig_e - .L0_orig_s); mknops ((-(.L0_diff > 0)) * .L0_diff); .L0_orig_p:
                                                                         ^
<instantiation>:18:5: error: invalid symbol redefinition
    .L0_repl_s1: call .; .L0_repl_e1:
    ^
<instantiation>:18:26: error: invalid symbol redefinition
    .L0_repl_s1: call .; .L0_repl_e1:
                         ^
<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) - (...
^
<instantiation>:1:37: error: invalid symbol redefinition
.L0_orig_s: call *genapic+64(%rip); .L0_orig_e: .L0_diff = (.L0_repl_e1 - .L0_repl_s1) - (...
                                    ^
<instantiation>:1:60: error: invalid reassignment of non-absolute variable '.L0_diff'
.L0_orig_s: call *genapic+64(%rip); .L0_orig_e: .L0_diff = (.L0_repl_e1 - .L0_repl_s1) - (...
                                                           ^
<inline asm>:1:2: note: while in macro instantiation
        ALTERNATIVE "call *genapic+64(%rip)", "call .", X86_FEATURE_LM
        ^
<instantiation>:1:156: error: invalid symbol redefinition
  ...- (.L0_orig_e - .L0_orig_s); mknops ((-(.L0_diff > 0)) * .L0_diff); .L0_orig_p:
                                                                         ^
<instantiation>:18:5: error: invalid symbol redefinition
    .L0_repl_s1: call .; .L0_repl_e1:
    ^
<instantiation>:18:26: error: invalid symbol redefinition
    .L0_repl_s1: call .; .L0_repl_e1:
                         ^

This is a bug in llvm that needs to be fixed before switching to use
the alternative assembly macros in inline assembly call sites.

Fixes: 67d01cdb5 ("x86: infrastructure to allow converting certain indirect calls to direct ones")
Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] x86: fix alternative_callN usage of ALTERNATIVE asm macro
Posted by Jan Beulich 4 years, 10 months ago
>>> On 27.05.19 at 14:39, <roger.pau@citrix.com> wrote:
> On Thu, May 23, 2019 at 03:41:23AM -0600, Jan Beulich wrote:
>> >>> 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.
> 
> Using the current assembly macros for inline asm alternatives would
> regress the build on llvm based toolchains. If that's indeed the path
> forward I will have to look into making those work in inline assembly
> instances.

Well, I'm open to arguments to the contrary (i.e. supporting the
current redundancy).

>> > 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.
> 
> Yes, that's my understanding also. I've already filled a bug report:
> 
> https://bugs.llvm.org/show_bug.cgi?id=42034 
> 
>> 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().)
> 
> IIRC I've fixed that one upstream quite some time ago, and should be
> fixed in versions >= 6.

Yet I understand we want to support older versions as well.

>> 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.
> 
> I can look into workarounds to this, the one that comes to mind is
> using .altmacro and LOCAL in order to create unique labels in the
> macro. I can test if such approach would work if the plan is to only
> rely on the assembly alternative code.

I'm surprised they support .altmacro. I wonder whether, as an
alternative, there wouldn't be a way to substitute the (asssembler
expaned) \@ for the (compiler expanded) %= when using the
macros from asm().

>> > --- 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.
> 
> Since the only change requested is related to the commit message,
> would you be OK to update the commit message to:
> 
> ---8<---
> x86: remove alternative_callN usage of ALTERNATIVE asm macro
> 
> 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, the more that using the
> assembly macro in an inline assembly instance triggers the following
> bug on llvm based toolchains:

Well, this still makes it sound as if the issue was a shortcoming of the
commit in question. How about pulling up the paragraph further down
ahead of the text above, slightly adjusted to

"There is a bug in llvm that needs to be fixed before switching to use
 the alternative assembly macros in inline assembly call sites. Therefore
 ..."

(perhaps also replacing "the more" then)?

Jan

> <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) - (...
> ^
> <instantiation>:1:37: error: invalid symbol redefinition
> .L0_orig_s: call *genapic+64(%rip); .L0_orig_e: .L0_diff = (.L0_repl_e1 - 
> .L0_repl_s1) - (...
>                                     ^
> <instantiation>:1:60: error: invalid reassignment of non-absolute variable 
> '.L0_diff'
> .L0_orig_s: call *genapic+64(%rip); .L0_orig_e: .L0_diff = (.L0_repl_e1 - 
> .L0_repl_s1) - (...
>                                                            ^
> <inline asm>:1:2: note: while in macro instantiation
>         ALTERNATIVE "call *genapic+64(%rip)", "call .", X86_FEATURE_LM
>         ^
> <instantiation>:1:156: error: invalid symbol redefinition
>   ...- (.L0_orig_e - .L0_orig_s); mknops ((-(.L0_diff > 0)) * .L0_diff); 
> .L0_orig_p:
>                                                                          ^
> <instantiation>:18:5: error: invalid symbol redefinition
>     .L0_repl_s1: call .; .L0_repl_e1:
>     ^
> <instantiation>:18:26: error: invalid symbol redefinition
>     .L0_repl_s1: call .; .L0_repl_e1:
>                          ^
> <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) - (...
> ^
> <instantiation>:1:37: error: invalid symbol redefinition
> .L0_orig_s: call *genapic+64(%rip); .L0_orig_e: .L0_diff = (.L0_repl_e1 - 
> .L0_repl_s1) - (...
>                                     ^
> <instantiation>:1:60: error: invalid reassignment of non-absolute variable 
> '.L0_diff'
> .L0_orig_s: call *genapic+64(%rip); .L0_orig_e: .L0_diff = (.L0_repl_e1 - 
> .L0_repl_s1) - (...
>                                                            ^
> <inline asm>:1:2: note: while in macro instantiation
>         ALTERNATIVE "call *genapic+64(%rip)", "call .", X86_FEATURE_LM
>         ^
> <instantiation>:1:156: error: invalid symbol redefinition
>   ...- (.L0_orig_e - .L0_orig_s); mknops ((-(.L0_diff > 0)) * .L0_diff); 
> .L0_orig_p:
>                                                                          ^
> <instantiation>:18:5: error: invalid symbol redefinition
>     .L0_repl_s1: call .; .L0_repl_e1:
>     ^
> <instantiation>:18:26: error: invalid symbol redefinition
>     .L0_repl_s1: call .; .L0_repl_e1:
>                          ^
> <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) - (...
> ^
> <instantiation>:1:37: error: invalid symbol redefinition
> .L0_orig_s: call *genapic+64(%rip); .L0_orig_e: .L0_diff = (.L0_repl_e1 - 
> .L0_repl_s1) - (...
>                                     ^
> <instantiation>:1:60: error: invalid reassignment of non-absolute variable 
> '.L0_diff'
> .L0_orig_s: call *genapic+64(%rip); .L0_orig_e: .L0_diff = (.L0_repl_e1 - 
> .L0_repl_s1) - (...
>                                                            ^
> <inline asm>:1:2: note: while in macro instantiation
>         ALTERNATIVE "call *genapic+64(%rip)", "call .", X86_FEATURE_LM
>         ^
> <instantiation>:1:156: error: invalid symbol redefinition
>   ...- (.L0_orig_e - .L0_orig_s); mknops ((-(.L0_diff > 0)) * .L0_diff); 
> .L0_orig_p:
>                                                                          ^
> <instantiation>:18:5: error: invalid symbol redefinition
>     .L0_repl_s1: call .; .L0_repl_e1:
>     ^
> <instantiation>:18:26: error: invalid symbol redefinition
>     .L0_repl_s1: call .; .L0_repl_e1:
>                          ^
> 
> This is a bug in llvm that needs to be fixed before switching to use
> the alternative assembly macros in inline assembly call sites.
> 
> Fixes: 67d01cdb5 ("x86: infrastructure to allow converting certain indirect calls to direct ones")
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] x86: fix alternative_callN usage of ALTERNATIVE asm macro
Posted by Roger Pau Monné 4 years, 10 months ago
On Mon, May 27, 2019 at 07:15:27AM -0600, Jan Beulich wrote:
> >>> On 27.05.19 at 14:39, <roger.pau@citrix.com> wrote:
> > On Thu, May 23, 2019 at 03:41:23AM -0600, Jan Beulich wrote:
> >> >>> 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.
> > 
> > Using the current assembly macros for inline asm alternatives would
> > regress the build on llvm based toolchains. If that's indeed the path
> > forward I will have to look into making those work in inline assembly
> > instances.
> 
> Well, I'm open to arguments to the contrary (i.e. supporting the
> current redundancy).

IIRC Andrew told me there where also issues with using the current asm
macros with GNU based toolchains, albeit I don't have any specific
data of what the issues actually are.

> >> 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.
> > 
> > I can look into workarounds to this, the one that comes to mind is
> > using .altmacro and LOCAL in order to create unique labels in the
> > macro. I can test if such approach would work if the plan is to only
> > rely on the assembly alternative code.
> 
> I'm surprised they support .altmacro.

.altmacro is supported by llvm, but it's still missing the LOCAL
directive, so my suggestion in the previous email is a no-go.

> I wonder whether, as an
> alternative, there wouldn't be a way to substitute the (asssembler
> expaned) \@ for the (compiler expanded) %= when using the
> macros from asm().

Maybe. TBH I don't see an obvious way to do this ATM. The alternative
asm macros are included using an inline assembly .include directive,
which means the file doesn't go through the preprocessor, leaving less
room to perform such substitutions.

> >> > --- 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.
> > 
> > Since the only change requested is related to the commit message,
> > would you be OK to update the commit message to:
> > 
> > ---8<---
> > x86: remove alternative_callN usage of ALTERNATIVE asm macro
> > 
> > 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, the more that using the
> > assembly macro in an inline assembly instance triggers the following
> > bug on llvm based toolchains:
> 
> Well, this still makes it sound as if the issue was a shortcoming of the
> commit in question. How about pulling up the paragraph further down
> ahead of the text above, slightly adjusted to
> 
> "There is a bug in llvm that needs to be fixed before switching to use
>  the alternative assembly macros in inline assembly call sites. Therefore
>  ..."
> 
> (perhaps also replacing "the more" then)?

Yes, I would s/, the more that u/. U/

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v2] x86: fix alternative_callN usage of ALTERNATIVE asm macro
Posted by Jan Beulich 4 years, 10 months ago
>>> On 27.05.19 at 16:25, <roger.pau@citrix.com> wrote:
> On Mon, May 27, 2019 at 07:15:27AM -0600, Jan Beulich wrote:
>> >>> On 27.05.19 at 14:39, <roger.pau@citrix.com> wrote:
>> > On Thu, May 23, 2019 at 03:41:23AM -0600, Jan Beulich wrote:
>> >> >>> 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.
>> > 
>> > Using the current assembly macros for inline asm alternatives would
>> > regress the build on llvm based toolchains. If that's indeed the path
>> > forward I will have to look into making those work in inline assembly
>> > instances.
>> 
>> Well, I'm open to arguments to the contrary (i.e. supporting the
>> current redundancy).
> 
> IIRC Andrew told me there where also issues with using the current asm
> macros with GNU based toolchains, albeit I don't have any specific
> data of what the issues actually are.

I'm not sure his wording is to the point. Quoting the respective Linux
commit:

"The macro based workarounds for GCC's inlining bugs caused
 regressions: distcc and other distro build setups broke, and the fixes
 are not easy nor will they solve regressions on already existing
 installations."

To me this doesn't sound like issues with the base tool chain itself.
Also their point of wanting to go the "asm inline()" route anyway
isn't really to the point here: While that will achieve the goal of
the series that was reverted, it won't address the duplication of
logic.

>> I wonder whether, as an
>> alternative, there wouldn't be a way to substitute the (asssembler
>> expaned) \@ for the (compiler expanded) %= when using the
>> macros from asm().
> 
> Maybe. TBH I don't see an obvious way to do this ATM. The alternative
> asm macros are included using an inline assembly .include directive,
> which means the file doesn't go through the preprocessor, leaving less
> room to perform such substitutions.

Right, this wouldn't be straightforward at all.

>> >> > --- 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.
>> > 
>> > Since the only change requested is related to the commit message,
>> > would you be OK to update the commit message to:
>> > 
>> > ---8<---
>> > x86: remove alternative_callN usage of ALTERNATIVE asm macro
>> > 
>> > 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, the more that using the
>> > assembly macro in an inline assembly instance triggers the following
>> > bug on llvm based toolchains:
>> 
>> Well, this still makes it sound as if the issue was a shortcoming of the
>> commit in question. How about pulling up the paragraph further down
>> ahead of the text above, slightly adjusted to
>> 
>> "There is a bug in llvm that needs to be fixed before switching to use
>>  the alternative assembly macros in inline assembly call sites. Therefore
>>  ..."
>> 
>> (perhaps also replacing "the more" then)?
> 
> Yes, I would s/, the more that u/. U/

Yes, I think I'd be fine with the result.

Jan


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