From: "Roger Pau Monné" <roger.pau@citrix.com> To: Jan Beulich <JBeulich@suse.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com>, Wei Liu <wei.liu2@citrix.com>, xen-devel <xen-devel@lists.xenproject.org> Subject: Re: [PATCH v2] x86: fix alternative_callN usage of ALTERNATIVE asm macro Date: Mon, 27 May 2019 16:25:09 +0200 [thread overview] Message-ID: <20190527142509.kjbilbhqkx5vwv7n@Air-de-Roger> (raw) In-Reply-To: <5CEBE2EF0200007800232C94@prv1-mh.provo.novell.com> 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
WARNING: multiple messages have this Message-ID (diff)
From: "Roger Pau Monné" <roger.pau@citrix.com> To: Jan Beulich <JBeulich@suse.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: Mon, 27 May 2019 16:25:09 +0200 [thread overview] Message-ID: <20190527142509.kjbilbhqkx5vwv7n@Air-de-Roger> (raw) Message-ID: <20190527142509.plJ-9J0kVeSHezuPS3oI97HNVEBCes0rguiM_WsqTmM@z> (raw) In-Reply-To: <5CEBE2EF0200007800232C94@prv1-mh.provo.novell.com> 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
next prev parent reply other threads:[~2019-05-27 14:25 UTC|newest] Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-05-22 16:45 [PATCH v2] x86: fix alternative_callN usage of ALTERNATIVE asm macro Roger Pau Monne 2019-05-22 16:45 ` [Xen-devel] " Roger Pau Monne 2019-05-23 9:41 ` Jan Beulich 2019-05-23 9:41 ` [Xen-devel] " Jan Beulich 2019-05-27 12:39 ` Roger Pau Monné 2019-05-27 12:39 ` [Xen-devel] " Roger Pau Monné 2019-05-27 13:15 ` Jan Beulich 2019-05-27 13:15 ` [Xen-devel] " Jan Beulich 2019-05-27 14:25 ` Roger Pau Monné [this message] 2019-05-27 14:25 ` Roger Pau Monné 2019-05-27 15:02 ` Jan Beulich 2019-05-27 15:02 ` [Xen-devel] " Jan Beulich
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20190527142509.kjbilbhqkx5vwv7n@Air-de-Roger \ --to=roger.pau@citrix.com \ --cc=JBeulich@suse.com \ --cc=andrew.cooper3@citrix.com \ --cc=wei.liu2@citrix.com \ --cc=xen-devel@lists.xenproject.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).