[PATCH v3 0/5] clang fixes

[PATCH v3 0/5] clang fixes

[Roger Pau Monne]

Hello,

The first 3 patches in this series restore the usage of
-no-integrated-as with clang only for assembly files unless it's
strictly needed.

Then patches 4 and 5 allow to get rid of -no-integrated-as even for
assembly files when using clang, thus being able to fully compile Xen
using clang's integrated assembler. Fully compiling Xen with clang's
integrated assembler will require clang 4.0 or newer.

This series has been tested with clang 3.5, clang 6.0 and gcc 6.4.0.

Thanks, Roger.

Roger Pau Monne (5):
  build: filter out command line assembler arguments
  x86/clang: fix build with indirect thunks
  x86: fix indirect thunk usage of CONFIG_INDIRECT_THUNK
  x86: move declaration of the exception_table to C
  x86: remove usage of .skip with non-absolute expressions

 Config.mk                              |  7 +++----
 xen/Rules.mk                           |  3 ---
 xen/arch/x86/Makefile                  |  6 +++---
 xen/arch/x86/Rules.mk                  | 17 ++++++++++++++---
 xen/arch/x86/extable.c                 |  3 ++-
 xen/arch/x86/traps.c                   | 32 ++++++++++++++++++++++++++++++--
 xen/arch/x86/x86_64/compat/entry.S     |  9 ++++++++-
 xen/arch/x86/x86_64/entry.S            | 32 +-------------------------------
 xen/arch/x86/x86_emulate/x86_emulate.c |  3 ++-
 xen/common/wait.c                      |  1 +
 xen/include/Makefile                   |  2 +-
 xen/include/asm-x86/asm_defns.h        | 31 ++++++++++++++++++++++++++++---
 xen/include/asm-x86/processor.h        |  1 -
 13 files changed, 93 insertions(+), 54 deletions(-)

-- 
2.15.1


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

[PATCH v3 1/5] build: filter out command line assembler arguments

[Roger Pau Monne]

If the assembler is not used. This happens when using cc -E or cc -S
for example. GCC will just ignore the -Wa,... when the assembler is
not called, but clang will complain loudly and fail.

This is a preparatory change in order to pass assembler arguments when
using clang.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 xen/arch/x86/Makefile | 6 +++---
 xen/include/Makefile  | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index d903b7abb9..021a4ba27e 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -215,15 +215,15 @@ efi/boot.init.o efi/runtime.o efi/compat.o efi/buildid.o: $(BASEDIR)/arch/x86/ef
 efi/boot.init.o efi/runtime.o efi/compat.o efi/buildid.o: ;
 
 asm-offsets.s: $(TARGET_SUBARCH)/asm-offsets.c
-	$(CC) $(filter-out -flto,$(CFLAGS)) -S -o $@ $<
+	$(CC) $(filter-out -Wa% -flto,$(CFLAGS)) -S -o $@ $<
 
 xen.lds: xen.lds.S
-	$(CC) -P -E -Ui386 $(AFLAGS) -o $@ $<
+	$(CC) -P -E -Ui386 $(filter-out -Wa%,$(AFLAGS)) -o $@ $<
 	sed -e 's/xen\.lds\.o:/xen\.lds:/g' <.xen.lds.d >.xen.lds.d.new
 	mv -f .xen.lds.d.new .xen.lds.d
 
 efi.lds: xen.lds.S
-	$(CC) -P -E -Ui386 -DEFI $(AFLAGS) -o $@ $<
+	$(CC) -P -E -Ui386 -DEFI $(filter-out -Wa%,$(AFLAGS)) -o $@ $<
 	sed -e 's/efi\.lds\.o:/efi\.lds:/g' <.$(@F).d >.$(@F).d.new
 	mv -f .$(@F).d.new .$(@F).d
 
diff --git a/xen/include/Makefile b/xen/include/Makefile
index 19066a33a0..094726aab5 100644
--- a/xen/include/Makefile
+++ b/xen/include/Makefile
@@ -65,7 +65,7 @@ compat/%.h: compat/%.i Makefile $(BASEDIR)/tools/compat-build-header.py
 	mv -f $@.new $@
 
 compat/%.i: compat/%.c Makefile
-	$(CPP) $(filter-out -M% %.d -include %/include/xen/config.h,$(CFLAGS)) $(cppflags-y) -o $@ $<
+	$(CPP) $(filter-out -Wa% -M% %.d -include %/include/xen/config.h,$(CFLAGS)) $(cppflags-y) -o $@ $<
 
 compat/%.c: public/%.h xlat.lst Makefile $(BASEDIR)/tools/compat-build-source.py
 	mkdir -p $(@D)
-- 
2.15.1


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

[PATCH v3 2/5] x86/clang: fix build with indirect thunks

[Roger Pau Monne]

The build with clang is currently broken because clang integrated
assembler requires asm macros to be declared inside the same inline
asm declaration where they are used.

In order to fix this always include indirect_thunk_asm.h in the same
asm declaration where it's being used.

This has been reported to upstream clang at:

https://bugs.llvm.org/show_bug.cgi?id=36110

