[PATCH 0/6] Remove unqalified ud2 instructions

[PATCH 0/6] Remove unqalified ud2 instructions

[Andrew Cooper]

Having ud2 instructions without associated bugframe entries creates
misleading information when investigating a crash.

This series adds bugframe entries to all ud2 instructions I could
locate, which leads to more useful error messages in failure cases.

Patch 1 is unrelated to the series (and a backport candidate).  However,
patch 2 depends (texturally, rather than functionally) on it.

Andrew Cooper (6):
  x86/vmx: Improvements to vmentry failure handling
  xen/x86: Replace unqualified ud2 instructions with BUG frames
  x86/bug: Break out the internals of BUG_FRAME()
  x86/vmx: Replace unqualified ud2 instructions with BUG frames
  xen/x86: Avoid using local labels for UNLIKELY() regions
  EXAMPLE TEST CODE

 xen/arch/x86/boot/x86_64.S        |  2 +-
 xen/arch/x86/hvm/vmx/entry.S      | 11 +++---
 xen/arch/x86/hvm/vmx/vmcs.c       | 15 +++-----
 xen/arch/x86/traps.c              | 17 +++++++++
 xen/arch/x86/x86_64/entry.S       |  4 +--
 xen/include/asm-x86/asm_defns.h   | 18 +++++-----
 xen/include/asm-x86/bug.h         | 37 ++++++++++++--------
 xen/include/asm-x86/hvm/vmx/vmx.h | 72 ++++++++++++++++++++++-----------------
 8 files changed, 102 insertions(+), 74 deletions(-)

-- 
2.1.4

[PATCH 1/6] x86/vmx: Improvements to vmentry failure handling

[Andrew Cooper]

Combine the almost identical vm_launch_fail() and vm_resume_fail() into a
single vmx_vmentry_failure().

Re-save all GPRs so that domain_crash() prints the real register values,
rather than the stack frame of the vmx_vmentry_failure() call.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Kevin Tian <kevin.tian@intel.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Jun Nakajima <jun.nakajima@intel.com>

v2:
 * Swap printk for gprintk
---
 xen/arch/x86/hvm/vmx/entry.S |  9 +++++----
 xen/arch/x86/hvm/vmx/vmcs.c  | 15 ++++-----------
 2 files changed, 9 insertions(+), 15 deletions(-)

diff --git a/xen/arch/x86/hvm/vmx/entry.S b/xen/arch/x86/hvm/vmx/entry.S
index 2a4ed57..a5438a4 100644
--- a/xen/arch/x86/hvm/vmx/entry.S
+++ b/xen/arch/x86/hvm/vmx/entry.S
@@ -101,14 +101,15 @@ UNLIKELY_END(realmode)
 
 /*.Lvmx_resume:*/
         VMRESUME
-        sti
-        call vm_resume_fail
-        ud2
+        jmp  .Lvmx_vmentry_fail
 
 .Lvmx_launch:
         VMLAUNCH
+
+.Lvmx_vmentry_fail:
         sti
-        call vm_launch_fail
+        SAVE_ALL
+        call vmx_vmentry_failure
         ud2
 
 ENTRY(vmx_asm_do_vmentry)
diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 4ea1ad1..53207e6 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -1612,21 +1612,14 @@ void vmx_destroy_vmcs(struct vcpu *v)
     free_xenheap_page(v->arch.hvm_vmx.msr_bitmap);
 }
 
-void vm_launch_fail(void)
-{
-    unsigned long error;
-
-    __vmread(VM_INSTRUCTION_ERROR, &error);
-    printk("<vm_launch_fail> error code %lx\n", error);
-    domain_crash_synchronous();
-}
-
-void vm_resume_fail(void)
+void vmx_vmentry_failure(void)
 {
+    struct vcpu *curr = current;
     unsigned long error;
 
     __vmread(VM_INSTRUCTION_ERROR, &error);
-    printk("<vm_resume_fail> error code %lx\n", error);
+    gprintk(XENLOG_ERR, "VM%s error: %#lx\n",
+            curr->arch.hvm_vmx.launched ? "RESUME" : "LAUNCH", error);
     domain_crash_synchronous();
 }
 
