[PATCH v2 0/5] x86: Use alternative mechanism to define CLAC/STAC

[PATCH v2 0/5] x86: Use alternative mechanism to define CLAC/STAC

[Feng Wu]

This patch set ports the basic alternative mechanism from Linux to Xen
and use it to define CLAC/STAC.

Version 1:
  * Initial version

Version 2:
  * Use STR() as the only method for performing preprocessor stringificaion.
  * Set ideal_nops to k8_nops by default
  * Make ideal_nops static and __initdata
  * Mask the lowest bit when handling relative jump in apply_alternatives()
  * Move .altinstr_replacemen and altinstructions to .init section
  * Include nops.h in alternative.h
  * Some code style changes

There are some changes related to ARM code in the first patch of this series,
but I don't have ARM environment to verify it. The changes in x86 side have
the same logic, and I verify it in x86 side.

Feng Wu (5):
  Use STR() as the only method for performing preprocessor
    stringificaion
  x86: Add definitions for NOP operation
  x86: Make set_nmi_callback return the old nmi callback
  x86: Port the basic alternative mechanism from Linux to Xen
  x86: Use alternative mechanism to define CLAC/STAC

 xen/arch/arm/domain_build.c           |   2 +-
 xen/arch/x86/Makefile                 |   1 +
 xen/arch/x86/alternative.c            | 213 ++++++++++++++++++++++++++++++++++
 xen/arch/x86/efi/boot.c               |   3 +-
 xen/arch/x86/setup.c                  |   3 +
 xen/arch/x86/traps.c                  |   4 +-
 xen/arch/x86/xen.lds.S                |  15 +++
 xen/include/asm-arm/arm32/processor.h |  10 +-
 xen/include/asm-arm/cpregs.h          |   2 -
 xen/include/asm-arm/sysregs.h         |   2 -
 xen/include/asm-x86/alternative.h     |  78 +++++++++++++
 xen/include/asm-x86/asm_defns.h       |  25 ++--
 xen/include/asm-x86/nmi.h             |   4 +-
 xen/include/asm-x86/nops.h            |  66 +++++++++++
 xen/include/xen/stringify.h           |  12 --
 15 files changed, 401 insertions(+), 39 deletions(-)
 create mode 100644 xen/arch/x86/alternative.c
 create mode 100644 xen/include/asm-x86/alternative.h
 create mode 100644 xen/include/asm-x86/nops.h
 delete mode 100644 xen/include/xen/stringify.h

-- 
1.8.3.1

[PATCH v2 1/5] Use STR() as the only method for performing preprocessor stringificaion

[Feng Wu]

There are two ways of performing preprocessor stringificaion in the
current code: __stringify() from stringify.h, and STR() from config.h.
This patch consolidates down to one.

Signed-off-by: Feng Wu <feng.wu@intel.com>
---
 xen/arch/arm/domain_build.c           |  2 +-
 xen/arch/x86/efi/boot.c               |  3 +--
 xen/include/asm-arm/arm32/processor.h | 10 +++++-----
 xen/include/asm-arm/cpregs.h          |  2 --
 xen/include/asm-arm/sysregs.h         |  2 --
 xen/include/asm-x86/asm_defns.h       |  5 ++---
 xen/include/xen/stringify.h           | 12 ------------
 7 files changed, 9 insertions(+), 27 deletions(-)
 delete mode 100644 xen/include/xen/stringify.h

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index c424793..9636097 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -324,7 +324,7 @@ static int make_hypervisor_node(struct domain *d,
                                 void *fdt, const struct dt_device_node *parent)
 {
     const char compat[] =
-        "xen,xen-"__stringify(XEN_VERSION)"."__stringify(XEN_SUBVERSION)"\0"
+        "xen,xen-"STR(XEN_VERSION)"."STR(XEN_SUBVERSION)"\0"
         "xen,xen";
     __be32 reg[4];
     gic_interrupt_t intr;
diff --git a/xen/arch/x86/efi/boot.c b/xen/arch/x86/efi/boot.c
index a772509..63874df 100644
--- a/xen/arch/x86/efi/boot.c
+++ b/xen/arch/x86/efi/boot.c
@@ -16,7 +16,6 @@
 # error Cannot use xen/pfn.h here!
 #endif
 #include <xen/string.h>
-#include <xen/stringify.h>
 #include <xen/vga.h>
 #include <asm/e820.h>
 #include <asm/edd.h>
@@ -875,7 +874,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
             StdOut->SetMode(StdOut, best);
     }
 
-    PrintStr(L"Xen " __stringify(XEN_VERSION) "." __stringify(XEN_SUBVERSION)
+    PrintStr(L"Xen " STR(XEN_VERSION) "." STR(XEN_SUBVERSION)
              XEN_EXTRAVERSION " (c/s " XEN_CHANGESET ") EFI loader\r\n");
 
     relocate_image(0);
diff --git a/xen/include/asm-arm/arm32/processor.h b/xen/include/asm-arm/arm32/processor.h
index f41644d..8fe1241 100644
--- a/xen/include/asm-arm/arm32/processor.h
+++ b/xen/include/asm-arm/arm32/processor.h
@@ -64,14 +64,14 @@ struct cpu_user_regs
 #define CP64(r, name...) __CP64(r, name)
 
 /* Stringified for inline assembly */
-#define LOAD_CP32(r, name...)  "mrc " __stringify(CP32(%r, name)) ";"
-#define STORE_CP32(r, name...) "mcr " __stringify(CP32(%r, name)) ";"
-#define LOAD_CP64(r, name...)  "mrrc " __stringify(CP64(%r, %H##r, name)) ";"
-#define STORE_CP64(r, name...) "mcrr " __stringify(CP64(%r, %H##r, name)) ";"
+#define LOAD_CP32(r, name...)  "mrc " STR(CP32(%r, name)) ";"
+#define STORE_CP32(r, name...) "mcr " STR(CP32(%r, name)) ";"
+#define LOAD_CP64(r, name...)  "mrrc " STR(CP64(%r, %H##r, name)) ";"
+#define STORE_CP64(r, name...) "mcrr " STR(CP64(%r, %H##r, name)) ";"
 
 /* Issue a CP operation which takes no argument,
  * uses r0 as a placeholder register. */
-#define CMD_CP32(name...)      "mcr " __stringify(CP32(r0, name)) ";"
+#define CMD_CP32(name...)      "mcr " STR(CP32(r0, name)) ";"
 
 #ifndef __ASSEMBLY__
 
diff --git a/xen/include/asm-arm/cpregs.h b/xen/include/asm-arm/cpregs.h
index bf8133e..db1894f 100644
--- a/xen/include/asm-arm/cpregs.h
+++ b/xen/include/asm-arm/cpregs.h
@@ -1,8 +1,6 @@
 #ifndef __ASM_ARM_CPREGS_H
 #define __ASM_ARM_CPREGS_H
 
-#include <xen/stringify.h>
-
 /*
  * AArch32 Co-processor registers.
  *
diff --git a/xen/include/asm-arm/sysregs.h b/xen/include/asm-arm/sysregs.h
index 4a4de34..ced3be7 100644
--- a/xen/include/asm-arm/sysregs.h
+++ b/xen/include/asm-arm/sysregs.h
@@ -3,8 +3,6 @@
 
 #ifdef CONFIG_ARM_64
 
-#include <xen/stringify.h>
-
 /* AArch 64 System Register Encodings */
 #define __HSR_SYSREG_c0  0
 #define __HSR_SYSREG_c1  1
diff --git a/xen/include/asm-x86/asm_defns.h b/xen/include/asm-x86/asm_defns.h
index 87a462f..0b9f898 100644
--- a/xen/include/asm-x86/asm_defns.h
+++ b/xen/include/asm-x86/asm_defns.h
@@ -8,7 +8,6 @@
 #endif
 #include <asm/processor.h>
 #include <asm/percpu.h>
-#include <xen/stringify.h>
 #include <asm/cpufeature.h>
 
 #ifndef __ASSEMBLY__
@@ -179,13 +178,13 @@ void ret_from_intr(void);
 static inline void clac(void)
 {
     if ( boot_cpu_has(X86_FEATURE_SMAP) )
-        asm volatile (__stringify(__ASM_CLAC) : : : "memory");
+        asm volatile (STR(__ASM_CLAC) : : : "memory");
 }
 
 static inline void stac(void)
 {
     if ( boot_cpu_has(X86_FEATURE_SMAP) )
-        asm volatile (__stringify(__ASM_STAC) : : : "memory");
+        asm volatile (STR(__ASM_STAC) : : : "memory");
 }
 #endif
 
diff --git a/xen/include/xen/stringify.h b/xen/include/xen/stringify.h
deleted file mode 100644
index 36808e0..0000000
--- a/xen/include/xen/stringify.h
+++ /dev/null
@@ -1,12 +0,0 @@
-#ifndef __XEN_STRINGIFY_H
-#define __XEN_STRINGIFY_H
-
-/* Indirect stringification.  Doing two levels allows the parameter to be a
- * macro itself.  For example, compile with -DFOO=bar, __stringify(FOO)
- * converts to "bar".
- */
-
-#define __stringify_1(x...) #x
-#define __stringify(x...)   __stringify_1(x)
-
-#endif /* !__XEN_STRINGIFY_H */
-- 
1.8.3.1

[PATCH v2 2/5] x86: Add definitions for NOP operation

[Feng Wu]

This patch adds definitions for different length of NOP operation.

Signed-off-by: Feng Wu <feng.wu@intel.com>
---
 xen/include/asm-x86/nops.h | 66 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 66 insertions(+)
 create mode 100644 xen/include/asm-x86/nops.h

diff --git a/xen/include/asm-x86/nops.h b/xen/include/asm-x86/nops.h
new file mode 100644
index 0000000..5809a8a
--- /dev/null
+++ b/xen/include/asm-x86/nops.h
@@ -0,0 +1,66 @@
+#ifndef __X86_ASM_NOPS_H__
+#define __X86_ASM_NOPS_H__
+
+#include <xen/config.h>
+
+/*
+ * Define nops for use with alternative().
+ */
+
+/*
+ * Opteron 64bit nops
+ * 1: nop
+ * 2: osp nop
+ * 3: osp osp nop
+ * 4: osp osp osp nop
+ */
+#define K8_NOP1 0x90
+#define K8_NOP2 0x66,K8_NOP1
+#define K8_NOP3 0x66,K8_NOP2
+#define K8_NOP4 0x66,K8_NOP3
+#define K8_NOP5 K8_NOP3,K8_NOP2
+#define K8_NOP6 K8_NOP3,K8_NOP3
+#define K8_NOP7 K8_NOP4,K8_NOP3
+#define K8_NOP8 K8_NOP4,K8_NOP4
+
+/*
+ * P6 nops
+ * uses eax dependencies (Intel-recommended choice)
+ * 1: nop
+ * 2: osp nop
+ * 3: nopl (%eax)
+ * 4: nopl 0x00(%eax)
+ * 5: nopl 0x00(%eax,%eax,1)
+ * 6: osp nopl 0x00(%eax,%eax,1)
+ * 7: nopl 0x00000000(%eax)
+ * 8: nopl 0x00000000(%eax,%eax,1)
+ *    Note: All the above are assumed to be a single instruction.
+ *          There is kernel code that depends on this.
+ */
+#define P6_NOP1 0x90
+#define P6_NOP2 0x66,0x90
+#define P6_NOP3 0x0f,0x1f,0x00
+#define P6_NOP4 0x0f,0x1f,0x40,0
+#define P6_NOP5 0x0f,0x1f,0x44,0x00,0
+#define P6_NOP6 0x66,0x0f,0x1f,0x44,0x00,0
+#define P6_NOP7 0x0f,0x1f,0x80,0,0,0,0
+#define P6_NOP8 0x0f,0x1f,0x84,0x00,0,0,0,0
+
+#ifdef __ASSEMBLY__
+#define _ASM_MK_NOP(x) .byte x
+#else
+#define _ASM_MK_NOP(x) ".byte " STR(x) "\n"
+#endif
+
+#define ASM_NOP1 _ASM_MK_NOP(K8_NOP1)
+#define ASM_NOP2 _ASM_MK_NOP(K8_NOP2)
+#define ASM_NOP3 _ASM_MK_NOP(K8_NOP3)
+#define ASM_NOP4 _ASM_MK_NOP(K8_NOP4)
+#define ASM_NOP5 _ASM_MK_NOP(K8_NOP5)
+#define ASM_NOP6 _ASM_MK_NOP(K8_NOP6)
+#define ASM_NOP7 _ASM_MK_NOP(K8_NOP7)
+#define ASM_NOP8 _ASM_MK_NOP(K8_NOP8)
+
+#define ASM_NOP_MAX 8
+
+#endif /* __X86_ASM_NOPS_H__ */
-- 
1.8.3.1

[PATCH v2 3/5] x86: Make set_nmi_callback return the old nmi callback

[Feng Wu]

This patch makes set_nmi_callback return the old nmi callback, so
we can set it back later.

Signed-off-by: Feng Wu <feng.wu@intel.com>
---
 xen/arch/x86/traps.c      | 4 +++-
 xen/include/asm-x86/nmi.h | 4 ++--
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 1722912..171cff0 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -3322,9 +3322,11 @@ void do_nmi(struct cpu_user_regs *regs)
     }
 }
 