This change has been tested with clang 3.5, clang 6.0 and gcc 6.4.0.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>
---
Changes since v1:
 - Use as-insn to check if the assembler supports .include.
 - Open code a check for whether the assembler forgets .macro-s
   between asm()-s.
---
 Config.mk                              |  7 +++----
 xen/Rules.mk                           |  6 ++++--
 xen/arch/x86/Rules.mk                  | 17 ++++++++++++++---
 xen/arch/x86/extable.c                 |  3 ++-
 xen/arch/x86/x86_emulate/x86_emulate.c |  3 ++-
 xen/common/wait.c                      |  1 +
 xen/include/asm-x86/asm_defns.h        | 28 +++++++++++++++++++++++++---
 7 files changed, 51 insertions(+), 14 deletions(-)

diff --git a/Config.mk b/Config.mk
index 51adc27d83..c55f023f35 100644
--- a/Config.mk
+++ b/Config.mk
@@ -157,17 +157,16 @@ ifndef XEN_HAS_CHECKPOLICY
 endif
 
 # as-insn: Check whether assembler supports an instruction.
-# Usage: cflags-y += $(call as-insn "insn",option-yes,option-no)
+# Usage: cflags-y += $(call as-insn cc,"insn",option-yes,option-no,FLAGS)
 as-insn = $(if $(shell echo 'void _(void) { asm volatile ( $(2) ); }' \
-                       | $(1) $(filter-out -M% %.d -include %/include/xen/config.h,$(AFLAGS)) \
+                       | $(1) $(filter-out -M% %.d -include %/include/xen/config.h,$(5)) \
                               -c -x c -o /dev/null - 2>&1),$(4),$(3))
-
 # as-insn-check: Add an option to compilation flags, but only if insn is
 #                supported by assembler.
 # Usage: $(call as-insn-check CFLAGS,CC,"nop",-DHAVE_GAS_NOP)
 as-insn-check = $(eval $(call as-insn-check-closure,$(1),$(2),$(3),$(4)))
 define as-insn-check-closure
-    ifeq ($$(call as-insn,$$($(2)),$(3),y,n),y)
+    ifeq ($$(call as-insn,$$($(2)),$(3),y,n,$$(AFLAGS)),y)
         $(1) += $(4)
     endif
 endef
diff --git a/xen/Rules.mk b/xen/Rules.mk
index 3cf40754a6..205f0aff30 100644
--- a/xen/Rules.mk
+++ b/xen/Rules.mk
@@ -66,8 +66,10 @@ endif
 
 AFLAGS-y                += -D__ASSEMBLY__
 
-# Clang's built-in assembler can't handle embedded .include's
-CFLAGS-$(clang)         += -no-integrated-as
+# Clang's built-in assembler doesn't understand .skip or .rept assembler
+# directives without an absolute value:
+# https://bugs.llvm.org/show_bug.cgi?id=27369
+AFLAGS-$(clang)         += -no-integrated-as
 
 ALL_OBJS := $(ALL_OBJS-y)
 
diff --git a/xen/arch/x86/Rules.mk b/xen/arch/x86/Rules.mk
index 56b2ea8356..aeae01cd97 100644
--- a/xen/arch/x86/Rules.mk
+++ b/xen/arch/x86/Rules.mk
@@ -42,8 +42,19 @@ CFLAGS += -DCONFIG_INDIRECT_THUNK
 export CONFIG_INDIRECT_THUNK=y
 endif
 
-# Set up the assembler include path properly for older GCC toolchains.  Clang
-# objects to the agument being passed however.
-ifneq ($(clang),y)
+# Set up the assembler include path properly for older toolchains.
 CFLAGS += -Wa,-I$(BASEDIR)/include
+
+ifeq ($(clang),y)
+    # Check whether clang asm()-s support .include.
+    ifeq ($(call as-insn,$(CC),".include \"asm/indirect_thunk_asm.h\"",y,n,$(CFLAGS)),n)
+        CFLAGS += -no-integrated-as
+    endif
+    # Check if the assembler keeps .macro-s between asm()-s.
+    ifeq ($(if $(shell echo 'void _(void) { asm volatile ( ".macro FOO\n.endm" ); \
+                                            asm volatile ( ".macro FOO\n.endm" ); }' \
+                       | $(CC) $(filter-out -M% %.d -include %/include/xen/config.h,$(CFLAGS)) \
+                              -c -x c -o /dev/null - 2>&1),n,y),y)
+        CFLAGS += -DCONFIG_INCLUDE_THUNK_INLINE
+    endif
 endif
diff --git a/xen/arch/x86/extable.c b/xen/arch/x86/extable.c
index 72f30d9060..30893c3770 100644
--- a/xen/arch/x86/extable.c
+++ b/xen/arch/x86/extable.c
@@ -158,7 +158,8 @@ static int __init stub_selftest(void)
         memcpy(ptr, tests[i].opc, ARRAY_SIZE(tests[i].opc));
         unmap_domain_page(ptr);
 