-- 
2.1.4

[PATCH v2 2/6] xen/x86: Replace unqualified ud2 instructions with BUG frames

[Andrew Cooper]

No functional change, other than the failure cases, which now produce a
far more clear error message.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
---
 xen/arch/x86/boot/x86_64.S      | 2 +-
 xen/arch/x86/hvm/vmx/entry.S    | 2 +-
 xen/arch/x86/x86_64/entry.S     | 4 ++--
 xen/include/asm-x86/asm_defns.h | 2 +-
 4 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/boot/x86_64.S b/xen/arch/x86/boot/x86_64.S
index c8bf9d0..94cf089 100644
--- a/xen/arch/x86/boot/x86_64.S
+++ b/xen/arch/x86/boot/x86_64.S
@@ -34,7 +34,7 @@ ENTRY(__high_start)
         /* Pass off the Multiboot info structure to C land. */
         mov     multiboot_ptr(%rip),%edi
         call    __start_xen
-        ud2     /* Force a panic (invalid opcode). */
+        BUG     /* __start_xen() shouldn't return. */
 
 /*** DESCRIPTOR TABLES ***/
 
diff --git a/xen/arch/x86/hvm/vmx/entry.S b/xen/arch/x86/hvm/vmx/entry.S
index a5438a4..5a1757e 100644
--- a/xen/arch/x86/hvm/vmx/entry.S
+++ b/xen/arch/x86/hvm/vmx/entry.S
@@ -110,7 +110,7 @@ UNLIKELY_END(realmode)
         sti
         SAVE_ALL
         call vmx_vmentry_failure
-        ud2
+        BUG  /* vmx_vmentry_failure() shouldn't return. */
 
 ENTRY(vmx_asm_do_vmentry)
         GET_CURRENT(%rbx)
diff --git a/xen/arch/x86/x86_64/entry.S b/xen/arch/x86/x86_64/entry.S
index 28c3214..d4dd8e8 100644
--- a/xen/arch/x86/x86_64/entry.S
+++ b/xen/arch/x86/x86_64/entry.S
@@ -547,7 +547,7 @@ exception_with_ints_disabled:
 FATAL_exception_with_ints_disabled:
         movq  %rsp,%rdi
         call  fatal_trap
-        ud2
+        BUG   /* fatal_trap() shouldn't return. */
 
 ENTRY(divide_error)
         pushq $0
@@ -620,7 +620,7 @@ ENTRY(double_fault)
         SAVE_ALL STAC
         movq  %rsp,%rdi
         call  do_double_fault
-        ud2
+        BUG   /* do_double_fault() shouldn't return. */
 
         .pushsection .init.text, "ax", @progbits
 ENTRY(early_page_fault)
diff --git a/xen/include/asm-x86/asm_defns.h b/xen/include/asm-x86/asm_defns.h
index d750f03..95ea21d 100644
--- a/xen/include/asm-x86/asm_defns.h
+++ b/xen/include/asm-x86/asm_defns.h
@@ -324,7 +324,7 @@ static always_inline void stac(void)
         jne   789f
         cmpq  UREGS_r12(%rsp),%r12
         je    987f
-789:    ud2
+789:    BUG   /* Corruption of partial register state. */
         .subsection 0
 #endif
 .endif
-- 
2.1.4

[PATCH 3/6] x86/bug: Break out the internals of BUG_FRAME()

[Andrew Cooper]

To allow bug frames can be created inside existing asm() statements.  In
order to do so, the current bugframe positional parameters are altered
to be named parameters, to avoid interactions with the parameters of the
existing asm() statement.

No functional change.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/include/asm-x86/bug.h | 37 ++++++++++++++++++++++---------------
 1 file changed, 22 insertions(+), 15 deletions(-)

diff --git a/xen/include/asm-x86/bug.h b/xen/include/asm-x86/bug.h
index cec6bce..e868e85 100644
--- a/xen/include/asm-x86/bug.h
+++ b/xen/include/asm-x86/bug.h
@@ -29,23 +29,30 @@ struct bug_frame {
                       ((1 << BUG_LINE_LO_WIDTH) - 1)))
 #define bug_msg(b) ((const char *)(b) + (b)->msg_disp[1])
 
