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

["Jan Beulich" <JBeulich@suse.com>]

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