-        asm volatile ( "INDIRECT_CALL %[stb]\n"
+        asm volatile ( INCLUDE_INDIRECT_THUNK
+                       "INDIRECT_CALL %[stb]\n"
                        ".Lret%=:\n\t"
                        ".pushsection .fixup,\"ax\"\n"
                        ".Lfix%=:\n\t"
diff --git a/xen/arch/x86/x86_emulate/x86_emulate.c b/xen/arch/x86/x86_emulate/x86_emulate.c
index ff0a003902..e525e6bb0d 100644
--- a/xen/arch/x86/x86_emulate/x86_emulate.c
+++ b/xen/arch/x86/x86_emulate/x86_emulate.c
@@ -867,7 +867,8 @@ static inline int mkec(uint8_t e, int32_t ec, ...)
 #ifdef __XEN__
 # define invoke_stub(pre, post, constraints...) do {                    \
     union stub_exception_token res_ = { .raw = ~0 };                    \
-    asm volatile ( pre "\n\tINDIRECT_CALL %[stub]\n\t" post "\n"        \
+    asm volatile ( INCLUDE_INDIRECT_THUNK                               \
+                   pre "\n\tINDIRECT_CALL %[stub]\n\t" post "\n"        \
                    ".Lret%=:\n\t"                                       \
                    ".pushsection .fixup,\"ax\"\n"                       \
                    ".Lfix%=:\n\t"                                       \
diff --git a/xen/common/wait.c b/xen/common/wait.c
index a57bc10d61..91dcd46342 100644
--- a/xen/common/wait.c
+++ b/xen/common/wait.c
@@ -207,6 +207,7 @@ void check_wakeup_from_wait(void)
      * restored from the stack, so are available for use here.
      */
     asm volatile (
+        INCLUDE_INDIRECT_THUNK
         "mov %1,%%"__OP"sp; INDIRECT_JMP %[ip]"
         : : "S" (wqv->stack), "D" (wqv->esp),
           "c" ((char *)get_cpu_info() - (char *)wqv->esp),
diff --git a/xen/include/asm-x86/asm_defns.h b/xen/include/asm-x86/asm_defns.h
index d2d91ca1fa..447bf67aed 100644
--- a/xen/include/asm-x86/asm_defns.h
+++ b/xen/include/asm-x86/asm_defns.h
@@ -16,9 +16,31 @@
 #ifdef __ASSEMBLY__
 # include <asm/indirect_thunk_asm.h>
 #else
-asm ( "\t.equ CONFIG_INDIRECT_THUNK, "
-      __stringify(IS_ENABLED(CONFIG_INDIRECT_THUNK)) );
-asm ( "\t.include \"asm/indirect_thunk_asm.h\"" );
+/*
+ * NB: for clang's integrated assembler the macro must be included in every
+ * asm() instance where it's used. For gcc/gas it only needs to be included
+ * once per file. Note that even guarding the indirect_thunk_asm.h with
+ * something like:
+ *
+ * .ifndef _X86_INDIRECT_THUNK_ASM_H_
+ * .equ _X86_INDIRECT_THUNK_ASM_H_, 1
+ * ...
+ * .endif
+ *
+ * Doesn't work with clang because it will keep the .equ defined between asm()
+ * instances, but will forget the .macro. This has been reported upstream:
+ * https://bugs.llvm.org/show_bug.cgi?id=36110
+ */
+# define INDIRECT_THUNK_ASM                                     \
+    "\t.equ CONFIG_INDIRECT_THUNK, "                            \
+    __stringify(IS_ENABLED(CONFIG_INDIRECT_THUNK)) "\n"         \
+    "\t.include \"asm/indirect_thunk_asm.h\"\n"
+# ifdef CONFIG_INCLUDE_THUNK_INLINE
+#  define INCLUDE_INDIRECT_THUNK INDIRECT_THUNK_ASM
+# else
+   asm ( INDIRECT_THUNK_ASM );
+#  define INCLUDE_INDIRECT_THUNK
+# endif
 #endif
 
 #ifndef __ASSEMBLY__
-- 
2.15.1


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

[PATCH v3 3/5] x86: fix indirect thunk usage of CONFIG_INDIRECT_THUNK

[Roger Pau Monne]

When indirect_thunk_asm.h is instantiated directly into assembly files
CONFIG_INDIRECT_THUNK might not be defined, and thus using .if against
it is wrong.

Add a check to define CONFIG_INDIRECT_THUNK to 0 if not defined, so
that using .if CONFIG_INDIRECT_THUNK is always correct.

This suppresses the following clang error:

<instantiation>:8:9: error: expected absolute expression
    .if CONFIG_INDIRECT_THUNK == 1
        ^
<instantiation>:1:1: note: while in macro instantiation
INDIRECT_BRANCH call %rdx
^
entry.S:589:9: note: while in macro instantiation
        INDIRECT_CALL %rdx
        ^

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/include/asm-x86/asm_defns.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/xen/include/asm-x86/asm_defns.h b/xen/include/asm-x86/asm_defns.h
index 447bf67aed..aa32f10967 100644
--- a/xen/include/asm-x86/asm_defns.h
+++ b/xen/include/asm-x86/asm_defns.h
@@ -14,6 +14,9 @@
 #include <asm/alternative.h>
 
 #ifdef __ASSEMBLY__
+# ifndef CONFIG_INDIRECT_THUNK
+#  define CONFIG_INDIRECT_THUNK 0
+# endif
 # include <asm/indirect_thunk_asm.h>
 #else
 /*
-- 
2.15.1


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

[PATCH v3 4/5] x86: move declaration of the exception_table to C

[Roger Pau Monne]

This makes the code cleaner because there's no need to declare the
exception_table in assembly, and also fixes the following error when
using clang's integrated assembler:

entry.S:834:15: error: unexpected token in '.rept' directive
        .rept 32 - ((. - exception_table) / 8)
              ^
entry.S:836:14: error: unmatched '.endr' directive
        .endr
             ^

This should be a non-functional change.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
Changes since v2:
 - Make do_trap/do_reserved_trap static.
 - Only use '...' to initialize the end of the exception_table array.

Changes since v1:
 - Cast to void *.
 - Align assignments.
 - Use ARRAY_SIZE instead of TRAP_nr.
---
 xen/arch/x86/traps.c            | 32 ++++++++++++++++++++++++++++++--
 xen/arch/x86/x86_64/entry.S     | 30 ------------------------------
 xen/include/asm-x86/processor.h |  1 -
 3 files changed, 30 insertions(+), 33 deletions(-)

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index a3e8f0c9b9..1187fd9c25 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -119,6 +119,34 @@ boolean_param("ler", opt_ler);
 #define stack_words_per_line 4
 #define ESP_BEFORE_EXCEPTION(regs) ((unsigned long *)regs->rsp)
 
+static void do_trap(struct cpu_user_regs *regs);
+static void do_reserved_trap(struct cpu_user_regs *regs);
+
+void (* const exception_table[TRAP_nr])(struct cpu_user_regs *regs) = {
+    [TRAP_divide_error]                 = do_trap,
+    [TRAP_debug]                        = do_debug,
+    [TRAP_nmi]                          = (void *)do_nmi,
+    [TRAP_int3]                         = do_int3,
+    [TRAP_overflow]                     = do_trap,
+    [TRAP_bounds]                       = do_trap,
+    [TRAP_invalid_op]                   = do_invalid_op,
+    [TRAP_no_device]                    = do_device_not_available,
+    [TRAP_double_fault]                 = do_reserved_trap,
+    [TRAP_copro_seg]                    = do_reserved_trap,
+    [TRAP_invalid_tss]                  = do_trap,
+    [TRAP_no_segment]                   = do_trap,
+    [TRAP_stack_error]                  = do_trap,
+    [TRAP_gp_fault]                     = do_general_protection,
+    [TRAP_page_fault]                   = do_page_fault,
+    [TRAP_spurious_int]                 = do_reserved_trap,
+    [TRAP_copro_error]                  = do_trap,
+    [TRAP_alignment_check]              = do_trap,
+    [TRAP_machine_check]                = (void *)do_machine_check,
+    [TRAP_simd_error]                   = do_trap,
+    [TRAP_virtualisation ...
+     (ARRAY_SIZE(exception_table) - 1)] = do_reserved_trap,
+};
+
 static void show_code(const struct cpu_user_regs *regs)
 {
     unsigned char insns_before[8] = {}, insns_after[16] = {};
@@ -687,7 +715,7 @@ void fatal_trap(const struct cpu_user_regs *regs, bool show_remote)
           (regs->eflags & X86_EFLAGS_IF) ? "" : ", IN INTERRUPT CONTEXT");
 }
 
-void do_reserved_trap(struct cpu_user_regs *regs)
+static void do_reserved_trap(struct cpu_user_regs *regs)
 {
     unsigned int trapnr = regs->entry_vector;
 
@@ -698,7 +726,7 @@ void do_reserved_trap(struct cpu_user_regs *regs)
     panic("FATAL RESERVED TRAP %#x: %s", trapnr, trapstr(trapnr));
 }
 
-void do_trap(struct cpu_user_regs *regs)
+static void do_trap(struct cpu_user_regs *regs)
 {
     struct vcpu *curr = current;
     unsigned int trapnr = regs->entry_vector;
diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
index 710c0616ba..af703f6c06 100644
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -806,36 +806,6 @@ ENTRY(enable_nmis)
 GLOBAL(trap_nop)
         iretq
 
-
-
-.section .rodata, "a", @progbits
-
-ENTRY(exception_table)
-        .quad do_trap
-        .quad do_debug
-        .quad do_nmi
-        .quad do_int3
-        .quad do_trap
-        .quad do_trap
-        .quad do_invalid_op
-        .quad do_device_not_available
-        .quad do_reserved_trap /* double_fault - has its own entry. */
-        .quad do_reserved_trap /* coproc_seg_overrun - Intel 387 only. */
-        .quad do_trap
-        .quad do_trap
-        .quad do_trap
-        .quad do_general_protection
-        .quad do_page_fault
-        .quad do_reserved_trap /* Default PIC spurious irq - architecturally reserved. */
-        .quad do_trap
-        .quad do_trap
-        .quad do_machine_check
-        .quad do_trap
-        .rept TRAP_nr - ((. - exception_table) / 8)
-        .quad do_reserved_trap /* Architecturally reserved exceptions. */
-        .endr
-        .size exception_table, . - exception_table
-
 /* Table of automatically generated entry points.  One per vector. */
         .section .init.rodata, "a", @progbits
 GLOBAL(autogen_entrypoints)
diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h
index e8c2f02e99..9c70a98aef 100644
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -501,7 +501,6 @@ DECLARE_TRAP_HANDLER(entry_int82);
 
 void trap_nop(void);
 void enable_nmis(void);
-void do_reserved_trap(struct cpu_user_regs *regs);
 
 void sysenter_entry(void);
 void sysenter_eflags_saved(void);
-- 
2.15.1


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

[PATCH v3 5/5] x86: remove usage of .skip with non-absolute expressions

[Roger Pau Monne]

Clang assembler doesn't support using .skip with non-absolute
expressions:

entry.S:109:15: error: expected absolute expression
        .skip .Lcr4_alt_end - .Lcr4_alt, 0x90
              ^

This usage of .skip was to fill code sections with NOPs in order for
them to be patched at run time if required by the alternatives
framework. Instead of using .skip use the appropriate number of NOPs
to match the size of the alternative code.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>
Cc: Ian Jackson <ian.jackson@eu.citrix.com>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Stefano Stabellini <sstabellini@kernel.org>
Cc: Tim Deegan <tim@xen.org>
Cc: Wei Liu <wei.liu2@citrix.com>
---
 xen/Rules.mk                       | 5 -----
 xen/arch/x86/x86_64/compat/entry.S | 9 ++++++++-
 xen/arch/x86/x86_64/entry.S        | 2 +-
 3 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/xen/Rules.mk b/xen/Rules.mk
index 205f0aff30..51bcd5804c 100644
--- a/xen/Rules.mk
+++ b/xen/Rules.mk
@@ -66,11 +66,6 @@ endif
 
 AFLAGS-y                += -D__ASSEMBLY__
 
-# Clang's built-in assembler doesn't understand .skip or .rept assembler
-# directives without an absolute value:
-# https://bugs.llvm.org/show_bug.cgi?id=27369
-AFLAGS-$(clang)         += -no-integrated-as
-
 ALL_OBJS := $(ALL_OBJS-y)
 
 # Get gcc to generate the dependencies for us.
diff --git a/xen/arch/x86/x86_64/compat/entry.S b/xen/arch/x86/x86_64/compat/entry.S
index e668f00c36..d94220b6b6 100644
--- a/xen/arch/x86/x86_64/compat/entry.S
+++ b/xen/arch/x86/x86_64/compat/entry.S
@@ -106,7 +106,14 @@ ENTRY(compat_restore_all_guest)
         mov   $~(X86_EFLAGS_IOPL|X86_EFLAGS_NT|X86_EFLAGS_VM),%r11d
         and   UREGS_eflags(%rsp),%r11d
 .Lcr4_orig:
-        .skip .Lcr4_alt_end - .Lcr4_alt, 0x90
+        ASM_NOP8 /* testb $3,UREGS_cs(%rsp) */
+        ASM_NOP2 /* jpe   .Lcr4_alt_end */
+        ASM_NOP8 /* mov   CPUINFO_cr4...(%rsp), %rax */
+        ASM_NOP6 /* and   $..., %rax */
+        ASM_NOP8 /* mov   %rax, CPUINFO_cr4...(%rsp) */
+        ASM_NOP3 /* mov   %rax, %cr4 */
+        ASM_NOP8 /* mov   %rax, CPUINFO_cr4...(%rsp) */
+        ASM_NOP2 /* jne   1b */
 .Lcr4_orig_end:
         .pushsection .altinstr_replacement, "ax"
 .Lcr4_alt:
diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
index af703f6c06..d9dce0e421 100644
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -529,7 +529,7 @@ handle_exception_saved:
 
 .Lcr4_pv32_orig:
         jmp   .Lcr4_pv32_done
-        .skip (.Lcr4_pv32_alt_end - .Lcr4_pv32_alt) - (. - .Lcr4_pv32_orig), 0xcc
+        ASM_NOP2 /* jmp is 2 bytes, the alternative mov is 4 bytes. */
         .pushsection .altinstr_replacement, "ax"
 .Lcr4_pv32_alt:
         mov   VCPU_domain(%rbx),%rax
-- 
2.15.1


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

Re: [PATCH v3 5/5] x86: remove usage of .skip with non-absolute expressions

[Wei Liu]

On Mon, Jan 29, 2018 at 12:26:43PM +0000, Roger Pau Monne wrote:
> Clang assembler doesn't support using .skip with non-absolute
> expressions:
> 

But so is GNU as. From its manual for .skip:

"This directive emits size bytes, each of value fill. Both size and fill
are absolute expressions."

> entry.S:109:15: error: expected absolute expression
>         .skip .Lcr4_alt_end - .Lcr4_alt, 0x90
>               ^
> 

OOI what makes .Lcr4_alt_end - .Lcr4_alt non-absolute?

Wei.

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

Re: [PATCH v3 5/5] x86: remove usage of .skip with non-absolute expressions

[Roger Pau Monné]

On Mon, Jan 29, 2018 at 12:43:10PM +0000, Wei Liu wrote:
> On Mon, Jan 29, 2018 at 12:26:43PM +0000, Roger Pau Monne wrote:
> > Clang assembler doesn't support using .skip with non-absolute
> > expressions:
> > 
> 
> But so is GNU as. From its manual for .skip:
> 
> "This directive emits size bytes, each of value fill. Both size and fill
> are absolute expressions."

I guess clang and as have different interpretations of what's an
absolute expression. Sadly I don't seem to be able to find neither
definitions.

I've already reported this to upstream clang long time ago, but
it doesn't seem to make any progress:

https://bugs.llvm.org/show_bug.cgi?id=27369

And I don't think I can hack this myself.

> > entry.S:109:15: error: expected absolute expression
> >         .skip .Lcr4_alt_end - .Lcr4_alt, 0x90
> >               ^
> > 
> 
> OOI what makes .Lcr4_alt_end - .Lcr4_alt non-absolute?

Clang doesn't consider symbols as absolute expressions, at least in the
context of .skip.

Thanks, Roger.

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

Re: [PATCH v3 5/5] x86: remove usage of .skip with non-absolute expressions

[Jan Beulich]

>>> On 29.01.18 at 13:43, <wei.liu2@citrix.com> wrote:
> On Mon, Jan 29, 2018 at 12:26:43PM +0000, Roger Pau Monne wrote:
>> Clang assembler doesn't support using .skip with non-absolute
>> expressions:
>> 
> 
> But so is GNU as. From its manual for .skip:
> 
> "This directive emits size bytes, each of value fill. Both size and fill
> are absolute expressions."
> 
>> entry.S:109:15: error: expected absolute expression
>>         .skip .Lcr4_alt_end - .Lcr4_alt, 0x90
>>               ^
>> 
> 
> OOI what makes .Lcr4_alt_end - .Lcr4_alt non-absolute?

I guess they expect to be able to calculate the value right at the
point they evaluate .skip's arguments. Whereas gas either records
a fragment with variable size, or (less likely, as that could end up
wrong) evaluates the expression right away (iirc the .skip sits after
the definition of both symbols, and iirc further gas [at least some
versions] has problems if that wasn't the case).

Jan


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

Re: [PATCH v3 5/5] x86: remove usage of .skip with non-absolute expressions

[Andrew Cooper]

On 29/01/18 13:02, Jan Beulich wrote:
>>>> On 29.01.18 at 13:43, <wei.liu2@citrix.com> wrote:
>> On Mon, Jan 29, 2018 at 12:26:43PM +0000, Roger Pau Monne wrote:
>>> Clang assembler doesn't support using .skip with non-absolute
>>> expressions:
>>>
>> But so is GNU as. From its manual for .skip:
>>
>> "This directive emits size bytes, each of value fill. Both size and fill
>> are absolute expressions."
>>
>>> entry.S:109:15: error: expected absolute expression
>>>         .skip .Lcr4_alt_end - .Lcr4_alt, 0x90
>>>               ^
>>>
>> OOI what makes .Lcr4_alt_end - .Lcr4_alt non-absolute?
> I guess they expect to be able to calculate the value right at the
> point they evaluate .skip's arguments. Whereas gas either records
> a fragment with variable size, or (less likely, as that could end up
> wrong) evaluates the expression right away (iirc the .skip sits after
> the definition of both symbols, and iirc further gas [at least some
> versions] has problems if that wasn't the case).

One thing I've been experimenting with, along with trying to organise
the alternatives into a mergeable section, is to first write the
alternatives into .discard section, which allows calculations like this
to be completed immediately, rather than being deferred.

~Andrew

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

Re: [PATCH v3 5/5] x86: remove usage of .skip with non-absolute expressions

[Jan Beulich]

>>> On 29.01.18 at 14:05, <andrew.cooper3@citrix.com> wrote:
> On 29/01/18 13:02, Jan Beulich wrote:
>>>>> On 29.01.18 at 13:43, <wei.liu2@citrix.com> wrote:
>>> On Mon, Jan 29, 2018 at 12:26:43PM +0000, Roger Pau Monne wrote:
>>>> Clang assembler doesn't support using .skip with non-absolute
>>>> expressions:
>>>>
>>> But so is GNU as. From its manual for .skip:
>>>
>>> "This directive emits size bytes, each of value fill. Both size and fill
>>> are absolute expressions."
>>>
>>>> entry.S:109:15: error: expected absolute expression
>>>>         .skip .Lcr4_alt_end - .Lcr4_alt, 0x90
>>>>               ^
>>>>
>>> OOI what makes .Lcr4_alt_end - .Lcr4_alt non-absolute?
>> I guess they expect to be able to calculate the value right at the
>> point they evaluate .skip's arguments. Whereas gas either records
>> a fragment with variable size, or (less likely, as that could end up
>> wrong) evaluates the expression right away (iirc the .skip sits after
>> the definition of both symbols, and iirc further gas [at least some
>> versions] has problems if that wasn't the case).
> 
> One thing I've been experimenting with, along with trying to organise
> the alternatives into a mergeable section, is to first write the
> alternatives into .discard section, which allows calculations like this
> to be completed immediately, rather than being deferred.

How would such calculations be completed immediately? It doesn't
matter what section you put things in - final addresses can be
known only once all input has been consumed by the assembler.

Jan


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

Re: [PATCH v3 1/5] build: filter out command line assembler arguments

[Ian Jackson]

Roger Pau Monne writes ("[PATCH v3 1/5] build: filter out command line assembler arguments"):
> If the assembler is not used. This happens when using cc -E or cc -S
> for example. GCC will just ignore the -Wa,... when the assembler is
> not called, but clang will complain loudly and fail.
...
>  asm-offsets.s: $(TARGET_SUBARCH)/asm-offsets.c
> -	$(CC) $(filter-out -flto,$(CFLAGS)) -S -o $@ $<
> +	$(CC) $(filter-out -Wa% -flto,$(CFLAGS)) -S -o $@ $<

-Wa% matches -Wall.

You probably meant -Wa,%

Ian.

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

Re: [PATCH v3 2/5] x86/clang: fix build with indirect thunks

[Jan Beulich]

>>> On 29.01.18 at 13:26, <roger.pau@citrix.com> wrote:
> --- a/Config.mk
> +++ b/Config.mk
> @@ -157,17 +157,16 @@ ifndef XEN_HAS_CHECKPOLICY
>  endif
>  
>  # as-insn: Check whether assembler supports an instruction.
> -# Usage: cflags-y += $(call as-insn "insn",option-yes,option-no)
> +# Usage: cflags-y += $(call as-insn cc,"insn",option-yes,option-no,FLAGS)
>  as-insn = $(if $(shell echo 'void _(void) { asm volatile ( $(2) ); }' \
> -                       | $(1) $(filter-out -M% %.d -include %/include/xen/config.h,$(AFLAGS)) \
> +                       | $(1) $(filter-out -M% %.d -include %/include/xen/config.h,$(5)) \

Despite this making for a larger diff, I don't think it makes sense to
add this new parameter as the last one. Natural would imo be (using
the provided example)

$(call as-insn cc,FLAGS,"insn",option-yes,option-no)

unless perhaps the flags can't be passed together with the build
tool name in the first place.

>                                -c -x c -o /dev/null - 2>&1),$(4),$(3))
> -

Please don't remove this blank line.

> --- a/xen/include/asm-x86/asm_defns.h
> +++ b/xen/include/asm-x86/asm_defns.h
> @@ -16,9 +16,31 @@
>  #ifdef __ASSEMBLY__
>  # include <asm/indirect_thunk_asm.h>
>  #else
> -asm ( "\t.equ CONFIG_INDIRECT_THUNK, "
> -      __stringify(IS_ENABLED(CONFIG_INDIRECT_THUNK)) );
> -asm ( "\t.include \"asm/indirect_thunk_asm.h\"" );
> +/*
> + * NB: for clang's integrated assembler the macro must be included in every
> + * asm() instance where it's used. For gcc/gas it only needs to be included
> + * once per file. Note that even guarding the indirect_thunk_asm.h with
> + * something like:
> + *
> + * .ifndef _X86_INDIRECT_THUNK_ASM_H_
> + * .equ _X86_INDIRECT_THUNK_ASM_H_, 1
> + * ...
> + * .endif
> + *
> + * Doesn't work with clang because it will keep the .equ defined between asm()

doesn't

> + * instances, but will forget the .macro. This has been reported upstream:
> + * https://bugs.llvm.org/show_bug.cgi?id=36110 
> + */

Ugly. One more remark concerning the potential (longer term)
negative aspects of the chose approach: Producing equates
and defining macros in the middle of other code is probably
fine, but what if other things get added to the header? Not
everything commonly done in a header (even if that's an
assembly only one) is necessarily benign to the surrounding
code.

IOW - I'm not convinced it is desirable to be able to use the
integrated assembler, if there are such significant quirks to
work around.

Jan


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

Re: [PATCH v3 3/5] x86: fix indirect thunk usage of CONFIG_INDIRECT_THUNK

[Jan Beulich]

>>> On 29.01.18 at 13:26, <roger.pau@citrix.com> wrote:
> --- a/xen/include/asm-x86/asm_defns.h
> +++ b/xen/include/asm-x86/asm_defns.h
> @@ -14,6 +14,9 @@
>  #include <asm/alternative.h>
>  
>  #ifdef __ASSEMBLY__
> +# ifndef CONFIG_INDIRECT_THUNK
> +#  define CONFIG_INDIRECT_THUNK 0
> +# endif

Why not an assembler equate? It is common to use "#ifdef CONFIG_..."
if wanting to test for an enabled config option, and such a check
will go wrong with a C level define.

Jan


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

Re: [PATCH v3 4/5] x86: move declaration of the exception_table to C

[Jan Beulich]

>>> On 29.01.18 at 13:26, <roger.pau@citrix.com> wrote:
> This makes the code cleaner because there's no need to declare the
> exception_table in assembly, and also fixes the following error when
> using clang's integrated assembler:
> 
> entry.S:834:15: error: unexpected token in '.rept' directive
>         .rept 32 - ((. - exception_table) / 8)
>               ^
> entry.S:836:14: error: unmatched '.endr' directive
>         .endr
>              ^
> 
> This should be a non-functional change.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>


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

Re: [PATCH v3 5/5] x86: remove usage of .skip with non-absolute expressions

[Jan Beulich]

>>> On 29.01.18 at 13:26, <roger.pau@citrix.com> wrote:
> Clang assembler doesn't support using .skip with non-absolute
> expressions:
> 
> entry.S:109:15: error: expected absolute expression
>         .skip .Lcr4_alt_end - .Lcr4_alt, 0x90
>               ^
> 
> This usage of .skip was to fill code sections with NOPs in order for
> them to be patched at run time if required by the alternatives
> framework. Instead of using .skip use the appropriate number of NOPs
> to match the size of the alternative code.

NAK - I've voiced a number of times my opposition to Andrew adding
the various ASM_NOP<N> in his Spectre v2 series, and I'm planning
to eliminate them in due course (by using - you guess it - .skip). It
is pretty much unacceptable to me to have to encode the size of
certain instructions in a second place, risking them to go out of sync
with what they shadow.

If ugliness like this is needed to support clang's integrated assembler,
then I see no way other than avoiding its use.

Jan


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

Re: [PATCH v3 3/5] x86: fix indirect thunk usage of CONFIG_INDIRECT_THUNK

[Roger Pau Monné]

On Mon, Jan 29, 2018 at 09:45:35AM -0700, Jan Beulich wrote:
> >>> On 29.01.18 at 13:26, <roger.pau@citrix.com> wrote:
> > --- a/xen/include/asm-x86/asm_defns.h
> > +++ b/xen/include/asm-x86/asm_defns.h
> > @@ -14,6 +14,9 @@
> >  #include <asm/alternative.h>
> >  
> >  #ifdef __ASSEMBLY__
> > +# ifndef CONFIG_INDIRECT_THUNK
> > +#  define CONFIG_INDIRECT_THUNK 0
> > +# endif
> 
> Why not an assembler equate? It is common to use "#ifdef CONFIG_..."
> if wanting to test for an enabled config option, and such a check
> will go wrong with a C level define.

I haven't used an equate because that doesn't work well with clang,
because clang keeps equates between asm()-s, but not .macro-s.

Anyway, since there seems to be comments in other patches that will
prevent from switching to clang's integrated assembler I guess it's
not worth arguing over this.

Thanks, Roger.

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

Re: [PATCH v3 5/5] x86: remove usage of .skip with non-absolute expressions

[Andrew Cooper]

On 29/01/18 16:50, Jan Beulich wrote:
>>>> On 29.01.18 at 13:26, <roger.pau@citrix.com> wrote:
>> Clang assembler doesn't support using .skip with non-absolute
>> expressions:
>>
>> entry.S:109:15: error: expected absolute expression
>>         .skip .Lcr4_alt_end - .Lcr4_alt, 0x90
>>               ^
>>
>> This usage of .skip was to fill code sections with NOPs in order for
>> them to be patched at run time if required by the alternatives
>> framework. Instead of using .skip use the appropriate number of NOPs
>> to match the size of the alternative code.
> NAK - I've voiced a number of times my opposition to Andrew adding
> the various ASM_NOP<N> in his Spectre v2 series, and I'm planning
> to eliminate them in due course (by using - you guess it - .skip). It
> is pretty much unacceptable to me to have to encode the size of
> certain instructions in a second place, risking them to go out of sync
> with what they shadow.

And FTR, the Spectre v2 series explicit sizes were only because I tried
fixing Xen's ability to do sensible things with nops in alternative, but
got far too bogged down and decided that the Spectre work was more
important.

I will be dusting the series off in due course and removing the explicit
sizes.

~Andrew

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

Re: [PATCH v3 5/5] x86: remove usage of .skip with non-absolute expressions

[Roger Pau Monné]

On Mon, Jan 29, 2018 at 09:50:56AM -0700, Jan Beulich wrote:
> >>> On 29.01.18 at 13:26, <roger.pau@citrix.com> wrote:
> > Clang assembler doesn't support using .skip with non-absolute
> > expressions:
> > 
> > entry.S:109:15: error: expected absolute expression
> >         .skip .Lcr4_alt_end - .Lcr4_alt, 0x90
> >               ^
> > 
> > This usage of .skip was to fill code sections with NOPs in order for
> > them to be patched at run time if required by the alternatives
> > framework. Instead of using .skip use the appropriate number of NOPs
> > to match the size of the alternative code.
> 
> NAK - I've voiced a number of times my opposition to Andrew adding
> the various ASM_NOP<N> in his Spectre v2 series, and I'm planning
> to eliminate them in due course (by using - you guess it - .skip). It
> is pretty much unacceptable to me to have to encode the size of
> certain instructions in a second place, risking them to go out of sync
> with what they shadow.
> 
> If ugliness like this is needed to support clang's integrated assembler,
> then I see no way other than avoiding its use.

Right, I agree this is ugly and prone to error. This seems to be fixed
in clang thunk, so I've asked for a backport of the fix to 6.0.

Roger.

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