-void set_nmi_callback(nmi_callback_t callback)
+nmi_callback_t set_nmi_callback(nmi_callback_t callback)
 {
+    nmi_callback_t old_nmi_callback = nmi_callback;
     nmi_callback = callback;
+    return old_nmi_callback;
 }
 
 void unset_nmi_callback(void)
diff --git a/xen/include/asm-x86/nmi.h b/xen/include/asm-x86/nmi.h
index 98b5e04..58cd9a1 100644
--- a/xen/include/asm-x86/nmi.h
+++ b/xen/include/asm-x86/nmi.h
@@ -15,9 +15,9 @@ typedef int (*nmi_callback_t)(struct cpu_user_regs *regs, int cpu);
  * set_nmi_callback
  *
  * Set a handler for an NMI. Only one handler may be
- * set. Return 1 if the NMI was handled.
+ * set. Return the old nmi callback handler.
  */
-void set_nmi_callback(nmi_callback_t callback);
+nmi_callback_t set_nmi_callback(nmi_callback_t callback);
  
 /** 
  * unset_nmi_callback
-- 
1.8.3.1

[PATCH v2 4/5] x86: Port the basic alternative mechanism from Linux to Xen

[Feng Wu]

This patch ports the basic alternative mechanism from Linux to Xen.
With this mechanism, we can patch code based on the CPU features.

Signed-off-by: Feng Wu <feng.wu@intel.com>
---
 xen/arch/x86/Makefile             |   1 +
 xen/arch/x86/alternative.c        | 213 ++++++++++++++++++++++++++++++++++++++
 xen/arch/x86/setup.c              |   3 +
 xen/arch/x86/xen.lds.S            |  15 +++
 xen/include/asm-x86/alternative.h |  78 ++++++++++++++
 5 files changed, 310 insertions(+)
 create mode 100644 xen/arch/x86/alternative.c
 create mode 100644 xen/include/asm-x86/alternative.h

diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index d502bdf..3734884 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -58,6 +58,7 @@ obj-y += crash.o
 obj-y += tboot.o
 obj-y += hpet.o
 obj-y += xstate.o
+obj-y += alternative.o
 
 obj-$(crash_debug) += gdbstub.o
 
diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
new file mode 100644
index 0000000..3dbc811
--- /dev/null
+++ b/xen/arch/x86/alternative.c
@@ -0,0 +1,213 @@
+/******************************************************************************
+ * alternative.c
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
+ */
+
+#include <xen/types.h>
+#include <asm/processor.h>
+#include <asm/alternative.h>
+#include <xen/init.h>
+#include <asm/system.h>
+#include <asm/traps.h>
+#include <asm/nmi.h>
+
+#define MAX_PATCH_LEN (255-1)
+
+extern struct alt_instr __alt_instructions[], __alt_instructions_end[];
+
+#ifdef K8_NOP1
+static const unsigned char k8nops[] __initconst = {
+    K8_NOP1,
+    K8_NOP2,
+    K8_NOP3,
+    K8_NOP4,
+    K8_NOP5,
+    K8_NOP6,
+    K8_NOP7,
+    K8_NOP8
+};
+static const unsigned char * const k8_nops[ASM_NOP_MAX+1] __initconst = {
+    NULL,
+    k8nops,
+    k8nops + 1,
+    k8nops + 1 + 2,
+    k8nops + 1 + 2 + 3,
+    k8nops + 1 + 2 + 3 + 4,
+    k8nops + 1 + 2 + 3 + 4 + 5,
+    k8nops + 1 + 2 + 3 + 4 + 5 + 6,
+    k8nops + 1 + 2 + 3 + 4 + 5 + 6 + 7
+};
+#endif
+
+#ifdef P6_NOP1
+static const unsigned char p6nops[] __initconst = {
+    P6_NOP1,
+    P6_NOP2,
+    P6_NOP3,
+    P6_NOP4,
+    P6_NOP5,
+    P6_NOP6,
+    P6_NOP7,
+    P6_NOP8
+};
+static const unsigned char * const p6_nops[ASM_NOP_MAX+1] __initconst = {
+    NULL,
+    p6nops,
+    p6nops + 1,
+    p6nops + 1 + 2,
+    p6nops + 1 + 2 + 3,
+    p6nops + 1 + 2 + 3 + 4,
+    p6nops + 1 + 2 + 3 + 4 + 5,
+    p6nops + 1 + 2 + 3 + 4 + 5 + 6,
+    p6nops + 1 + 2 + 3 + 4 + 5 + 6 + 7
+};
+#endif
+
+static const unsigned char * const *ideal_nops __initdata = k8_nops;
+
+static int __init mask_nmi_callback(struct cpu_user_regs *regs, int cpu)
+{
+    return 1;
+}
+
+static void __init arch_init_ideal_nops(void)
+{
+    /*
+     * Due to a decoder implementation quirk, some
+     * specific Intel CPUs actually perform better with
+     * the "k8_nops" than with the SDM-recommended NOPs.
+     */
+    if ( (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL) &&
+         !(boot_cpu_data.x86 == 6 &&
+           boot_cpu_data.x86_model >= 0x0f &&
+           boot_cpu_data.x86_model != 0x1c &&
+           boot_cpu_data.x86_model != 0x26 &&
+           boot_cpu_data.x86_model != 0x27 &&
+           boot_cpu_data.x86_model < 0x30) )
+        ideal_nops = p6_nops;
+}
+
+/* Use this to add nops to a buffer, then text_poke the whole buffer. */
+static void __init add_nops(void *insns, unsigned int len)
+{
+    while ( len > 0 )
+    {
+        unsigned int noplen = len;
+        if ( noplen > ASM_NOP_MAX )
+            noplen = ASM_NOP_MAX;
+        memcpy(insns, ideal_nops[noplen], noplen);
+        insns += noplen;
+        len -= noplen;
+    }
+}
+
+/*
+ * text_poke_early - Update instructions on a live kernel at boot time
+ * @addr: address to modify
+ * @opcode: source of the copy
+ * @len: length to copy
+ *
+ * When you use this code to patch more than one byte of an instruction
+ * you need to make sure that other CPUs cannot execute this code in parallel.
+ * Also no thread must be currently preempted in the middle of these
+ * instructions. And on the local CPU you need to be protected again NMI or MCE
+ * handlers seeing an inconsistent instruction while you patch.
+ */
+static void *__init text_poke_early(void *addr, const void *opcode, size_t len)
+{
+    unsigned long flags;
+
+    local_irq_save(flags);
+    memcpy(addr, opcode, len);
+    sync_core();
+    local_irq_restore(flags);
+
+    return addr;
+}
+
+/*
+ * Replace instructions with better alternatives for this CPU type.
+ * This runs before SMP is initialized to avoid SMP problems with
+ * self modifying code. This implies that asymmetric systems where
+ * APs have less capabilities than the boot processor are not handled.
+ * Tough. Make sure you disable such features by hand.
+ */
+
+static void __init apply_alternatives(struct alt_instr *start, struct alt_instr *end)
+{
+    struct alt_instr *a;
+    u8 *instr, *replacement;
+    u8 insnbuf[MAX_PATCH_LEN];
+
+    printk(KERN_INFO "alt table %p -> %p\n", start, end);
+
+    /*
+     * The scan order should be from start to end. A later scanned
+     * alternative code can overwrite a previous scanned alternative code.
+     * Some kernel functions (e.g. memcpy, memset, etc) use this order to
+     * patch code.
+     *
+     * So be careful if you want to change the scan order to any other
+     * order.
+     */
+    for ( a = start; a < end; a++ )
+    {
+        instr = (u8 *)&a->instr_offset + a->instr_offset;
+        replacement = (u8 *)&a->repl_offset + a->repl_offset;
+        BUG_ON(a->replacementlen > a->instrlen);
+        BUG_ON(a->instrlen > sizeof(insnbuf));
+        BUG_ON(a->cpuid >= NCAPINTS * 32);
+        if ( !boot_cpu_has(a->cpuid) )
+            continue;
+
+        memcpy(insnbuf, replacement, a->replacementlen);
+
+        /* 0xe8/0xe9 is a relative jump; fix the offset. */
+        if ( (*insnbuf & 0xfe) == 0xe8 && a->replacementlen == 5 )
+            *(s32 *)(insnbuf + 1) += replacement - instr;
+
+        add_nops(insnbuf + a->replacementlen,
+                 a->instrlen - a->replacementlen);
+        text_poke_early(instr, insnbuf, a->instrlen);
+    }
+}
+
+void __init alternative_instructions(void)
+{
+    nmi_callback_t saved_nmi_callback;
+
+    arch_init_ideal_nops();
+
+    /*
+     * The patching is not fully atomic, so try to avoid local interruptions
+     * that might execute the to be patched code.
+     * Other CPUs are not running.
+     */
+    saved_nmi_callback = set_nmi_callback(mask_nmi_callback);
+    /*
+     * Don't stop machine check exceptions while patching.
+     * MCEs only happen when something got corrupted and in this
+     * case we must do something about the corruption.
+     * Ignoring it is worse than a unlikely patching race.
+     * Also machine checks tend to be broadcast and if one CPU
+     * goes into machine check the others follow quickly, so we don't
+     * expect a machine check to cause undue problems during to code
+     * patching.
+     */
+
+    apply_alternatives(__alt_instructions, __alt_instructions_end);
+    set_nmi_callback(saved_nmi_callback);
+}
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 508649d..d16453a 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -48,6 +48,7 @@
 #include <asm/setup.h>
 #include <xen/cpu.h>
 #include <asm/nmi.h>