+#define _ASM_BUGFRAME_TEXT(second_frame)                                     \
+    ".Lbug%=: ud2\n"                                                         \
+    ".pushsection .bug_frames.%c[bf_type], \"a\", @progbits\n"               \
+    ".p2align 2\n"                                                           \
+    ".Lfrm%=:\n"                                                             \
+    ".long (.Lbug%= - .Lfrm%=) + %c[bf_line_hi]\n"                           \
+    ".long (%c[bf_ptr] - .Lfrm%=) + %c[bf_line_lo]\n"                        \
+    ".if " #second_frame "\n"                                                \
+    ".long 0, %c[bf_msg] - .Lfrm%=\n"                                        \
+    ".endif\n"                                                               \
+    ".popsection\n"                                                          \
+
+#define _ASM_BUGFRAME_INFO(type, line, ptr, msg)                             \
+    [bf_type]    "i" (type),                                                 \
+    [bf_ptr]     "i" (ptr),                                                  \
+    [bf_msg]     "i" (msg),                                                  \
+    [bf_line_lo] "i" ((line & ((1 << BUG_LINE_LO_WIDTH) - 1))                \
+                      << BUG_DISP_WIDTH),                                    \
+    [bf_line_hi] "i" (((line) >> BUG_LINE_LO_WIDTH) << BUG_DISP_WIDTH)
+
 #define BUG_FRAME(type, line, ptr, second_frame, msg) do {                   \
     BUILD_BUG_ON((line) >> (BUG_LINE_LO_WIDTH + BUG_LINE_HI_WIDTH));         \
-    asm volatile ( ".Lbug%=: ud2\n"                                          \
-                   ".pushsection .bug_frames.%c0, \"a\", @progbits\n"        \
-                   ".p2align 2\n"                                            \
-                   ".Lfrm%=:\n"                                              \
-                   ".long (.Lbug%= - .Lfrm%=) + %c4\n"                       \
-                   ".long (%c1 - .Lfrm%=) + %c3\n"                           \
-                   ".if " #second_frame "\n"                                 \
-                   ".long 0, %c2 - .Lfrm%=\n"                                \
-                   ".endif\n"                                                \
-                   ".popsection"                                             \
-                   :                                                         \
-                   : "i" (type), "i" (ptr), "i" (msg),                       \
-                     "i" ((line & ((1 << BUG_LINE_LO_WIDTH) - 1))            \
-                          << BUG_DISP_WIDTH),                                \
-                     "i" (((line) >> BUG_LINE_LO_WIDTH) << BUG_DISP_WIDTH)); \
+    asm volatile ( _ASM_BUGFRAME_TEXT(second_frame)                          \
+                   :: _ASM_BUGFRAME_INFO(type, line, ptr, msg) );            \
 } while (0)
 
 
-- 
2.1.4

[PATCH 4/6] x86/vmx: Replace unqualified ud2 instructions with BUG frames

[Andrew Cooper]

Using new _ASM_BUGFRAME* internals.

A side effect of complicating the ASM statements is that GCC now chooses to
out-of-line the stub functions, resulting in identical copies being present in
all translation units.  As with the stac()/clac() stubs, force them always
inline.

No functional change, other than the failure cases, which now produce a
far more clear error message.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.com>
---
 xen/include/asm-x86/hvm/vmx/vmx.h | 70 ++++++++++++++++++++++-----------------
 1 file changed, 39 insertions(+), 31 deletions(-)

diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h b/xen/include/asm-x86/hvm/vmx/vmx.h
index e750a76..f16a306 100644
--- a/xen/include/asm-x86/hvm/vmx/vmx.h
+++ b/xen/include/asm-x86/hvm/vmx/vmx.h
@@ -281,7 +281,7 @@ extern uint8_t posted_intr_vector;
 #define INVVPID_ALL_CONTEXT                     2
 #define INVVPID_SINGLE_CONTEXT_RETAINING_GLOBAL 3
 
-static inline void __vmptrld(u64 addr)
+static always_inline void __vmptrld(u64 addr)
 {
     asm volatile (
 #ifdef HAVE_GAS_VMX
@@ -289,20 +289,21 @@ static inline void __vmptrld(u64 addr)
 #else
                    VMPTRLD_OPCODE MODRM_EAX_06
 #endif
-                   /* CF==1 or ZF==1 --> crash (ud2) */
+                   /* CF==1 or ZF==1 --> BUG() */
                    UNLIKELY_START(be, vmptrld)
-                   "\tud2\n"
+                   _ASM_BUGFRAME_TEXT(0)
                    UNLIKELY_END_SECTION
                    :
 #ifdef HAVE_GAS_VMX
-                   : "m" (addr)
+                   : "m" (addr),
 #else
-                   : "a" (&addr)
+                   : "a" (&addr),
 #endif
+                     _ASM_BUGFRAME_INFO(BUGFRAME_bug, __LINE__, __FILE__, 0)
                    : "memory");
 }
 
-static inline void __vmpclear(u64 addr)
+static always_inline void __vmpclear(u64 addr)
 {
     asm volatile (
 #ifdef HAVE_GAS_VMX
@@ -310,20 +311,21 @@ static inline void __vmpclear(u64 addr)
 #else
                    VMCLEAR_OPCODE MODRM_EAX_06
 #endif
-                   /* CF==1 or ZF==1 --> crash (ud2) */
+                   /* CF==1 or ZF==1 --> BUG() */
                    UNLIKELY_START(be, vmclear)
-                   "\tud2\n"
+                   _ASM_BUGFRAME_TEXT(0)
                    UNLIKELY_END_SECTION
                    :
 #ifdef HAVE_GAS_VMX
-                   : "m" (addr)
+                   : "m" (addr),
 #else
-                   : "a" (&addr)
+                   : "a" (&addr),
 #endif
+                     _ASM_BUGFRAME_INFO(BUGFRAME_bug, __LINE__, __FILE__, 0)
                    : "memory");
 }
 
-static inline void __vmread(unsigned long field, unsigned long *value)
+static always_inline void __vmread(unsigned long field, unsigned long *value)
 {
     asm volatile (
 #ifdef HAVE_GAS_VMX
@@ -331,20 +333,22 @@ static inline void __vmread(unsigned long field, unsigned long *value)
 #else
                    VMREAD_OPCODE MODRM_EAX_ECX
 #endif
-                   /* CF==1 or ZF==1 --> crash (ud2) */
+                   /* CF==1 or ZF==1 --> BUG() */
                    UNLIKELY_START(be, vmread)
-                   "\tud2\n"
+                   _ASM_BUGFRAME_TEXT(0)
                    UNLIKELY_END_SECTION
 #ifdef HAVE_GAS_VMX
                    : "=rm" (*value)
-                   : "r" (field));
+                   : "r" (field),
 #else
                    : "=c" (*value)
-                   : "a" (field));
+                   : "a" (field),
 #endif
+                     _ASM_BUGFRAME_INFO(BUGFRAME_bug, __LINE__, __FILE__, 0)
+        );
 }
 
-static inline void __vmwrite(unsigned long field, unsigned long value)
+static always_inline void __vmwrite(unsigned long field, unsigned long value)
 {
     asm volatile (
 #ifdef HAVE_GAS_VMX
@@ -352,16 +356,18 @@ static inline void __vmwrite(unsigned long field, unsigned long value)
 #else
                    VMWRITE_OPCODE MODRM_EAX_ECX
 #endif
-                   /* CF==1 or ZF==1 --> crash (ud2) */
+                   /* CF==1 or ZF==1 --> BUG() */
                    UNLIKELY_START(be, vmwrite)
-                   "\tud2\n"
+                   _ASM_BUGFRAME_TEXT(0)
                    UNLIKELY_END_SECTION
-                   : 
+                   :
 #ifdef HAVE_GAS_VMX
-                   : "r" (field) , "rm" (value));
+                   : "r" (field) , "rm" (value),
 #else
-                   : "a" (field) , "c" (value));
+                   : "a" (field) , "c" (value),
 #endif
+                     _ASM_BUGFRAME_INFO(BUGFRAME_bug, __LINE__, __FILE__, 0)
+        );
 }
 
 static inline bool_t __vmread_safe(unsigned long field, unsigned long *value)
