xen/include/asm-x86/alternative.h | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-)
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
>>> 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
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
>>> 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
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
>>> 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
© 2016 - 2024 Red Hat, Inc.