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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, May 27, 2019 at 07:15:27AM -0600, Jan Beulich wrote:
> >>> On 27.05.19 at 14:39, <roger.pau@xxxxxxxxxx> wrote:
> > On Thu, May 23, 2019 at 03:41:23AM -0600, Jan Beulich wrote:
> >> >>> On 22.05.19 at 18:45, <roger.pau@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel