Re: [Xen-devel] [PATCH 5/7] x86/alt: Support for automatic padding calculations

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

On Tue, Feb 13, 2018 at 10:09:15AM +0000, Andrew Cooper wrote:
> On 13/02/2018 09:45, Roger Pau Monné wrote:
> > On Mon, Feb 12, 2018 at 11:23:05AM +0000, Andrew Cooper wrote:
> >>  .macro ALTERNATIVE oldinstr, newinstr, feature
> >>  .L\@_orig_s:
> >>      \oldinstr
> >>  .L\@_orig_e:
> >> +     .skip (-((repl_len(1) - orig_len) > 0) * (repl_len(1) - orig_len)), 
> >> 0x90
> > clang chokes on this expression, because of the negation at the
> > beginning and I'm also failing to see why are you adding such
> > negation. AFAICT using:
> >
> > .skip (((repl_len(1) - orig_len) > 0) * (repl_len(1) - orig_len)), 0x90
> >
> > Is correct: it adds the right padding if the alternative code is
> > bigger than the original one, while not adding anything is the
> > original code is greater than the alternative one.
> >
> > The negation just turns the 1 to -1, thus converting the result of the
> > whole expression into a negative value.
> 
> /sigh so Clang and GAS have different ideas of true.
> 
> The reason for this negation is stated in the commit message.  "x > 0"
> in GAS yields 0 or -1, rather than the expected 1.

That's unfortunate. What about something along the lines of:

---8<---
diff --git a/xen/arch/x86/Rules.mk b/xen/arch/x86/Rules.mk
index aeae01cd97..db442a45b7 100644
--- a/xen/arch/x86/Rules.mk
+++ b/xen/arch/x86/Rules.mk
@@ -23,6 +23,7 @@ $(call as-insn-check,CFLAGS,CC,"rdseed 
%eax",-DHAVE_GAS_RDSEED)
 $(call as-insn-check,CFLAGS,CC,".equ \"x\"$$(comma)1", \
                      -U__OBJECT_LABEL__ -DHAVE_GAS_QUOTED_SYM \
                      '-D__OBJECT_LABEL__=$(subst $(BASEDIR)/,,$(CURDIR))/$$@')
+$(call as-insn-check,CFLAGS,CC,".skip (-(1 > 
0))$$(comma)0x90",-DAS_NEGATIVE_TRUE)
 
 CFLAGS += -mno-red-zone -fpic -fno-asynchronous-unwind-tables
 
diff --git a/xen/include/asm-x86/alternative-asm.h 
b/xen/include/asm-x86/alternative-asm.h
index f7e37cb891..6ce6479e5b 100644
--- a/xen/include/asm-x86/alternative-asm.h
+++ b/xen/include/asm-x86/alternative-asm.h
@@ -25,11 +25,18 @@
 #define decl_repl(insn, nr)     .L\@_repl_s\()nr: insn; .L\@_repl_e\()nr:
 #define gas_max(a, b)          ((a) ^ (((a) ^ (b)) & -(-((a) < (b)))))
 
+#ifdef AS_NEGATIVE_TRUE
+#define as_true -
+#else
+#define as_true
+#endif
+
 .macro ALTERNATIVE oldinstr, newinstr, feature
 .L\@_orig_s:
     \oldinstr
 .L\@_orig_e:
-     .skip (-((repl_len(1) - orig_len) > 0) * (repl_len(1) - orig_len)), 0x90
+     .skip (as_true((repl_len(1) - orig_len) > 0) * (repl_len(1) - orig_len)), 
\
+           0x90
 .L\@_orig_p:
 
     .pushsection .altinstructions, "a", @progbits
@@ -56,8 +63,8 @@
 .L\@_orig_s:
     \oldinstr
 .L\@_orig_e:
-    .skip (-((gas_max(repl_len(1), repl_len(2)) - orig_len) > 0) * \
-             (gas_max(repl_len(1), repl_len(2)) - orig_len)), 0x90
+    .skip (as_true((gas_max(repl_len(1), repl_len(2)) - orig_len) > 0) * \
+           (gas_max(repl_len(1), repl_len(2)) - orig_len)), 0x90
 .L\@_orig_p:
 
     .pushsection .altinstructions, "a", @progbits
diff --git a/xen/include/asm-x86/alternative.h 
b/xen/include/asm-x86/alternative.h
index 20dea2245a..ea76fa9f8d 100644
--- a/xen/include/asm-x86/alternative.h
+++ b/xen/include/asm-x86/alternative.h
@@ -37,19 +37,25 @@ extern void alternative_instructions(void);
 #define gas_max(a, b)                                         \
     "((" a ") ^ (((" a ") ^ (" b ")) & -(-((" a ") < (" b ")))))"
 
-#define OLDINSTR_1(oldinstr, n1)                              \
-    ".L%=_orig_s:\n\t" oldinstr "\n .L%=_orig_e:\n\t"         \
-    ".skip (-(("alt_repl_len(n1)"-"alt_orig_len") > 0) * "    \
-             "("alt_repl_len(n1)"-"alt_orig_len")), 0x90\n\t" \
+#ifdef AS_NEGATIVE_TRUE
+#define as_true -
+#else
+#define as_true
+#endif
+
+#define OLDINSTR_1(oldinstr, n1)                                        \
+    ".L%=_orig_s:\n\t" oldinstr "\n .L%=_orig_e:\n\t"                   \
+    ".skip ("as_true"(("alt_repl_len(n1)"-"alt_orig_len") > 0) * "      \
+             "("alt_repl_len(n1)"-"alt_orig_len")), 0x90\n\t"           \
     ".L%=_orig_p:\n\t"
 
 #define ALT_PADDING_LEN(n1, n2) \
     gas_max((alt_repl_len(n1), alt_repl_len(n2))"-"alt_orig_len
 
-#define OLDINSTR_2(oldinstr, n1, n2)                          \
-    ".L%=_orig_s:\n\t" oldinstr "\n .L%=_orig_e:\n\t"         \
-    ".skip (-(("ALT_PADDING_LEN(n1, n2)") > 0) * "            \
-             "("ALT_PADDING_LEN(n1, n2)")), 0x90\n\t"         \
+#define OLDINSTR_2(oldinstr, n1, n2)                                    \
+    ".L%=_orig_s:\n\t" oldinstr "\n .L%=_orig_e:\n\t"                   \
+    ".skip ("as_true"(("ALT_PADDING_LEN(n1, n2)") > 0) * "              \
+             "("ALT_PADDING_LEN(n1, n2)")), 0x90\n\t"                   \
     ".L%=_orig_p:\n\t"
 
 #define ALTINSTR_ENTRY(feature, num)                                    \


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