@@ -387,7 +393,7 @@ static inline bool_t __vmread_safe(unsigned long field, unsigned long *value)
     return okay;
 }
 
-static inline void __invept(unsigned long type, u64 eptp, u64 gpa)
+static always_inline void __invept(unsigned long type, u64 eptp, u64 gpa)
 {
     struct {
         u64 eptp, gpa;
@@ -407,20 +413,21 @@ static inline void __invept(unsigned long type, u64 eptp, u64 gpa)
 #else
                    INVEPT_OPCODE MODRM_EAX_08
 #endif
-                   /* CF==1 or ZF==1 --> crash (ud2) */
+                   /* CF==1 or ZF==1 --> BUG() */
                    UNLIKELY_START(be, invept)
-                   "\tud2\n"
+                   _ASM_BUGFRAME_TEXT(0)
                    UNLIKELY_END_SECTION
                    :
 #ifdef HAVE_GAS_EPT
-                   : "m" (operand), "r" (type)
+                   : "m" (operand), "r" (type),
 #else
-                   : "a" (&operand), "c" (type)
+                   : "a" (&operand), "c" (type),
 #endif
+                     _ASM_BUGFRAME_INFO(BUGFRAME_bug, __LINE__, __FILE__, 0)
                    : "memory" );
 }
 
-static inline void __invvpid(unsigned long type, u16 vpid, u64 gva)
+static always_inline void __invvpid(unsigned long type, u16 vpid, u64 gva)
 {
     struct __packed {
         u64 vpid:16;
@@ -435,18 +442,19 @@ static inline void __invvpid(unsigned long type, u16 vpid, u64 gva)
 #else
                    INVVPID_OPCODE MODRM_EAX_08
 #endif
-                   /* CF==1 or ZF==1 --> crash (ud2) */
+                   /* CF==1 or ZF==1 --> BUG() */
                    UNLIKELY_START(be, invvpid)
-                   "\tud2\n"
+                   _ASM_BUGFRAME_TEXT(0)
                    UNLIKELY_END_SECTION "\n"
                    "2:"
                    _ASM_EXTABLE(1b, 2b)
                    :
 #ifdef HAVE_GAS_EPT
-                   : "m" (operand), "r" (type)
+                   : "m" (operand), "r" (type),
 #else
-                   : "a" (&operand), "c" (type)
+                   : "a" (&operand), "c" (type),
 #endif
+                     _ASM_BUGFRAME_INFO(BUGFRAME_bug, __LINE__, __FILE__, 0)
                    : "memory" );
 }
 
-- 
2.1.4

[PATCH 5/6] xen/x86: Avoid using local labels for UNLIKELY() regions

[Andrew Cooper]

Using local labels causes the stack trace to use the last non-local
label emitted by the compiler in the translation unit, which is almost
always unrelated.

e.g. A (contrived debug) example switches from:

  (XEN) ----[ Xen-4.7-unstable  x86_64  debug=y  Not tainted ]----
  (XEN) CPU:    0
  (XEN) RIP:    e008:[<ffff82d0801961e2>] asm_domain_crash_synchronous+0x44/0x4c
  ...
  (XEN) Xen call trace:
  (XEN)    [<ffff82d0801961e2>] asm_domain_crash_synchronous+0x44/0x4c
  (XEN)    [<ffff82d080114bdf>] handle_keypress+0xa4/0xd9

to:

  (XEN) ----[ Xen-4.7-unstable  x86_64  debug=y  Not tainted ]----
  (XEN) CPU:    0
  (XEN) RIP:    e008:[<ffff82d0801961e2>] unlikely.vmptrld.921+0/0x8
  ...
  (XEN) Xen call trace:
  (XEN)    [<ffff82d0801961e2>] unlikely.vmptrld.921+0/0x8
  (XEN)    [<ffff82d080114bdf>] handle_keypress+0xa4/0xd9

which is far more relevant when identifying %eip.

Additionally, correct the inclusion of the tag parameter in the C UNLIKELY
blocks, to use the passed tag, rather than a literal ".tag".

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>