+#include <asm/alternative.h>
 
 /* opt_nosmp: If true, secondary processors are ignored. */
 static bool_t __initdata opt_nosmp;
@@ -1288,6 +1289,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
     if ( cpu_has_fsgsbase )
         set_in_cr4(X86_CR4_FSGSBASE);
 
+    alternative_instructions();
+
     local_irq_enable();
 
     pt_pci_init();
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 17db361..d4b1f1a 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -105,6 +105,12 @@ SECTIONS
   .init.text : {
        _sinittext = .;
        *(.init.text)
+       /*
+        * Here are the replacement instructions. The linker sticks them
+        * as binary blobs. The .altinstructions has enough data to get
+        * the address and the length of them to patch the kernel safely.
+        */
+       *(.altinstr_replacement)
        _einittext = .;
   } :text
   .init.data : {
@@ -120,6 +126,15 @@ SECTIONS
        __trampoline_seg_start = .;
        *(.trampoline_seg)
        __trampoline_seg_stop = .;
+       /*
+        * struct alt_inst entries. From the header (alternative.h):
+        * "Alternative instructions for different CPU types or capabilities"
+        * Think locking instructions on spinlocks.
+        */
+       . = ALIGN(8);
+        __alt_instructions = .;
+        *(.altinstructions)
+        __alt_instructions_end = .;
 
        . = ALIGN(8);
        __ctors_start = .;
diff --git a/xen/include/asm-x86/alternative.h b/xen/include/asm-x86/alternative.h
new file mode 100644
index 0000000..55a6604
--- /dev/null
+++ b/xen/include/asm-x86/alternative.h
@@ -0,0 +1,78 @@
+#ifndef __X86_ALTERNATIVE_H__
+#define __X86_ALTERNATIVE_H__
+
+#include <asm/nops.h>
+
+#ifdef __ASSEMBLY__
+.macro altinstruction_entry orig alt feature orig_len alt_len
+        .long \orig - .
+        .long \alt - .
+        .word \feature
+        .byte \orig_len
+        .byte \alt_len
+.endm
+#else
+#include <xen/types.h>
+
+struct alt_instr {
+    s32 instr_offset;       /* original instruction */
+    s32 repl_offset;        /* offset to replacement instruction */
+    u16 cpuid;              /* cpuid bit set for replacement */
+    u8  instrlen;           /* length of original instruction */
+    u8  replacementlen;     /* length of new instruction, <= instrlen */
+};
+
+extern void alternative_instructions(void);
+
+#define OLDINSTR(oldinstr)      "661:\n\t" oldinstr "\n662:\n"
+
+#define b_replacement(number)   "663"#number
+#define e_replacement(number)   "664"#number
+
+#define alt_slen "662b-661b"
+#define alt_rlen(number) e_replacement(number)"f-"b_replacement(number)"f"
+
+#define ALTINSTR_ENTRY(feature, number)                                       \
+        " .long 661b - .\n"                             /* label           */ \
+        " .long " b_replacement(number)"f - .\n"        /* new instruction */ \
+        " .word " STR(feature) "\n"                     /* feature bit     */ \
+        " .byte " alt_slen "\n"                         /* source len      */ \
+        " .byte " alt_rlen(number) "\n"                 /* replacement len */
+
+#define DISCARD_ENTRY(number)                           /* rlen <= slen */    \
+        " .byte 0xff + (" alt_rlen(number) ") - (" alt_slen ")\n"
+
+#define ALTINSTR_REPLACEMENT(newinstr, feature, number) /* replacement */     \
+        b_replacement(number)":\n\t" newinstr "\n" e_replacement(number) ":\n\t"
+
+/* alternative assembly primitive: */
+#define ALTERNATIVE(oldinstr, newinstr, feature)                        \
+        OLDINSTR(oldinstr)                                              \
+        ".pushsection .altinstructions,\"a\"\n"                         \
+        ALTINSTR_ENTRY(feature, 1)                                      \
+        ".popsection\n"                                                 \
+        ".pushsection .discard,\"aw\",@progbits\n"                      \
+        DISCARD_ENTRY(1)                                                \
+        ".popsection\n"                                                 \
+        ".pushsection .altinstr_replacement, \"ax\"\n"                  \
+        ALTINSTR_REPLACEMENT(newinstr, feature, 1)                      \
+        ".popsection"
+
+/*
+ * Alternative instructions for different CPU types or capabilities.
+ *
+ * This allows to use optimized instructions even on generic binary
+ * kernels.
+ *
+ * length of oldinstr must be longer or equal the length of newinstr
+ * It can be padded with nops as needed.
+ *
+ * For non barrier like inlines please define new variants
+ * without volatile and memory clobber.
+ */
+#define alternative(oldinstr, newinstr, feature)                        \
+        asm volatile (ALTERNATIVE(oldinstr, newinstr, feature) : : : "memory")
+
+#endif  /*  __ASSEMBLY__  */
+
+#endif /* __X86_ALTERNATIVE_H__ */
-- 
1.8.3.1

[PATCH v2 5/5] x86: Use alternative mechanism to define CLAC/STAC

[Feng Wu]

This patch use alternative mechanism to define CLAC/STAC.

Signed-off-by: Feng Wu <feng.wu@intel.com>
---
 xen/include/asm-x86/asm_defns.h | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/xen/include/asm-x86/asm_defns.h b/xen/include/asm-x86/asm_defns.h
index 0b9f898..dd50b97 100644
--- a/xen/include/asm-x86/asm_defns.h
+++ b/xen/include/asm-x86/asm_defns.h
@@ -9,6 +9,7 @@
 #include <asm/processor.h>
 #include <asm/percpu.h>
 #include <asm/cpufeature.h>
+#include <asm/alternative.h>
 
 #ifndef __ASSEMBLY__
 void ret_from_intr(void);
@@ -165,26 +166,27 @@ void ret_from_intr(void);
 #define __ASM_STAC      .byte 0x0f,0x01,0xcb
 
 #ifdef __ASSEMBLY__
-#define ASM_AC(op)                                       \
-        btl $X86_FEATURE_SMAP & 31,                      \
-        CPUINFO_FEATURE_OFFSET(X86_FEATURE_SMAP)+boot_cpu_data(%rip); \
-        jnc 881f;                                        \
-        __ASM_##op;                                      \
-881:
-
+#define ASM_AC(op)                                                     \
+        661: ASM_NOP3;                                                 \
+        .pushsection .altinstr_replacement, "ax";                      \
+        662: __ASM_##op;                                               \
+        .popsection;                                                   \
+        .pushsection .altinstructions, "a";                            \
+        altinstruction_entry 661b, 662b, X86_FEATURE_SMAP, 3, 3;       \
+        .popsection
 #define ASM_STAC ASM_AC(STAC)
 #define ASM_CLAC ASM_AC(CLAC)
 #else
 static inline void clac(void)
 {
-    if ( boot_cpu_has(X86_FEATURE_SMAP) )
-        asm volatile (STR(__ASM_CLAC) : : : "memory");
+    /* Note: a barrier is implicit in alternative() */
+    alternative(ASM_NOP3, STR(__ASM_CLAC), X86_FEATURE_SMAP);
 }
 
 static inline void stac(void)
 {
-    if ( boot_cpu_has(X86_FEATURE_SMAP) )
-        asm volatile (STR(__ASM_STAC) : : : "memory");
+    /* Note: a barrier is implicit in alternative() */
+    alternative(ASM_NOP3, STR(__ASM_STAC), X86_FEATURE_SMAP);
 }
 #endif
 
-- 
1.8.3.1

Re: [PATCH v2 1/5] Use STR() as the only method for performing preprocessor stringificaion

[Andrew Cooper]

On 29/05/2014 06:34, Feng Wu wrote:
> There are two ways of performing preprocessor stringificaion in the
> current code: __stringify() from stringify.h, and STR() from config.h.
> This patch consolidates down to one.
>
> Signed-off-by: Feng Wu <feng.wu@intel.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

> ---
>   xen/arch/arm/domain_build.c           |  2 +-
>   xen/arch/x86/efi/boot.c               |  3 +--
>   xen/include/asm-arm/arm32/processor.h | 10 +++++-----
>   xen/include/asm-arm/cpregs.h          |  2 --
>   xen/include/asm-arm/sysregs.h         |  2 --
>   xen/include/asm-x86/asm_defns.h       |  5 ++---
>   xen/include/xen/stringify.h           | 12 ------------
>   7 files changed, 9 insertions(+), 27 deletions(-)
>   delete mode 100644 xen/include/xen/stringify.h
>
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index c424793..9636097 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -324,7 +324,7 @@ static int make_hypervisor_node(struct domain *d,
>                                   void *fdt, const struct dt_device_node *parent)
>   {
>       const char compat[] =
> -        "xen,xen-"__stringify(XEN_VERSION)"."__stringify(XEN_SUBVERSION)"\0"
> +        "xen,xen-"STR(XEN_VERSION)"."STR(XEN_SUBVERSION)"\0"
>           "xen,xen";
>       __be32 reg[4];
>       gic_interrupt_t intr;
> diff --git a/xen/arch/x86/efi/boot.c b/xen/arch/x86/efi/boot.c
> index a772509..63874df 100644
> --- a/xen/arch/x86/efi/boot.c
> +++ b/xen/arch/x86/efi/boot.c
> @@ -16,7 +16,6 @@
>   # error Cannot use xen/pfn.h here!
>   #endif
>   #include <xen/string.h>
> -#include <xen/stringify.h>
>   #include <xen/vga.h>
>   #include <asm/e820.h>
>   #include <asm/edd.h>
> @@ -875,7 +874,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
>               StdOut->SetMode(StdOut, best);
>       }
>   
> -    PrintStr(L"Xen " __stringify(XEN_VERSION) "." __stringify(XEN_SUBVERSION)
> +    PrintStr(L"Xen " STR(XEN_VERSION) "." STR(XEN_SUBVERSION)
>                XEN_EXTRAVERSION " (c/s " XEN_CHANGESET ") EFI loader\r\n");
>   
>       relocate_image(0);
> diff --git a/xen/include/asm-arm/arm32/processor.h b/xen/include/asm-arm/arm32/processor.h
> index f41644d..8fe1241 100644
> --- a/xen/include/asm-arm/arm32/processor.h
> +++ b/xen/include/asm-arm/arm32/processor.h
> @@ -64,14 +64,14 @@ struct cpu_user_regs
>   #define CP64(r, name...) __CP64(r, name)
>   
>   /* Stringified for inline assembly */
> -#define LOAD_CP32(r, name...)  "mrc " __stringify(CP32(%r, name)) ";"
> -#define STORE_CP32(r, name...) "mcr " __stringify(CP32(%r, name)) ";"
> -#define LOAD_CP64(r, name...)  "mrrc " __stringify(CP64(%r, %H##r, name)) ";"
> -#define STORE_CP64(r, name...) "mcrr " __stringify(CP64(%r, %H##r, name)) ";"
> +#define LOAD_CP32(r, name...)  "mrc " STR(CP32(%r, name)) ";"
> +#define STORE_CP32(r, name...) "mcr " STR(CP32(%r, name)) ";"
> +#define LOAD_CP64(r, name...)  "mrrc " STR(CP64(%r, %H##r, name)) ";"
> +#define STORE_CP64(r, name...) "mcrr " STR(CP64(%r, %H##r, name)) ";"
>   
>   /* Issue a CP operation which takes no argument,
>    * uses r0 as a placeholder register. */
> -#define CMD_CP32(name...)      "mcr " __stringify(CP32(r0, name)) ";"
> +#define CMD_CP32(name...)      "mcr " STR(CP32(r0, name)) ";"
>   
>   #ifndef __ASSEMBLY__
>   
> diff --git a/xen/include/asm-arm/cpregs.h b/xen/include/asm-arm/cpregs.h
> index bf8133e..db1894f 100644
> --- a/xen/include/asm-arm/cpregs.h
> +++ b/xen/include/asm-arm/cpregs.h
> @@ -1,8 +1,6 @@
>   #ifndef __ASM_ARM_CPREGS_H
>   #define __ASM_ARM_CPREGS_H
>   
> -#include <xen/stringify.h>
> -
>   /*
>    * AArch32 Co-processor registers.
>    *
> diff --git a/xen/include/asm-arm/sysregs.h b/xen/include/asm-arm/sysregs.h
> index 4a4de34..ced3be7 100644
> --- a/xen/include/asm-arm/sysregs.h
> +++ b/xen/include/asm-arm/sysregs.h
> @@ -3,8 +3,6 @@
>   
>   #ifdef CONFIG_ARM_64
>   
> -#include <xen/stringify.h>
> -
>   /* AArch 64 System Register Encodings */
>   #define __HSR_SYSREG_c0  0
>   #define __HSR_SYSREG_c1  1
> diff --git a/xen/include/asm-x86/asm_defns.h b/xen/include/asm-x86/asm_defns.h
> index 87a462f..0b9f898 100644
> --- a/xen/include/asm-x86/asm_defns.h
> +++ b/xen/include/asm-x86/asm_defns.h
> @@ -8,7 +8,6 @@
>   #endif
>   #include <asm/processor.h>
>   #include <asm/percpu.h>
> -#include <xen/stringify.h>
>   #include <asm/cpufeature.h>
>   
>   #ifndef __ASSEMBLY__
> @@ -179,13 +178,13 @@ void ret_from_intr(void);
>   static inline void clac(void)
>   {
>       if ( boot_cpu_has(X86_FEATURE_SMAP) )
> -        asm volatile (__stringify(__ASM_CLAC) : : : "memory");
> +        asm volatile (STR(__ASM_CLAC) : : : "memory");
>   }
>   
>   static inline void stac(void)
>   {
>       if ( boot_cpu_has(X86_FEATURE_SMAP) )
> -        asm volatile (__stringify(__ASM_STAC) : : : "memory");
> +        asm volatile (STR(__ASM_STAC) : : : "memory");
>   }
>   #endif
>   
> diff --git a/xen/include/xen/stringify.h b/xen/include/xen/stringify.h
> deleted file mode 100644
> index 36808e0..0000000
> --- a/xen/include/xen/stringify.h
> +++ /dev/null
> @@ -1,12 +0,0 @@
> -#ifndef __XEN_STRINGIFY_H
> -#define __XEN_STRINGIFY_H
> -
> -/* Indirect stringification.  Doing two levels allows the parameter to be a
> - * macro itself.  For example, compile with -DFOO=bar, __stringify(FOO)
> - * converts to "bar".
> - */
> -
> -#define __stringify_1(x...) #x
> -#define __stringify(x...)   __stringify_1(x)
> -
> -#endif /* !__XEN_STRINGIFY_H */

Re: [PATCH v2 3/5] x86: Make set_nmi_callback return the old nmi callback

[Andrew Cooper]

On 29/05/2014 06:34, Feng Wu wrote:
> This patch makes set_nmi_callback return the old nmi callback, so
> we can set it back later.
>
> Signed-off-by: Feng Wu <feng.wu@intel.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

> ---
>   xen/arch/x86/traps.c      | 4 +++-
>   xen/include/asm-x86/nmi.h | 4 ++--
>   2 files changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> index 1722912..171cff0 100644
> --- a/xen/arch/x86/traps.c
> +++ b/xen/arch/x86/traps.c
> @@ -3322,9 +3322,11 @@ void do_nmi(struct cpu_user_regs *regs)
>       }
>   }
>   
> -void set_nmi_callback(nmi_callback_t callback)
> +nmi_callback_t set_nmi_callback(nmi_callback_t callback)
>   {
> +    nmi_callback_t old_nmi_callback = nmi_callback;
>       nmi_callback = callback;
> +    return old_nmi_callback;
>   }
>   
>   void unset_nmi_callback(void)
> diff --git a/xen/include/asm-x86/nmi.h b/xen/include/asm-x86/nmi.h
> index 98b5e04..58cd9a1 100644
> --- a/xen/include/asm-x86/nmi.h
> +++ b/xen/include/asm-x86/nmi.h
> @@ -15,9 +15,9 @@ typedef int (*nmi_callback_t)(struct cpu_user_regs *regs, int cpu);
>    * set_nmi_callback
>    *
>    * Set a handler for an NMI. Only one handler may be
> - * set. Return 1 if the NMI was handled.
> + * set. Return the old nmi callback handler.
>    */
> -void set_nmi_callback(nmi_callback_t callback);
> +nmi_callback_t set_nmi_callback(nmi_callback_t callback);
>    
>   /**
>    * unset_nmi_callback

Re: [PATCH v2 2/5] x86: Add definitions for NOP operation

[Andrew Cooper]

On 29/05/2014 06:34, Feng Wu wrote:
> This patch adds definitions for different length of NOP operation.
>
> Signed-off-by: Feng Wu <feng.wu@intel.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

> ---
>   xen/include/asm-x86/nops.h | 66 ++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 66 insertions(+)
>   create mode 100644 xen/include/asm-x86/nops.h
>
> diff --git a/xen/include/asm-x86/nops.h b/xen/include/asm-x86/nops.h
> new file mode 100644
> index 0000000..5809a8a
> --- /dev/null
> +++ b/xen/include/asm-x86/nops.h
> @@ -0,0 +1,66 @@
> +#ifndef __X86_ASM_NOPS_H__
> +#define __X86_ASM_NOPS_H__
> +
> +#include <xen/config.h>
> +
> +/*
> + * Define nops for use with alternative().
> + */
> +
> +/*
> + * Opteron 64bit nops
> + * 1: nop
> + * 2: osp nop
> + * 3: osp osp nop
> + * 4: osp osp osp nop
> + */
> +#define K8_NOP1 0x90
> +#define K8_NOP2 0x66,K8_NOP1
> +#define K8_NOP3 0x66,K8_NOP2
> +#define K8_NOP4 0x66,K8_NOP3
> +#define K8_NOP5 K8_NOP3,K8_NOP2
> +#define K8_NOP6 K8_NOP3,K8_NOP3
> +#define K8_NOP7 K8_NOP4,K8_NOP3
> +#define K8_NOP8 K8_NOP4,K8_NOP4
> +
> +/*
> + * P6 nops
> + * uses eax dependencies (Intel-recommended choice)
> + * 1: nop
> + * 2: osp nop
> + * 3: nopl (%eax)
> + * 4: nopl 0x00(%eax)
> + * 5: nopl 0x00(%eax,%eax,1)
> + * 6: osp nopl 0x00(%eax,%eax,1)
> + * 7: nopl 0x00000000(%eax)
> + * 8: nopl 0x00000000(%eax,%eax,1)
> + *    Note: All the above are assumed to be a single instruction.
> + *          There is kernel code that depends on this.
> + */
> +#define P6_NOP1 0x90
> +#define P6_NOP2 0x66,0x90
> +#define P6_NOP3 0x0f,0x1f,0x00
> +#define P6_NOP4 0x0f,0x1f,0x40,0
> +#define P6_NOP5 0x0f,0x1f,0x44,0x00,0
> +#define P6_NOP6 0x66,0x0f,0x1f,0x44,0x00,0
> +#define P6_NOP7 0x0f,0x1f,0x80,0,0,0,0
> +#define P6_NOP8 0x0f,0x1f,0x84,0x00,0,0,0,0
> +
> +#ifdef __ASSEMBLY__
> +#define _ASM_MK_NOP(x) .byte x
> +#else
> +#define _ASM_MK_NOP(x) ".byte " STR(x) "\n"
> +#endif
> +
> +#define ASM_NOP1 _ASM_MK_NOP(K8_NOP1)
> +#define ASM_NOP2 _ASM_MK_NOP(K8_NOP2)
> +#define ASM_NOP3 _ASM_MK_NOP(K8_NOP3)
> +#define ASM_NOP4 _ASM_MK_NOP(K8_NOP4)
> +#define ASM_NOP5 _ASM_MK_NOP(K8_NOP5)
> +#define ASM_NOP6 _ASM_MK_NOP(K8_NOP6)
> +#define ASM_NOP7 _ASM_MK_NOP(K8_NOP7)
> +#define ASM_NOP8 _ASM_MK_NOP(K8_NOP8)
> +
> +#define ASM_NOP_MAX 8
> +
> +#endif /* __X86_ASM_NOPS_H__ */

Re: [PATCH v2 4/5] x86: Port the basic alternative mechanism from Linux to Xen

[Andrew Cooper]

On 29/05/2014 06:34, Feng Wu wrote:
> This patch ports the basic alternative mechanism from Linux to Xen.
> With this mechanism, we can patch code based on the CPU features.
>
> Signed-off-by: Feng Wu <feng.wu@intel.com>
> ---
>   xen/arch/x86/Makefile             |   1 +
>   xen/arch/x86/alternative.c        | 213 ++++++++++++++++++++++++++++++++++++++
>   xen/arch/x86/setup.c              |   3 +
>   xen/arch/x86/xen.lds.S            |  15 +++
>   xen/include/asm-x86/alternative.h |  78 ++++++++++++++
>   5 files changed, 310 insertions(+)
>   create mode 100644 xen/arch/x86/alternative.c
>   create mode 100644 xen/include/asm-x86/alternative.h
>
> diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> index d502bdf..3734884 100644
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -58,6 +58,7 @@ obj-y += crash.o
>   obj-y += tboot.o
>   obj-y += hpet.o
>   obj-y += xstate.o
> +obj-y += alternative.o
>   
>   obj-$(crash_debug) += gdbstub.o
>   
> diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
> new file mode 100644
> index 0000000..3dbc811
> --- /dev/null
> +++ b/xen/arch/x86/alternative.c
> @@ -0,0 +1,213 @@
> +/******************************************************************************
> + * alternative.c
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
> + */
> +
> +#include <xen/types.h>
> +#include <asm/processor.h>
> +#include <asm/alternative.h>
> +#include <xen/init.h>
> +#include <asm/system.h>
> +#include <asm/traps.h>
> +#include <asm/nmi.h>
> +
> +#define MAX_PATCH_LEN (255-1)
> +
> +extern struct alt_instr __alt_instructions[], __alt_instructions_end[];
> +
> +#ifdef K8_NOP1
> +static const unsigned char k8nops[] __initconst = {
> +    K8_NOP1,
> +    K8_NOP2,
> +    K8_NOP3,
> +    K8_NOP4,
> +    K8_NOP5,
> +    K8_NOP6,
> +    K8_NOP7,
> +    K8_NOP8
> +};
> +static const unsigned char * const k8_nops[ASM_NOP_MAX+1] __initconst = {
> +    NULL,
> +    k8nops,
> +    k8nops + 1,
> +    k8nops + 1 + 2,
> +    k8nops + 1 + 2 + 3,
> +    k8nops + 1 + 2 + 3 + 4,
> +    k8nops + 1 + 2 + 3 + 4 + 5,
> +    k8nops + 1 + 2 + 3 + 4 + 5 + 6,
> +    k8nops + 1 + 2 + 3 + 4 + 5 + 6 + 7
> +};
> +#endif
> +
> +#ifdef P6_NOP1
> +static const unsigned char p6nops[] __initconst = {
> +    P6_NOP1,
> +    P6_NOP2,
> +    P6_NOP3,
> +    P6_NOP4,
> +    P6_NOP5,
> +    P6_NOP6,
> +    P6_NOP7,
> +    P6_NOP8
> +};
> +static const unsigned char * const p6_nops[ASM_NOP_MAX+1] __initconst = {
> +    NULL,
> +    p6nops,
> +    p6nops + 1,
> +    p6nops + 1 + 2,
> +    p6nops + 1 + 2 + 3,
> +    p6nops + 1 + 2 + 3 + 4,
> +    p6nops + 1 + 2 + 3 + 4 + 5,
> +    p6nops + 1 + 2 + 3 + 4 + 5 + 6,
> +    p6nops + 1 + 2 + 3 + 4 + 5 + 6 + 7
> +};
> +#endif
> +
> +static const unsigned char * const *ideal_nops __initdata = k8_nops;
> +
> +static int __init mask_nmi_callback(struct cpu_user_regs *regs, int cpu)
> +{
> +    return 1;
> +}
> +
> +static void __init arch_init_ideal_nops(void)
> +{
> +    /*
> +     * Due to a decoder implementation quirk, some
> +     * specific Intel CPUs actually perform better with
> +     * the "k8_nops" than with the SDM-recommended NOPs.
> +     */
> +    if ( (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL) &&
> +         !(boot_cpu_data.x86 == 6 &&
> +           boot_cpu_data.x86_model >= 0x0f &&
> +           boot_cpu_data.x86_model != 0x1c &&
> +           boot_cpu_data.x86_model != 0x26 &&
> +           boot_cpu_data.x86_model != 0x27 &&
> +           boot_cpu_data.x86_model < 0x30) )
> +        ideal_nops = p6_nops;
> +}
> +
> +/* Use this to add nops to a buffer, then text_poke the whole buffer. */
> +static void __init add_nops(void *insns, unsigned int len)
> +{
> +    while ( len > 0 )
> +    {
> +        unsigned int noplen = len;
> +        if ( noplen > ASM_NOP_MAX )
> +            noplen = ASM_NOP_MAX;
> +        memcpy(insns, ideal_nops[noplen], noplen);
> +        insns += noplen;
> +        len -= noplen;
> +    }
> +}
> +
> +/*
> + * text_poke_early - Update instructions on a live kernel at boot time
> + * @addr: address to modify
> + * @opcode: source of the copy
> + * @len: length to copy
> + *
> + * When you use this code to patch more than one byte of an instruction
> + * you need to make sure that other CPUs cannot execute this code in parallel.
> + * Also no thread must be currently preempted in the middle of these
> + * instructions. And on the local CPU you need to be protected again NMI or MCE
> + * handlers seeing an inconsistent instruction while you patch.
> + */
> +static void *__init text_poke_early(void *addr, const void *opcode, size_t len)
> +{
> +    unsigned long flags;
> +
> +    local_irq_save(flags);
> +    memcpy(addr, opcode, len);
> +    sync_core();
> +    local_irq_restore(flags);
> +
> +    return addr;
> +}
> +
> +/*
> + * Replace instructions with better alternatives for this CPU type.
> + * This runs before SMP is initialized to avoid SMP problems with
> + * self modifying code. This implies that asymmetric systems where
> + * APs have less capabilities than the boot processor are not handled.
> + * Tough. Make sure you disable such features by hand.
> + */
> +

Excess newline

> +static void __init apply_alternatives(struct alt_instr *start, struct alt_instr *end)
> +{
> +    struct alt_instr *a;
> +    u8 *instr, *replacement;
> +    u8 insnbuf[MAX_PATCH_LEN];
> +
> +    printk(KERN_INFO "alt table %p -> %p\n", start, end);
> +
> +    /*
> +     * The scan order should be from start to end. A later scanned
> +     * alternative code can overwrite a previous scanned alternative code.
> +     * Some kernel functions (e.g. memcpy, memset, etc) use this order to
> +     * patch code.
> +     *
> +     * So be careful if you want to change the scan order to any other
> +     * order.
> +     */
> +    for ( a = start; a < end; a++ )
> +    {
> +        instr = (u8 *)&a->instr_offset + a->instr_offset;
> +        replacement = (u8 *)&a->repl_offset + a->repl_offset;
> +        BUG_ON(a->replacementlen > a->instrlen);
> +        BUG_ON(a->instrlen > sizeof(insnbuf));
> +        BUG_ON(a->cpuid >= NCAPINTS * 32);
> +        if ( !boot_cpu_has(a->cpuid) )
> +            continue;
> +
> +        memcpy(insnbuf, replacement, a->replacementlen);
> +
> +        /* 0xe8/0xe9 is a relative jump; fix the offset. */
> +        if ( (*insnbuf & 0xfe) == 0xe8 && a->replacementlen == 5 )
> +            *(s32 *)(insnbuf + 1) += replacement - instr;
> +
> +        add_nops(insnbuf + a->replacementlen,
> +                 a->instrlen - a->replacementlen);
> +        text_poke_early(instr, insnbuf, a->instrlen);
> +    }
> +}
> +
> +void __init alternative_instructions(void)
> +{
> +    nmi_callback_t saved_nmi_callback;
> +
> +    arch_init_ideal_nops();
> +
> +    /*
> +     * The patching is not fully atomic, so try to avoid local interruptions
> +     * that might execute the to be patched code.
> +     * Other CPUs are not running.
> +     */
> +    saved_nmi_callback = set_nmi_callback(mask_nmi_callback);

Newline here

> +    /*
> +     * Don't stop machine check exceptions while patching.
> +     * MCEs only happen when something got corrupted and in this
> +     * case we must do something about the corruption.
> +     * Ignoring it is worse than a unlikely patching race.
> +     * Also machine checks tend to be broadcast and if one CPU
> +     * goes into machine check the others follow quickly, so we don't
> +     * expect a machine check to cause undue problems during to code
> +     * patching.
> +     */
> +

but not here.

> +    apply_alternatives(__alt_instructions, __alt_instructions_end);

Possibly also here.

> +    set_nmi_callback(saved_nmi_callback);
> +}
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 508649d..d16453a 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -48,6 +48,7 @@
>   #include <asm/setup.h>
>   #include <xen/cpu.h>
>   #include <asm/nmi.h>
> +#include <asm/alternative.h>
>   
>   /* opt_nosmp: If true, secondary processors are ignored. */
>   static bool_t __initdata opt_nosmp;
> @@ -1288,6 +1289,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>       if ( cpu_has_fsgsbase )
>           set_in_cr4(X86_CR4_FSGSBASE);
>   
> +    alternative_instructions();
> +

Given this ordering, it might be cleaner to have an 
ASSERT(!local_irq_enabled()) in the top of alternative_instructions(), 
and forgo the local_irq_save/restore() in text_poke_early().

If you can move this higher up before enabling MCEs in CR4, it might be 
slightly more resilient.

~Andrew

>       local_irq_enable();
>   
>       pt_pci_init();
> diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
> index 17db361..d4b1f1a 100644
> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -105,6 +105,12 @@ SECTIONS
>     .init.text : {
>          _sinittext = .;
>          *(.init.text)
> +       /*
> +        * Here are the replacement instructions. The linker sticks them
> +        * as binary blobs. The .altinstructions has enough data to get
> +        * the address and the length of them to patch the kernel safely.
> +        */
> +       *(.altinstr_replacement)
>          _einittext = .;
>     } :text
>     .init.data : {
> @@ -120,6 +126,15 @@ SECTIONS
>          __trampoline_seg_start = .;
>          *(.trampoline_seg)
>          __trampoline_seg_stop = .;
> +       /*
> +        * struct alt_inst entries. From the header (alternative.h):
> +        * "Alternative instructions for different CPU types or capabilities"
> +        * Think locking instructions on spinlocks.
> +        */
> +       . = ALIGN(8);
> +        __alt_instructions = .;
> +        *(.altinstructions)
> +        __alt_instructions_end = .;
>   
>          . = ALIGN(8);
>          __ctors_start = .;
> diff --git a/xen/include/asm-x86/alternative.h b/xen/include/asm-x86/alternative.h
> new file mode 100644
> index 0000000..55a6604
> --- /dev/null
> +++ b/xen/include/asm-x86/alternative.h
> @@ -0,0 +1,78 @@
> +#ifndef __X86_ALTERNATIVE_H__
> +#define __X86_ALTERNATIVE_H__
> +
> +#include <asm/nops.h>
> +
> +#ifdef __ASSEMBLY__
> +.macro altinstruction_entry orig alt feature orig_len alt_len
> +        .long \orig - .
> +        .long \alt - .
> +        .word \feature
> +        .byte \orig_len
> +        .byte \alt_len
> +.endm
> +#else
> +#include <xen/types.h>
> +
> +struct alt_instr {
> +    s32 instr_offset;       /* original instruction */
> +    s32 repl_offset;        /* offset to replacement instruction */
> +    u16 cpuid;              /* cpuid bit set for replacement */
> +    u8  instrlen;           /* length of original instruction */
> +    u8  replacementlen;     /* length of new instruction, <= instrlen */
> +};
> +
> +extern void alternative_instructions(void);
> +
> +#define OLDINSTR(oldinstr)      "661:\n\t" oldinstr "\n662:\n"
> +
> +#define b_replacement(number)   "663"#number
> +#define e_replacement(number)   "664"#number
> +
> +#define alt_slen "662b-661b"
> +#define alt_rlen(number) e_replacement(number)"f-"b_replacement(number)"f"
> +
> +#define ALTINSTR_ENTRY(feature, number)                                       \
> +        " .long 661b - .\n"                             /* label           */ \
> +        " .long " b_replacement(number)"f - .\n"        /* new instruction */ \
> +        " .word " STR(feature) "\n"                     /* feature bit     */ \
> +        " .byte " alt_slen "\n"                         /* source len      */ \
> +        " .byte " alt_rlen(number) "\n"                 /* replacement len */
> +
> +#define DISCARD_ENTRY(number)                           /* rlen <= slen */    \
> +        " .byte 0xff + (" alt_rlen(number) ") - (" alt_slen ")\n"
> +
> +#define ALTINSTR_REPLACEMENT(newinstr, feature, number) /* replacement */     \
> +        b_replacement(number)":\n\t" newinstr "\n" e_replacement(number) ":\n\t"
> +
> +/* alternative assembly primitive: */
> +#define ALTERNATIVE(oldinstr, newinstr, feature)                        \
> +        OLDINSTR(oldinstr)                                              \
> +        ".pushsection .altinstructions,\"a\"\n"                         \
> +        ALTINSTR_ENTRY(feature, 1)                                      \
> +        ".popsection\n"                                                 \
> +        ".pushsection .discard,\"aw\",@progbits\n"                      \
> +        DISCARD_ENTRY(1)                                                \
> +        ".popsection\n"                                                 \
> +        ".pushsection .altinstr_replacement, \"ax\"\n"                  \
> +        ALTINSTR_REPLACEMENT(newinstr, feature, 1)                      \
> +        ".popsection"
> +
> +/*
> + * Alternative instructions for different CPU types or capabilities.
> + *
> + * This allows to use optimized instructions even on generic binary
> + * kernels.
> + *
> + * length of oldinstr must be longer or equal the length of newinstr
> + * It can be padded with nops as needed.
> + *
> + * For non barrier like inlines please define new variants
> + * without volatile and memory clobber.
> + */
> +#define alternative(oldinstr, newinstr, feature)                        \
> +        asm volatile (ALTERNATIVE(oldinstr, newinstr, feature) : : : "memory")
> +
> +#endif  /*  __ASSEMBLY__  */
> +
> +#endif /* __X86_ALTERNATIVE_H__ */

Re: [PATCH v2 5/5] x86: Use alternative mechanism to define CLAC/STAC

[Andrew Cooper]

On 29/05/2014 06:34, Feng Wu wrote:
> This patch use alternative mechanism to define CLAC/STAC.
>
> Signed-off-by: Feng Wu <feng.wu@intel.com>

Reviewed-by: Andrew Cooper <andrew.cooper@citrix.com>

> ---
>   xen/include/asm-x86/asm_defns.h | 24 +++++++++++++-----------
>   1 file changed, 13 insertions(+), 11 deletions(-)
>
> diff --git a/xen/include/asm-x86/asm_defns.h b/xen/include/asm-x86/asm_defns.h
> index 0b9f898..dd50b97 100644
> --- a/xen/include/asm-x86/asm_defns.h
> +++ b/xen/include/asm-x86/asm_defns.h
> @@ -9,6 +9,7 @@
>   #include <asm/processor.h>
>   #include <asm/percpu.h>
>   #include <asm/cpufeature.h>
> +#include <asm/alternative.h>
>   
>   #ifndef __ASSEMBLY__
>   void ret_from_intr(void);
> @@ -165,26 +166,27 @@ void ret_from_intr(void);
>   #define __ASM_STAC      .byte 0x0f,0x01,0xcb
>   
>   #ifdef __ASSEMBLY__
> -#define ASM_AC(op)                                       \
> -        btl $X86_FEATURE_SMAP & 31,                      \
> -        CPUINFO_FEATURE_OFFSET(X86_FEATURE_SMAP)+boot_cpu_data(%rip); \
> -        jnc 881f;                                        \
> -        __ASM_##op;                                      \
> -881:
> -
> +#define ASM_AC(op)                                                     \
> +        661: ASM_NOP3;                                                 \
> +        .pushsection .altinstr_replacement, "ax";                      \
> +        662: __ASM_##op;                                               \
> +        .popsection;                                                   \
> +        .pushsection .altinstructions, "a";                            \
> +        altinstruction_entry 661b, 662b, X86_FEATURE_SMAP, 3, 3;       \
> +        .popsection
>   #define ASM_STAC ASM_AC(STAC)
>   #define ASM_CLAC ASM_AC(CLAC)
>   #else
>   static inline void clac(void)
>   {
> -    if ( boot_cpu_has(X86_FEATURE_SMAP) )
> -        asm volatile (STR(__ASM_CLAC) : : : "memory");
> +    /* Note: a barrier is implicit in alternative() */
> +    alternative(ASM_NOP3, STR(__ASM_CLAC), X86_FEATURE_SMAP);
>   }
>   
>   static inline void stac(void)
>   {
> -    if ( boot_cpu_has(X86_FEATURE_SMAP) )
> -        asm volatile (STR(__ASM_STAC) : : : "memory");
> +    /* Note: a barrier is implicit in alternative() */
> +    alternative(ASM_NOP3, STR(__ASM_STAC), X86_FEATURE_SMAP);
>   }
>   #endif
>   

Re: [PATCH v2 4/5] x86: Port the basic alternative mechanism from Linux to Xen

[Wu, Feng]

> -----Original Message-----
> From: Andrew Cooper [mailto:amc96@hermes.cam.ac.uk] On Behalf Of
> Andrew Cooper
> Sent: Thursday, May 29, 2014 4:56 PM
> To: Wu, Feng; xen-devel@lists.xen.org
> Cc: keir.xen@gmail.com; JBeulich@suse.com; ian.campbell@citrix.com;
> stefano.stabellini@citrix.com; tim@xen.org
> Subject: Re: [PATCH v2 4/5] x86: Port the basic alternative mechanism from
> Linux to Xen
> 
> On 29/05/2014 06:34, Feng Wu wrote:
> > This patch ports the basic alternative mechanism from Linux to Xen.
> > With this mechanism, we can patch code based on the CPU features.
> >
> > Signed-off-by: Feng Wu <feng.wu@intel.com>
> > ---
> >   xen/arch/x86/Makefile             |   1 +
> >   xen/arch/x86/alternative.c        | 213
> ++++++++++++++++++++++++++++++++++++++
> >   xen/arch/x86/setup.c              |   3 +
> >   xen/arch/x86/xen.lds.S            |  15 +++
> >   xen/include/asm-x86/alternative.h |  78 ++++++++++++++
> >   5 files changed, 310 insertions(+)
> >   create mode 100644 xen/arch/x86/alternative.c
> >   create mode 100644 xen/include/asm-x86/alternative.h
> >
> > diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> > index d502bdf..3734884 100644
> > --- a/xen/arch/x86/Makefile
> > +++ b/xen/arch/x86/Makefile
> > @@ -58,6 +58,7 @@ obj-y += crash.o
> >   obj-y += tboot.o
> >   obj-y += hpet.o
> >   obj-y += xstate.o
> > +obj-y += alternative.o
> >
> >   obj-$(crash_debug) += gdbstub.o
> >
> > diff --git a/xen/arch/x86/alternative.c b/xen/arch/x86/alternative.c
> > new file mode 100644
> > index 0000000..3dbc811
> > --- /dev/null
> > +++ b/xen/arch/x86/alternative.c
> > @@ -0,0 +1,213 @@
> >
> +/**************************************************************
> ****************
> > + * alternative.c
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License as published by
> > + * the Free Software Foundation; either version 2 of the License, or
> > + * (at your option) any later version.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> > + * GNU General Public License for more details.
> > + *
> > + * You should have received a copy of the GNU General Public License
> > + * along with this program; if not, write to the Free Software
> > + * Foundation, 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301,
> USA.
> > + */
> > +
> > +#include <xen/types.h>
> > +#include <asm/processor.h>
> > +#include <asm/alternative.h>
> > +#include <xen/init.h>
> > +#include <asm/system.h>
> > +#include <asm/traps.h>
> > +#include <asm/nmi.h>
> > +
> > +#define MAX_PATCH_LEN (255-1)
> > +
> > +extern struct alt_instr __alt_instructions[], __alt_instructions_end[];
> > +
> > +#ifdef K8_NOP1
> > +static const unsigned char k8nops[] __initconst = {
> > +    K8_NOP1,
> > +    K8_NOP2,
> > +    K8_NOP3,
> > +    K8_NOP4,
> > +    K8_NOP5,
> > +    K8_NOP6,
> > +    K8_NOP7,
> > +    K8_NOP8
> > +};
> > +static const unsigned char * const k8_nops[ASM_NOP_MAX+1] __initconst =
> {
> > +    NULL,
> > +    k8nops,
> > +    k8nops + 1,
> > +    k8nops + 1 + 2,
> > +    k8nops + 1 + 2 + 3,
> > +    k8nops + 1 + 2 + 3 + 4,
> > +    k8nops + 1 + 2 + 3 + 4 + 5,
> > +    k8nops + 1 + 2 + 3 + 4 + 5 + 6,
> > +    k8nops + 1 + 2 + 3 + 4 + 5 + 6 + 7
> > +};
> > +#endif
> > +
> > +#ifdef P6_NOP1
> > +static const unsigned char p6nops[] __initconst = {
> > +    P6_NOP1,
> > +    P6_NOP2,
> > +    P6_NOP3,
> > +    P6_NOP4,
> > +    P6_NOP5,
> > +    P6_NOP6,
> > +    P6_NOP7,
> > +    P6_NOP8
> > +};
> > +static const unsigned char * const p6_nops[ASM_NOP_MAX+1] __initconst
> = {
> > +    NULL,
> > +    p6nops,
> > +    p6nops + 1,
> > +    p6nops + 1 + 2,
> > +    p6nops + 1 + 2 + 3,
> > +    p6nops + 1 + 2 + 3 + 4,
> > +    p6nops + 1 + 2 + 3 + 4 + 5,
> > +    p6nops + 1 + 2 + 3 + 4 + 5 + 6,
> > +    p6nops + 1 + 2 + 3 + 4 + 5 + 6 + 7
> > +};
> > +#endif
> > +
> > +static const unsigned char * const *ideal_nops __initdata = k8_nops;
> > +
> > +static int __init mask_nmi_callback(struct cpu_user_regs *regs, int cpu)
> > +{
> > +    return 1;
> > +}
> > +
> > +static void __init arch_init_ideal_nops(void)
> > +{
> > +    /*
> > +     * Due to a decoder implementation quirk, some
> > +     * specific Intel CPUs actually perform better with
> > +     * the "k8_nops" than with the SDM-recommended NOPs.
> > +     */
> > +    if ( (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL) &&
> > +         !(boot_cpu_data.x86 == 6 &&
> > +           boot_cpu_data.x86_model >= 0x0f &&
> > +           boot_cpu_data.x86_model != 0x1c &&
> > +           boot_cpu_data.x86_model != 0x26 &&
> > +           boot_cpu_data.x86_model != 0x27 &&
> > +           boot_cpu_data.x86_model < 0x30) )
> > +        ideal_nops = p6_nops;
> > +}
> > +
> > +/* Use this to add nops to a buffer, then text_poke the whole buffer. */
> > +static void __init add_nops(void *insns, unsigned int len)
> > +{
> > +    while ( len > 0 )
> > +    {
> > +        unsigned int noplen = len;
> > +        if ( noplen > ASM_NOP_MAX )
> > +            noplen = ASM_NOP_MAX;
> > +        memcpy(insns, ideal_nops[noplen], noplen);
> > +        insns += noplen;
> > +        len -= noplen;
> > +    }
> > +}
> > +
> > +/*
> > + * text_poke_early - Update instructions on a live kernel at boot time
> > + * @addr: address to modify
> > + * @opcode: source of the copy
> > + * @len: length to copy
> > + *
> > + * When you use this code to patch more than one byte of an instruction
> > + * you need to make sure that other CPUs cannot execute this code in
> parallel.
> > + * Also no thread must be currently preempted in the middle of these
> > + * instructions. And on the local CPU you need to be protected again NMI or
> MCE
> > + * handlers seeing an inconsistent instruction while you patch.
> > + */
> > +static void *__init text_poke_early(void *addr, const void *opcode, size_t
> len)
> > +{
> > +    unsigned long flags;
> > +
> > +    local_irq_save(flags);
> > +    memcpy(addr, opcode, len);
> > +    sync_core();
> > +    local_irq_restore(flags);
> > +
> > +    return addr;
> > +}
> > +
> > +/*
> > + * Replace instructions with better alternatives for this CPU type.
> > + * This runs before SMP is initialized to avoid SMP problems with
> > + * self modifying code. This implies that asymmetric systems where
> > + * APs have less capabilities than the boot processor are not handled.
> > + * Tough. Make sure you disable such features by hand.
> > + */
> > +
> 
> Excess newline
> 
> > +static void __init apply_alternatives(struct alt_instr *start, struct alt_instr
> *end)
> > +{
> > +    struct alt_instr *a;
> > +    u8 *instr, *replacement;
> > +    u8 insnbuf[MAX_PATCH_LEN];
> > +
> > +    printk(KERN_INFO "alt table %p -> %p\n", start, end);
> > +
> > +    /*
> > +     * The scan order should be from start to end. A later scanned
> > +     * alternative code can overwrite a previous scanned alternative code.
> > +     * Some kernel functions (e.g. memcpy, memset, etc) use this order to
> > +     * patch code.
> > +     *
> > +     * So be careful if you want to change the scan order to any other
> > +     * order.
> > +     */
> > +    for ( a = start; a < end; a++ )
> > +    {
> > +        instr = (u8 *)&a->instr_offset + a->instr_offset;
> > +        replacement = (u8 *)&a->repl_offset + a->repl_offset;
> > +        BUG_ON(a->replacementlen > a->instrlen);
> > +        BUG_ON(a->instrlen > sizeof(insnbuf));
> > +        BUG_ON(a->cpuid >= NCAPINTS * 32);
> > +        if ( !boot_cpu_has(a->cpuid) )
> > +            continue;
> > +
> > +        memcpy(insnbuf, replacement, a->replacementlen);
> > +
> > +        /* 0xe8/0xe9 is a relative jump; fix the offset. */
> > +        if ( (*insnbuf & 0xfe) == 0xe8 && a->replacementlen == 5 )
> > +            *(s32 *)(insnbuf + 1) += replacement - instr;
> > +
> > +        add_nops(insnbuf + a->replacementlen,
> > +                 a->instrlen - a->replacementlen);
> > +        text_poke_early(instr, insnbuf, a->instrlen);
> > +    }
> > +}
> > +
> > +void __init alternative_instructions(void)
> > +{
> > +    nmi_callback_t saved_nmi_callback;
> > +
> > +    arch_init_ideal_nops();
> > +
> > +    /*
> > +     * The patching is not fully atomic, so try to avoid local interruptions
> > +     * that might execute the to be patched code.
> > +     * Other CPUs are not running.
> > +     */
> > +    saved_nmi_callback = set_nmi_callback(mask_nmi_callback);
> 
> Newline here
> 
> > +    /*
> > +     * Don't stop machine check exceptions while patching.
> > +     * MCEs only happen when something got corrupted and in this
> > +     * case we must do something about the corruption.
> > +     * Ignoring it is worse than a unlikely patching race.
> > +     * Also machine checks tend to be broadcast and if one CPU
> > +     * goes into machine check the others follow quickly, so we don't
> > +     * expect a machine check to cause undue problems during to code
> > +     * patching.
> > +     */
> > +
> 
> but not here.
> 
> > +    apply_alternatives(__alt_instructions, __alt_instructions_end);
> 
> Possibly also here.
> 
> > +    set_nmi_callback(saved_nmi_callback);
> > +}
> > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> > index 508649d..d16453a 100644
> > --- a/xen/arch/x86/setup.c
> > +++ b/xen/arch/x86/setup.c
> > @@ -48,6 +48,7 @@
> >   #include <asm/setup.h>
> >   #include <xen/cpu.h>
> >   #include <asm/nmi.h>
> > +#include <asm/alternative.h>
> >
> >   /* opt_nosmp: If true, secondary processors are ignored. */
> >   static bool_t __initdata opt_nosmp;
> > @@ -1288,6 +1289,8 @@ void __init noreturn __start_xen(unsigned long
> mbi_p)
> >       if ( cpu_has_fsgsbase )
> >           set_in_cr4(X86_CR4_FSGSBASE);
> >
> > +    alternative_instructions();
> > +
> 
> Given this ordering, it might be cleaner to have an
> ASSERT(!local_irq_enabled()) in the top of alternative_instructions(),
> and forgo the local_irq_save/restore() in text_poke_early().
> 
> If you can move this higher up before enabling MCEs in CR4, it might be
> slightly more resilient.
> 
> ~Andrew

MCE bit in CR4 is set in identify_cpu() --> mcheck_init() --> set_in_cr4(X86_CR4_MCE), but
apply_alternatives() needs boot_cpu_data.x86_capability being ready, since it calls boot_cpu_has().
If we put alternative_instructions() before enabling MCEs in CR4, which place do you suggest? Thanks!

Thanks,
Feng

> 
> >       local_irq_enable();
> >
> >       pt_pci_init();
> > diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
> > index 17db361..d4b1f1a 100644
> > --- a/xen/arch/x86/xen.lds.S
> > +++ b/xen/arch/x86/xen.lds.S
> > @@ -105,6 +105,12 @@ SECTIONS
> >     .init.text : {
> >          _sinittext = .;
> >          *(.init.text)
> > +       /*
> > +        * Here are the replacement instructions. The linker sticks them
> > +        * as binary blobs. The .altinstructions has enough data to get
> > +        * the address and the length of them to patch the kernel safely.
> > +        */
> > +       *(.altinstr_replacement)
> >          _einittext = .;
> >     } :text
> >     .init.data : {
> > @@ -120,6 +126,15 @@ SECTIONS
> >          __trampoline_seg_start = .;
> >          *(.trampoline_seg)
> >          __trampoline_seg_stop = .;
> > +       /*
> > +        * struct alt_inst entries. From the header (alternative.h):
> > +        * "Alternative instructions for different CPU types or capabilities"
> > +        * Think locking instructions on spinlocks.
> > +        */
> > +       . = ALIGN(8);
> > +        __alt_instructions = .;
> > +        *(.altinstructions)
> > +        __alt_instructions_end = .;
> >
> >          . = ALIGN(8);
> >          __ctors_start = .;
> > diff --git a/xen/include/asm-x86/alternative.h
> b/xen/include/asm-x86/alternative.h
> > new file mode 100644
> > index 0000000..55a6604
> > --- /dev/null
> > +++ b/xen/include/asm-x86/alternative.h
> > @@ -0,0 +1,78 @@
> > +#ifndef __X86_ALTERNATIVE_H__
> > +#define __X86_ALTERNATIVE_H__
> > +
> > +#include <asm/nops.h>
> > +
> > +#ifdef __ASSEMBLY__
> > +.macro altinstruction_entry orig alt feature orig_len alt_len
> > +        .long \orig - .
> > +        .long \alt - .
> > +        .word \feature
> > +        .byte \orig_len
> > +        .byte \alt_len
> > +.endm
> > +#else
> > +#include <xen/types.h>
> > +
> > +struct alt_instr {
> > +    s32 instr_offset;       /* original instruction */
> > +    s32 repl_offset;        /* offset to replacement instruction */
> > +    u16 cpuid;              /* cpuid bit set for replacement */
> > +    u8  instrlen;           /* length of original instruction */
> > +    u8  replacementlen;     /* length of new instruction, <= instrlen */
> > +};
> > +
> > +extern void alternative_instructions(void);
> > +
> > +#define OLDINSTR(oldinstr)      "661:\n\t" oldinstr "\n662:\n"
> > +
> > +#define b_replacement(number)   "663"#number
> > +#define e_replacement(number)   "664"#number
> > +
> > +#define alt_slen "662b-661b"
> > +#define alt_rlen(number)
> e_replacement(number)"f-"b_replacement(number)"f"
> > +
> > +#define ALTINSTR_ENTRY(feature, number)
> \
> > +        " .long 661b - .\n"                             /* label
> */ \
> > +        " .long " b_replacement(number)"f - .\n"        /* new
> instruction */ \
> > +        " .word " STR(feature) "\n"                     /* feature bit
> */ \
> > +        " .byte " alt_slen "\n"                         /* source len
> */ \
> > +        " .byte " alt_rlen(number) "\n"                 /*
> replacement len */
> > +
> > +#define DISCARD_ENTRY(number)                           /* rlen
> <= slen */    \
> > +        " .byte 0xff + (" alt_rlen(number) ") - (" alt_slen ")\n"
> > +
> > +#define ALTINSTR_REPLACEMENT(newinstr, feature, number) /*
> replacement */     \
> > +        b_replacement(number)":\n\t" newinstr "\n"
> e_replacement(number) ":\n\t"
> > +
> > +/* alternative assembly primitive: */
> > +#define ALTERNATIVE(oldinstr, newinstr, feature)
> \
> > +        OLDINSTR(oldinstr)
> \
> > +        ".pushsection .altinstructions,\"a\"\n"
> \
> > +        ALTINSTR_ENTRY(feature, 1)
> \
> > +        ".popsection\n"
> \
> > +        ".pushsection .discard,\"aw\",@progbits\n"
> \
> > +        DISCARD_ENTRY(1)
> \
> > +        ".popsection\n"
> \
> > +        ".pushsection .altinstr_replacement, \"ax\"\n"
> \
> > +        ALTINSTR_REPLACEMENT(newinstr, feature, 1)
> \
> > +        ".popsection"
> > +
> > +/*
> > + * Alternative instructions for different CPU types or capabilities.
> > + *
> > + * This allows to use optimized instructions even on generic binary
> > + * kernels.
> > + *
> > + * length of oldinstr must be longer or equal the length of newinstr
> > + * It can be padded with nops as needed.
> > + *
> > + * For non barrier like inlines please define new variants
> > + * without volatile and memory clobber.
> > + */
> > +#define alternative(oldinstr, newinstr, feature)
> \
> > +        asm volatile (ALTERNATIVE(oldinstr, newinstr, feature) : : :
> "memory")
> > +
> > +#endif  /*  __ASSEMBLY__  */
> > +
> > +#endif /* __X86_ALTERNATIVE_H__ */

Re: [PATCH v2 4/5] x86: Port the basic alternative mechanism from Linux to Xen

[Andrew Cooper]

On 29/05/2014 10:28, Wu, Feng wrote:
>
> 8 @@ void __init noreturn __start_xen(unsigned long
>> mbi_p)
>>>        if ( cpu_has_fsgsbase )
>>>            set_in_cr4(X86_CR4_FSGSBASE);
>>>
>>> +    alternative_instructions();
>>> +
>> Given this ordering, it might be cleaner to have an
>> ASSERT(!local_irq_enabled()) in the top of alternative_instructions(),
>> and forgo the local_irq_save/restore() in text_poke_early().
>>
>> If you can move this higher up before enabling MCEs in CR4, it might be
>> slightly more resilient.
>>
>> ~Andrew
> MCE bit in CR4 is set in identify_cpu() --> mcheck_init() --> set_in_cr4(X86_CR4_MCE), but
> apply_alternatives() needs boot_cpu_data.x86_capability being ready, since it calls boot_cpu_has().
> If we put alternative_instructions() before enabling MCEs in CR4, which place do you suggest? Thanks!
>
> Thanks,
> Feng

One option would be to temporarily disable it in cr4 at the same point 
that NMIs are nopped out, in the same way as temporarily disabling 
CR4.SMAP when building dom0.

At the end of the day, an MCE will certainly result in a crash, but at 
least it wouldn't be from a weird fault because some of the codepath in 
the MCE handler was midway through being patched.

~Andrew

Re: [PATCH v2 1/5] Use STR() as the only method for performing preprocessor stringificaion

[Jan Beulich]

>>> Feng Wu <feng.wu@intel.com> 05/29/14 7:36 AM >>>
>There are two ways of performing preprocessor stringificaion in the
>current code: __stringify() from stringify.h, and STR() from config.h.
>This patch consolidates down to one.

But backwards. You didn't ask which way to do the consolidation, and I suppose
you also didn't check which one got added later. We should retain the (Linux
conforming) __stringify() and drop STR(), if we do any such cleanup change at all.

Jan

Re: [PATCH v2 4/5] x86: Port the basic alternative mechanism from Linux to Xen

[Wu, Feng]

> -----Original Message-----
> From: Andrew Cooper [mailto:amc96@hermes.cam.ac.uk] On Behalf Of
> Andrew Cooper
> Sent: Thursday, May 29, 2014 10:59 PM
> To: Wu, Feng; xen-devel@lists.xen.org
> Cc: keir.xen@gmail.com; JBeulich@suse.com; ian.campbell@citrix.com;
> stefano.stabellini@citrix.com; tim@xen.org
> Subject: Re: [PATCH v2 4/5] x86: Port the basic alternative mechanism from
> Linux to Xen
> 
> On 29/05/2014 10:28, Wu, Feng wrote:
> >
> > 8 @@ void __init noreturn __start_xen(unsigned long
> >> mbi_p)
> >>>        if ( cpu_has_fsgsbase )
> >>>            set_in_cr4(X86_CR4_FSGSBASE);
> >>>
> >>> +    alternative_instructions();
> >>> +
> >> Given this ordering, it might be cleaner to have an
> >> ASSERT(!local_irq_enabled()) in the top of alternative_instructions(),
> >> and forgo the local_irq_save/restore() in text_poke_early().
> >>
> >> If you can move this higher up before enabling MCEs in CR4, it might be
> >> slightly more resilient.
> >>
> >> ~Andrew
> > MCE bit in CR4 is set in identify_cpu() --> mcheck_init() -->
> set_in_cr4(X86_CR4_MCE), but
> > apply_alternatives() needs boot_cpu_data.x86_capability being ready, since it
> calls boot_cpu_has().
> > If we put alternative_instructions() before enabling MCEs in CR4, which place
> do you suggest? Thanks!
> >
> > Thanks,
> > Feng
> 
> One option would be to temporarily disable it in cr4 at the same point
> that NMIs are nopped out, in the same way as temporarily disabling
> CR4.SMAP when building dom0.
> 
> At the end of the day, an MCE will certainly result in a crash, but at
> least it wouldn't be from a weird fault because some of the codepath in
> the MCE handler was midway through being patched.
> 
> ~Andrew

>From the original comments below, which is from Linux:

        /*
         * Don't stop machine check exceptions while patching.
         * MCEs only happen when something got corrupted and in this
         * case we must do something about the corruption.
         * Ignoring it is worse than a unlikely patching race.
         * Also machine checks tend to be broadcast and if one CPU
         * goes into machine check the others follow quickly, so we don't
         * expect a machine check to cause undue problems during to code
         * patching.
         */

We can see that the MCE remains enabled here on purpose. It says "Ignoring the MCE is worse than a _unlikely_ patching race."
Do you think we still need to disable MCE, or should we follow the current solution in Linux?

Thanks,
Feng

Re: [PATCH v2 4/5] x86: Port the basic alternative mechanism from Linux to Xen

[Jan Beulich]

>>> "Wu, Feng" <feng.wu@intel.com> 05/30/14 6:48 AM >>>
> From: Andrew Cooper [mailto:amc96@hermes.cam.ac.uk] On Behalf Of
> Andrew Cooper
>> One option would be to temporarily disable it in cr4 at the same point
>> that NMIs are nopped out, in the same way as temporarily disabling
>> CR4.SMAP when building dom0.
>> 
>> At the end of the day, an MCE will certainly result in a crash, but at
>> least it wouldn't be from a weird fault because some of the codepath in
>> the MCE handler was midway through being patched.
>
>From the original comments below, which is from Linux:
>
        >/*
         >* Don't stop machine check exceptions while patching.
         >* MCEs only happen when something got corrupted and in this
         >* case we must do something about the corruption.
         >* Ignoring it is worse than a unlikely patching race.
         >* Also machine checks tend to be broadcast and if one CPU
         >* goes into machine check the others follow quickly, so we don't
         >* expect a machine check to cause undue problems during to code
         >* patching.
         >*/
>
>We can see that the MCE remains enabled here on purpose. It says
> "Ignoring the MCE is worse than a _unlikely_ patching race."
>Do you think we still need to disable MCE, or should we follow the
> current solution in Linux?

FWIW I can see downsides to both approaches, and hence would tend
towards following the original (Linux) route here despite the broadcasting
aspect mentioned above being IIRC an Intel specific, i.e. more of a
problem on AMD systems.

Jan