It is not obvious from the documentation whether an expansion of %= is unique
to the current translation unit, or entire linked binary; I suspect the
former.  There are no duplicate symbols introduced by this change in my
current compile, but I realise that this is hardly a guarantee.
---
 xen/include/asm-x86/asm_defns.h | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/xen/include/asm-x86/asm_defns.h b/xen/include/asm-x86/asm_defns.h
index 95ea21d..1028375 100644
--- a/xen/include/asm-x86/asm_defns.h
+++ b/xen/include/asm-x86/asm_defns.h
@@ -95,9 +95,9 @@ void ret_from_intr(void);
 
 #define UNLIKELY_START(cond, tag) \
         .Ldispatch.tag:           \
-        j##cond .Lunlikely.tag;   \
+        j##cond unlikely.tag;     \
         .subsection 1;            \
-        .Lunlikely.tag:
+        unlikely.tag:
 
 #define UNLIKELY_DISPATCH_LABEL(tag) \
         .Ldispatch.tag
@@ -153,15 +153,15 @@ void ret_from_intr(void);
 
 #endif
 
-#define UNLIKELY_START(cond, tag)          \
-        "j" #cond " .Lunlikely%=.tag;\n\t" \
-        UNLIKELY_START_SECTION "\n"        \
-        ".Lunlikely%=.tag:"
+#define UNLIKELY_START(cond, tag)              \
+        "j" #cond " unlikely." #tag ".%=;\n\t" \
+        UNLIKELY_START_SECTION "\n"            \
+        "unlikely." #tag ".%=:"
 
 #define UNLIKELY_END(tag)                  \
-        "jmp .Llikely%=.tag;\n\t"          \
+        "jmp .Llikely." #tag ".%=;\n\t"    \
         UNLIKELY_END_SECTION "\n"          \
-        ".Llikely%=.tag:"
+        ".Llikely." #tag ".%=:"
 
 #endif
 
-- 
2.1.4

[PATCH 6/6] EXAMPLE TEST CODE

[Andrew Cooper]

---
 xen/arch/x86/traps.c              | 17 +++++++++++++++++
 xen/include/asm-x86/hvm/vmx/vmx.h |  4 +++-
 2 files changed, 20 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index b32f696..bc34d56 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -4045,6 +4045,23 @@ void asm_domain_crash_synchronous(unsigned long addr)
     __domain_crash_synchronous();
 }
 
+#include <xen/keyhandler.h>
+#include <asm/hvm/vmx/vmx.h>
+
+static void do_extreme_debug(unsigned char key, struct cpu_user_regs *regs)
+{
+    printk("'%c' pressed -> Extreme debugging in progress...\n", key);
+
+    __vmptrld(0x2);
+}
+
+static int __init extreme_debug_keyhandler_init(void)
+{
+    register_irq_keyhandler('1', &do_extreme_debug, "Extreme debugging", 0);
+    return 0;
+}
+__initcall(extreme_debug_keyhandler_init);
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/include/asm-x86/hvm/vmx/vmx.h b/xen/include/asm-x86/hvm/vmx/vmx.h
index f16a306..42f904e 100644
--- a/xen/include/asm-x86/hvm/vmx/vmx.h
+++ b/xen/include/asm-x86/hvm/vmx/vmx.h
@@ -292,14 +292,16 @@ static always_inline void __vmptrld(u64 addr)
                    /* CF==1 or ZF==1 --> BUG() */
                    UNLIKELY_START(be, vmptrld)
                    _ASM_BUGFRAME_TEXT(0)
+                   "jmp 1f;"
                    UNLIKELY_END_SECTION
+                   "\n1:"
                    :
 #ifdef HAVE_GAS_VMX
                    : "m" (addr),
 #else
                    : "a" (&addr),
 #endif
-                     _ASM_BUGFRAME_INFO(BUGFRAME_bug, __LINE__, __FILE__, 0)
+                     _ASM_BUGFRAME_INFO(BUGFRAME_warn, __LINE__, __FILE__, 0)
                    : "memory");
 }
 
-- 
2.1.4

Re: [PATCH v2 2/6] xen/x86: Replace unqualified ud2 instructions with BUG frames

[Tian, Kevin]

> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Saturday, October 31, 2015 3:59 AM
> 
> No functional change, other than the failure cases, which now produce a
> far more clear error message.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Kevin Tian <kevin.tian@intel.com>

Re: [PATCH 4/6] x86/vmx: Replace unqualified ud2 instructions with BUG frames

[Tian, Kevin]

> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: Saturday, October 31, 2015 3:59 AM
> 
> Using new _ASM_BUGFRAME* internals.
> 
> A side effect of complicating the ASM statements is that GCC now chooses to
> out-of-line the stub functions, resulting in identical copies being present in
> all translation units.  As with the stac()/clac() stubs, force them always
> inline.
> 
> No functional change, other than the failure cases, which now produce a
> far more clear error message.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Acked-by: Kevin Tian <kevin.tian@intel.com>

Re: [PATCH 5/6] xen/x86: Avoid using local labels for UNLIKELY() regions

[Jan Beulich]

>>> On 30.10.15 at 20:59, <andrew.cooper3@citrix.com> wrote:
> Using local labels causes the stack trace to use the last non-local
> label emitted by the compiler in the translation unit, which is almost
> always unrelated.
> 
> e.g. A (contrived debug) example switches from:
> 
>   (XEN) ----[ Xen-4.7-unstable  x86_64  debug=y  Not tainted ]----
>   (XEN) CPU:    0
>   (XEN) RIP:    e008:[<ffff82d0801961e2>] 
> asm_domain_crash_synchronous+0x44/0x4c
>   ...
>   (XEN) Xen call trace:
>   (XEN)    [<ffff82d0801961e2>] asm_domain_crash_synchronous+0x44/0x4c
>   (XEN)    [<ffff82d080114bdf>] handle_keypress+0xa4/0xd9
> 
> to:
> 
>   (XEN) ----[ Xen-4.7-unstable  x86_64  debug=y  Not tainted ]----
>   (XEN) CPU:    0
>   (XEN) RIP:    e008:[<ffff82d0801961e2>] unlikely.vmptrld.921+0/0x8
>   ...
>   (XEN) Xen call trace:
>   (XEN)    [<ffff82d0801961e2>] unlikely.vmptrld.921+0/0x8
>   (XEN)    [<ffff82d080114bdf>] handle_keypress+0xa4/0xd9
> 
> which is far more relevant when identifying %eip.

I disagree; I specifically used local labels here to avoid cluttering the
symbol table. If anything I'd agree to adding a label to the start of
subsection 1 (or .fixup), but it's not clear how emitting such labels
could be avoided when that section is empty (such labels would clash
with whatever label is first in the next compilation unit). Maybe we
could discard them in the symbols processing tool if their address
matches another symbol.

> Additionally, correct the inclusion of the tag parameter in the C UNLIKELY
> blocks, to use the passed tag, rather than a literal ".tag".

This part I agree with of course, and would therefore like to as for this
to be split off.

Jan

Re: [PATCH 5/6] xen/x86: Avoid using local labels for UNLIKELY() regions

[Andrew Cooper]

On 04/11/15 14:45, Jan Beulich wrote:
>>>> On 30.10.15 at 20:59, <andrew.cooper3@citrix.com> wrote:
>> Using local labels causes the stack trace to use the last non-local
>> label emitted by the compiler in the translation unit, which is almost
>> always unrelated.
>>
>> e.g. A (contrived debug) example switches from:
>>
>>   (XEN) ----[ Xen-4.7-unstable  x86_64  debug=y  Not tainted ]----
>>   (XEN) CPU:    0
>>   (XEN) RIP:    e008:[<ffff82d0801961e2>] 
>> asm_domain_crash_synchronous+0x44/0x4c
>>   ...
>>   (XEN) Xen call trace:
>>   (XEN)    [<ffff82d0801961e2>] asm_domain_crash_synchronous+0x44/0x4c
>>   (XEN)    [<ffff82d080114bdf>] handle_keypress+0xa4/0xd9
>>
>> to:
>>
>>   (XEN) ----[ Xen-4.7-unstable  x86_64  debug=y  Not tainted ]----
>>   (XEN) CPU:    0
>>   (XEN) RIP:    e008:[<ffff82d0801961e2>] unlikely.vmptrld.921+0/0x8
>>   ...
>>   (XEN) Xen call trace:
>>   (XEN)    [<ffff82d0801961e2>] unlikely.vmptrld.921+0/0x8
>>   (XEN)    [<ffff82d080114bdf>] handle_keypress+0xa4/0xd9
>>
>> which is far more relevant when identifying %eip.
> I disagree; I specifically used local labels here to avoid cluttering the
> symbol table.

Needless cluttering is indeed bad.  However, the effect here is a
misleading stack trace.

As the entire point of the symbol table is to generate usable
information in situations like this, I don't see this as an issue.

>  If anything I'd agree to adding a label to the start of
> subsection 1 (or .fixup),

How is this suggestion any different to what I have presented?  Each
unlikely section still gets a non-local label.

> but it's not clear how emitting such labels
> could be avoided when that section is empty (such labels would clash
> with whatever label is first in the next compilation unit).

All UNLIKELY() sections have content, as they are manually created.  Or
have I missed something?

> Maybe we
> could discard them in the symbols processing tool if their address
> matches another symbol.
>
>> Additionally, correct the inclusion of the tag parameter in the C UNLIKELY
>> blocks, to use the passed tag, rather than a literal ".tag".
> This part I agree with of course, and would therefore like to as for this
> to be split off.

I will see about this, after we have agreed on the other bits.

~Andrew

Re: [PATCH 5/6] xen/x86: Avoid using local labels for UNLIKELY() regions

[Jan Beulich]

>>> On 04.11.15 at 16:07, <andrew.cooper3@citrix.com> wrote:
> On 04/11/15 14:45, Jan Beulich wrote:
>>>>> On 30.10.15 at 20:59, <andrew.cooper3@citrix.com> wrote:
>>> Using local labels causes the stack trace to use the last non-local
>>> label emitted by the compiler in the translation unit, which is almost
>>> always unrelated.
>>>
>>> e.g. A (contrived debug) example switches from:
>>>
>>>   (XEN) ----[ Xen-4.7-unstable  x86_64  debug=y  Not tainted ]----
>>>   (XEN) CPU:    0
>>>   (XEN) RIP:    e008:[<ffff82d0801961e2>] 
>>> asm_domain_crash_synchronous+0x44/0x4c
>>>   ...
>>>   (XEN) Xen call trace:
>>>   (XEN)    [<ffff82d0801961e2>] asm_domain_crash_synchronous+0x44/0x4c
>>>   (XEN)    [<ffff82d080114bdf>] handle_keypress+0xa4/0xd9
>>>
>>> to:
>>>
>>>   (XEN) ----[ Xen-4.7-unstable  x86_64  debug=y  Not tainted ]----
>>>   (XEN) CPU:    0
>>>   (XEN) RIP:    e008:[<ffff82d0801961e2>] unlikely.vmptrld.921+0/0x8
>>>   ...
>>>   (XEN) Xen call trace:
>>>   (XEN)    [<ffff82d0801961e2>] unlikely.vmptrld.921+0/0x8
>>>   (XEN)    [<ffff82d080114bdf>] handle_keypress+0xa4/0xd9
>>>
>>> which is far more relevant when identifying %eip.
>> I disagree; I specifically used local labels here to avoid cluttering the
>> symbol table.
> 
> Needless cluttering is indeed bad.  However, the effect here is a
> misleading stack trace.
> 
> As the entire point of the symbol table is to generate usable
> information in situations like this, I don't see this as an issue.
> 
>>  If anything I'd agree to adding a label to the start of
>> subsection 1 (or .fixup),
> 
> How is this suggestion any different to what I have presented?  Each
> unlikely section still gets a non-local label.

Your proposal produces one label per code addition to the unlikely
section. My proposal generates just one label, no matter how many
contributions to the section get done in a compilation unit.

>> but it's not clear how emitting such labels
>> could be avoided when that section is empty (such labels would clash
>> with whatever label is first in the next compilation unit).
> 
> All UNLIKELY() sections have content, as they are manually created.  Or
> have I missed something?

I guess you take each .subsection as representation of a separate
section, while I take only their set per compilation unit as one (and
this being a sub-section means even that is still to wide a term).

Jan