[PATCH v4 0/4] xen/x86: add testing for self modifying code and livepatch

[PATCH v4 0/4] xen/x86: add testing for self modifying code and livepatch

[Roger Pau Monne]

Hello,

The following series contains a misc set of fixes and improvements.

There's one improvement for the hypervisor to set function alignment for
livepatch builds in order to make sure there's always enough space in a
function to be live-patched.

Following patches attempt to introduce a set of tests for self modifying
code, currently one test using the alternatives framework, and one test
for livepatch.

Last patch hooks the newly introduced livepatch test into the gitlab CI
using QEMU and an Alpine Linux dom0:

https://gitlab.com/xen-project/people/royger/xen/-/pipelines/1108069448

Roger Pau Monne (4):
  x86/livepatch: align functions to ensure minimal distance between
    entry points
  xen/x86: introduce self modifying code test
  x86/livepatch: introduce a basic live patch test to gitlab CI
  automation: add x86-64 livepatching test

 automation/gitlab-ci/build.yaml               |  9 +++
 automation/gitlab-ci/test.yaml                |  8 +++
 automation/scripts/build-livepatch            | 27 +++++++
 .../scripts/qemu-alpine-x86_64-livepatch.sh   | 68 ++++++++++++++++++
 tools/include/xenctrl.h                       |  2 +
 tools/libs/ctrl/xc_misc.c                     | 14 ++++
 tools/misc/xen-livepatch.c                    | 29 ++++++++
 xen/Kconfig                                   | 19 +++++
 xen/Makefile                                  |  3 +
 xen/arch/arm/livepatch.c                      |  2 +
 xen/arch/arm/xen.lds.S                        |  4 ++
 xen/arch/x86/Kconfig                          |  1 +
 xen/arch/x86/Makefile                         |  1 +
 xen/arch/x86/include/asm/test.h               | 37 ++++++++++
 xen/arch/x86/livepatch.c                      |  4 ++
 xen/arch/x86/setup.c                          |  3 +
 xen/arch/x86/sysctl.c                         |  9 +++
 xen/arch/x86/test/Makefile                    |  3 +
 xen/arch/x86/test/smoc-lp-alt.c               | 24 +++++++
 xen/arch/x86/test/smoc-lp.c                   | 24 +++++++
 xen/arch/x86/test/smoc.c                      | 70 +++++++++++++++++++
 xen/arch/x86/xen.lds.S                        |  4 ++
 xen/common/Kconfig                            |  5 +-
 xen/common/kernel.c                           |  5 +-
 xen/include/public/sysctl.h                   | 13 ++++
 xen/include/xen/lib.h                         |  1 +
 26 files changed, 386 insertions(+), 3 deletions(-)
 create mode 100755 automation/scripts/build-livepatch
 create mode 100755 automation/scripts/qemu-alpine-x86_64-livepatch.sh
 create mode 100644 xen/arch/x86/include/asm/test.h
 create mode 100644 xen/arch/x86/test/Makefile
 create mode 100644 xen/arch/x86/test/smoc-lp-alt.c
 create mode 100644 xen/arch/x86/test/smoc-lp.c
 create mode 100644 xen/arch/x86/test/smoc.c

-- 
2.43.0

[PATCH v4 1/4] x86/livepatch: align functions to ensure minimal distance between entry points

[Roger Pau Monne]

The minimal function size requirements for livepatch are either 5 bytes (for
jmp) or 9 bytes (for endbr + jmp) on x86, and always 4 bytes on Arm.  Ensure
that distance between functions entry points is always at least of the minimal
required size for livepatch instruction replacement to be successful.

Add an additional align directive to the linker script, in order to ensure that
the next section placed after the .text.* (per-function sections) is also
aligned to the required boundary, so that the distance of the last function
entry point with the next symbol is also of minimal size.

Note that it's possible for the compiler to end up using a higher function
alignment regardless of the passed value, so this change just make sure that
the minimum required for livepatch to work is present.  Different compilers
handle the option differently, as clang will ignore -falign-functions value
if it's smaller than the one that would be set by the optimization level, while
gcc seems to always honor the function alignment passed in -falign-functions.
In order to cope with this behavior and avoid that setting -falign-functions
results in an alignment inferior to what the optimization level would have
selected force x86 release builds to use a function alignment of 16 bytes.

The compiler option -falign-functions is not available on at least clang 3.8,
so introduce a Kconfig check for it and make the livepatch option depend on the
compiler supporting the option.

The naming of the option(s) CONFIG_FUNCTION_ALIGNMENT is explicitly not
mentioning CC in preparation for the option also being used by assembly code.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v3:
 - Test for compiler option with -falign-functions.
 - Make FUNCTION_ALIGNMENT depend on CC_HAS_FUNCTION_ALIGNMENT.
 - Set 16byte function alignment for x86 release builds.

Changes since v2:
 - Add Arm side.
 - Align end of section in the linker script to ensure enough padding for the
   last function.
 - Expand commit message and subject.
 - Rework Kconfig options.
 - Check that the compiler supports the option.

Changes since v1:
 - New in this version.
---
 xen/Kconfig              | 19 +++++++++++++++++++
 xen/Makefile             |  3 +++
 xen/arch/arm/livepatch.c |  2 ++
 xen/arch/arm/xen.lds.S   |  4 ++++
 xen/arch/x86/Kconfig     |  1 +
 xen/arch/x86/livepatch.c |  4 ++++
 xen/arch/x86/xen.lds.S   |  4 ++++
 xen/common/Kconfig       |  5 ++++-
 8 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/xen/Kconfig b/xen/Kconfig
index 134e6e68ad84..c2cc3fe165eb 100644
--- a/xen/Kconfig
+++ b/xen/Kconfig
@@ -37,6 +37,25 @@ config CC_HAS_VISIBILITY_ATTRIBUTE
 config CC_SPLIT_SECTIONS
 	bool
 
+# Set function alignment.
+#
+# Allow setting on a boolean basis, and then convert such selection to an
+# integer for the build system and code to consume more easily.
+config CC_HAS_FUNCTION_ALIGNMENT
+	def_bool $(cc-option,-falign-functions)
+config FUNCTION_ALIGNMENT_4B
+	bool
+config FUNCTION_ALIGNMENT_8B
+	bool
+config FUNCTION_ALIGNMENT_16B
+	bool
+config FUNCTION_ALIGNMENT
+	int
+	depends on CC_HAS_FUNCTION_ALIGNMENT
+	default 16 if FUNCTION_ALIGNMENT_16B
+	default  8 if  FUNCTION_ALIGNMENT_8B
+	default  4 if  FUNCTION_ALIGNMENT_4B
+
 source "arch/$(SRCARCH)/Kconfig"
 
 config DEFCONFIG_LIST
diff --git a/xen/Makefile b/xen/Makefile
index 21832d640225..162cb2bda1c5 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -390,6 +390,9 @@ CFLAGS += -fomit-frame-pointer
 endif
 
 CFLAGS-$(CONFIG_CC_SPLIT_SECTIONS) += -ffunction-sections -fdata-sections
+ifdef CONFIG_FUNCTION_ALIGNMENT
+CFLAGS += -falign-functions=$(CONFIG_FUNCTION_ALIGNMENT)
+endif
 
 CFLAGS += -nostdinc -fno-builtin -fno-common
 CFLAGS += -Werror -Wredundant-decls -Wwrite-strings -Wno-pointer-arith
diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c
index bbca1e5a5ed3..aa8ae8c38d28 100644
--- a/xen/arch/arm/livepatch.c
+++ b/xen/arch/arm/livepatch.c
@@ -68,6 +68,8 @@ void arch_livepatch_revive(void)
 
 int arch_livepatch_verify_func(const struct livepatch_func *func)
 {
+    BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE > CONFIG_FUNCTION_ALIGNMENT);
+
     /* If NOPing only do up to maximum amount we can put in the ->opaque. */
     if ( !func->new_addr && (func->new_size > LIVEPATCH_OPAQUE_SIZE ||
          func->new_size % ARCH_PATCH_INSN_SIZE) )
diff --git a/xen/arch/arm/xen.lds.S b/xen/arch/arm/xen.lds.S
index 59b80d122fd0..afaf1e996b0e 100644
--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -44,6 +44,10 @@ SECTIONS
 #ifdef CONFIG_CC_SPLIT_SECTIONS
        *(.text.*)
 #endif
+#ifdef CONFIG_FUNCTION_ALIGNMENT
+       /* Ensure enough distance with the next placed section. */
+       . = ALIGN(CONFIG_FUNCTION_ALIGNMENT);
+#endif
 
        *(.fixup)
        *(.gnu.warning)
diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 1acdffc51c22..0cd741be5b6f 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -29,6 +29,7 @@ config X86
 	select HAS_UBSAN
 	select HAS_VPCI if HVM
 	select NEEDS_LIBELF
+	select FUNCTION_ALIGNMENT_16B if !DEBUG
 
 config ARCH_DEFCONFIG
 	string
diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
index ee539f001b73..b00ad7120da9 100644
--- a/xen/arch/x86/livepatch.c
+++ b/xen/arch/x86/livepatch.c
@@ -109,6 +109,10 @@ int arch_livepatch_verify_func(const struct livepatch_func *func)
          */
         uint8_t needed = ARCH_PATCH_INSN_SIZE;
 
+        BUILD_BUG_ON(ARCH_PATCH_INSN_SIZE +
+                     (IS_ENABLED(CONIFG_XEN_IBT) ? ENDBR64_LEN : 0) >
+                     CONFIG_FUNCTION_ALIGNMENT);
+
         if ( is_endbr64(func->old_addr) || is_endbr64_poison(func->old_addr) )
             needed += ENDBR64_LEN;
 
diff --git a/xen/arch/x86/xen.lds.S b/xen/arch/x86/xen.lds.S
index 8930e14fc40e..5b3332300d44 100644
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -99,6 +99,10 @@ SECTIONS
        *(.text)
 #ifdef CONFIG_CC_SPLIT_SECTIONS
        *(.text.*)
+#endif
+#ifdef CONFIG_FUNCTION_ALIGNMENT
+       /* Ensure enough distance with the next placed section. */
+       . = ALIGN(CONFIG_FUNCTION_ALIGNMENT);
 #endif
        *(.text.__x86_indirect_thunk_*)
 
diff --git a/xen/common/Kconfig b/xen/common/Kconfig
index 310ad4229cdf..c9a21c3c8a07 100644
--- a/xen/common/Kconfig
+++ b/xen/common/Kconfig
@@ -395,8 +395,11 @@ config CRYPTO
 config LIVEPATCH
 	bool "Live patching support"
 	default X86
-	depends on "$(XEN_HAS_BUILD_ID)" = "y"
+	depends on "$(XEN_HAS_BUILD_ID)" = "y" && CC_HAS_FUNCTION_ALIGNMENT
 	select CC_SPLIT_SECTIONS
+	select FUNCTION_ALIGNMENT_16B if XEN_IBT
+	select FUNCTION_ALIGNMENT_8B  if X86
+	select FUNCTION_ALIGNMENT_4B
 	---help---
 	  Allows a running Xen hypervisor to be dynamically patched using
 	  binary patches without rebooting. This is primarily used to binarily
-- 
2.43.0

[PATCH v4 2/4] xen/x86: introduce self modifying code test

[Roger Pau Monne]

Introduce a helper to perform checks related to self modifying code, and start
by creating a simple test to check that alternatives have been applied.

Such test is hooked into the boot process and called just after alternatives
have been applied.  In case of failure a message is printed, and the hypervisor
is tainted as not having passed the tests, this does require introducing a new
taint bit (printed as 'T').

A new sysctl is also introduced to run the tests on demand.  While there are no
current users introduced here, further changes will introduce those, and it's
helpful to have the interface defined in the sysctl header from the start.

Note the sysctl visibility is not limited to x86, albeit the only
implementation is for x86.  It's expected that other architectures can reuse
the same sysctl and structure, with possibly different tests.  Leave adjusting
those to when support for a different architecture is introduced, as the
sysctl interface is not stable anyway.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v3:
 - Rename taint variable.
 - Introduce a wrapper to run all selftests.
 - Only print messages and taint the hypervisor when tests are executed on
   boot.

Changes since v2:
 - Rename to smoc and place in test/smoc*
 - fix inline assembly.

Changes since v1:
 - Rework test and interface.
---
 tools/include/xenctrl.h         |  2 +
 tools/libs/ctrl/xc_misc.c       | 14 +++++++
 xen/arch/x86/Makefile           |  1 +
 xen/arch/x86/include/asm/test.h | 30 +++++++++++++++
 xen/arch/x86/setup.c            |  3 ++
 xen/arch/x86/sysctl.c           |  9 +++++
 xen/arch/x86/test/Makefile      |  1 +
 xen/arch/x86/test/smoc.c        | 66 +++++++++++++++++++++++++++++++++
 xen/common/kernel.c             |  5 ++-
 xen/include/public/sysctl.h     |  9 +++++
 xen/include/xen/lib.h           |  1 +
 11 files changed, 139 insertions(+), 2 deletions(-)
 create mode 100644 xen/arch/x86/include/asm/test.h
 create mode 100644 xen/arch/x86/test/Makefile
 create mode 100644 xen/arch/x86/test/smoc.c

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 2ef8b4e05422..0af796ae84e8 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -2658,6 +2658,8 @@ int xc_dt_overlay(xc_interface *xch, void *overlay_fdt,
                   uint32_t overlay_fdt_size, uint8_t overlay_op);
 #endif
 
+int xc_test_smoc(xc_interface *xch, uint32_t tests, uint32_t *result);
+
 /* Compat shims */
 #include "xenctrl_compat.h"
 
diff --git a/tools/libs/ctrl/xc_misc.c b/tools/libs/ctrl/xc_misc.c
index 5ecdfa2c7934..1d3d5929cf96 100644
--- a/tools/libs/ctrl/xc_misc.c
+++ b/tools/libs/ctrl/xc_misc.c
@@ -1021,6 +1021,20 @@ int xc_livepatch_replace(xc_interface *xch, char *name, uint32_t timeout, uint32
     return _xc_livepatch_action(xch, name, LIVEPATCH_ACTION_REPLACE, timeout, flags);
 }
 
+int xc_test_smoc(xc_interface *xch, uint32_t tests, uint32_t *result)
+{
+    struct xen_sysctl sysctl = {
+        .cmd = XEN_SYSCTL_test_smoc,
+        .u.test_smoc.tests = tests,
+    };
+    int rc = do_sysctl(xch, &sysctl);
+
+    if ( !rc )
+        *result = sysctl.u.test_smoc.results;
+
+    return rc;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index f3abdf9cd111..ad5112b03c64 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_HVM) += hvm/
 obj-y += mm/
 obj-$(CONFIG_XENOPROF) += oprofile/
 obj-$(CONFIG_PV) += pv/
+obj-y += test/
 obj-y += x86_64/
 obj-y += x86_emulate/
 
diff --git a/xen/arch/x86/include/asm/test.h b/xen/arch/x86/include/asm/test.h
new file mode 100644
index 000000000000..e96e709c6a52
--- /dev/null
+++ b/xen/arch/x86/include/asm/test.h
@@ -0,0 +1,30 @@
+#ifndef _ASM_X86_TEST_H_
+#define _ASM_X86_TEST_H_
+
+#include <xen/types.h>
+
+int test_smoc(uint32_t selection, uint32_t *results);
+
+static inline void execute_selftests(void)
+{
+    const uint32_t exec_mask = XEN_SYSCTL_TEST_SMOC_ALL;
+    uint32_t result;
+    int rc;
+
+    printk(XENLOG_INFO "Checking Self Modify Code\n");
+    rc = test_smoc(exec_mask, &result);
+    if ( rc || (result & exec_mask) != exec_mask )
+        add_taint(TAINT_ERROR_SELFTEST);
+}
+
+#endif	/* _ASM_X86_TEST_H_ */
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 3cba2be0af6c..e974a8f8d725 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -58,6 +58,7 @@
 #include <asm/microcode.h>
 #include <asm/prot-key.h>
 #include <asm/pv/domain.h>
+#include <asm/test.h>
 
 /* opt_nosmp: If true, secondary processors are ignored. */
 static bool __initdata opt_nosmp;
@@ -1951,6 +1952,8 @@ void asmlinkage __init noreturn __start_xen(unsigned long mbi_p)
 
     alternative_branches();
 
+    execute_selftests();
+
     /*
      * NB: when running as a PV shim VCPUOP_up/down is wired to the shim
      * physical cpu_add/remove functions, so launch the guest with only
diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c
index 1d40d82c5ad2..a61a99f8696a 100644
--- a/xen/arch/x86/sysctl.c
+++ b/xen/arch/x86/sysctl.c
@@ -25,6 +25,7 @@
 #include <asm/processor.h>
 #include <asm/setup.h>
 #include <asm/smp.h>
+#include <asm/test.h>
 #include <asm/numa.h>
 #include <xen/nodemask.h>
 #include <xen/cpu.h>
@@ -423,6 +424,14 @@ long arch_do_sysctl(
         break;
     }
 
+    case XEN_SYSCTL_test_smoc:
+        ret = test_smoc(sysctl->u.test_smoc.tests,
+                        &sysctl->u.test_smoc.results);
+        if ( !ret && __copy_field_to_guest(u_sysctl, sysctl,
+                                           u.test_smoc.results) )
+            ret = -EFAULT;
+        break;
+
     default:
         ret = -ENOSYS;
         break;
diff --git a/xen/arch/x86/test/Makefile b/xen/arch/x86/test/Makefile
new file mode 100644
index 000000000000..b504b8196659
--- /dev/null
+++ b/xen/arch/x86/test/Makefile
@@ -0,0 +1 @@
+obj-y += smoc.o
diff --git a/xen/arch/x86/test/smoc.c b/xen/arch/x86/test/smoc.c
new file mode 100644
index 000000000000..09db5cee9ae2
--- /dev/null
+++ b/xen/arch/x86/test/smoc.c
@@ -0,0 +1,66 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#include <xen/errno.h>
+
+#include <asm/alternative.h>
+#include <asm/cpufeature.h>
+#include <asm/test.h>
+
+static bool cf_check test_insn_replacement(void)
+{
+#define EXPECTED_VALUE 2
+    unsigned int r = ~EXPECTED_VALUE;
+
+    alternative_io("", "mov %1, %0", X86_FEATURE_ALWAYS,
+                   "+r" (r), "i" (EXPECTED_VALUE));
+
+    return r == EXPECTED_VALUE;
+#undef EXPECTED_VALUE
+}
+
+int test_smoc(uint32_t selection, uint32_t *results)
+{
+    struct {
+        unsigned int mask;
+        bool (*test)(void);
+        const char *name;
+    } static const tests[] = {
+        { XEN_SYSCTL_TEST_SMOC_INSN_REPL, &test_insn_replacement,
+          "alternative instruction replacement" },
+    };
+    unsigned int i;
+
+    if ( selection & ~XEN_SYSCTL_TEST_SMOC_ALL )
+        return -EINVAL;
+
+    if ( results )
+        *results = 0;
+
+    for ( i = 0; i < ARRAY_SIZE(tests); i++ )
+    {
+        if ( !(selection & tests[i].mask) )
+            continue;
+
+        if ( tests[i].test() )
+        {
+            if ( results )
+                *results |= tests[i].mask;
+            continue;
+        }
+
+        if ( system_state < SYS_STATE_active )
+            printk(XENLOG_ERR "%s test failed\n", tests[i].name);
+    }
+
+    return 0;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/common/kernel.c b/xen/common/kernel.c
index 08dbaa2a054c..77f93f137cac 100644
--- a/xen/common/kernel.c
+++ b/xen/common/kernel.c
@@ -386,13 +386,14 @@ char *print_tainted(char *str)
 {
     if ( tainted )
     {
-        snprintf(str, TAINT_STRING_MAX_LEN, "Tainted: %c%c%c%c%c%c",
+        snprintf(str, TAINT_STRING_MAX_LEN, "Tainted: %c%c%c%c%c%c%c",
                  tainted & TAINT_MACHINE_INSECURE ? 'I' : ' ',
                  tainted & TAINT_MACHINE_CHECK ? 'M' : ' ',
                  tainted & TAINT_SYNC_CONSOLE ? 'C' : ' ',
                  tainted & TAINT_ERROR_INJECT ? 'E' : ' ',
                  tainted & TAINT_HVM_FEP ? 'H' : ' ',
-                 tainted & TAINT_CPU_OUT_OF_SPEC ? 'S' : ' ');
+                 tainted & TAINT_CPU_OUT_OF_SPEC ? 'S' : ' ',
+                 tainted & TAINT_ERROR_SELFTEST ? 'T' : ' ');
     }
     else
     {
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 9b19679caeb1..d015a490da38 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -1201,6 +1201,13 @@ struct xen_sysctl_dt_overlay {
 };
 #endif
 
+struct xen_sysctl_test_smoc {
+    uint32_t tests;     /* IN: bitmap with selected tests to execute. */
+#define XEN_SYSCTL_TEST_SMOC_INSN_REPL   (1U << 0)
+#define XEN_SYSCTL_TEST_SMOC_ALL         (XEN_SYSCTL_TEST_SMOC_INSN_REPL)
+    uint32_t results;   /* OUT: test result: 1 -> success, 0 -> failure. */
+};
+
 struct xen_sysctl {
     uint32_t cmd;
 #define XEN_SYSCTL_readconsole                    1
@@ -1232,6 +1239,7 @@ struct xen_sysctl {
 /* #define XEN_SYSCTL_set_parameter              28 */
 #define XEN_SYSCTL_get_cpu_policy                29
 #define XEN_SYSCTL_dt_overlay                    30
+#define XEN_SYSCTL_test_smoc                     31
     uint32_t interface_version; /* XEN_SYSCTL_INTERFACE_VERSION */
     union {
         struct xen_sysctl_readconsole       readconsole;
@@ -1259,6 +1267,7 @@ struct xen_sysctl {
         struct xen_sysctl_cpu_levelling_caps cpu_levelling_caps;
         struct xen_sysctl_cpu_featureset    cpu_featureset;
         struct xen_sysctl_livepatch_op      livepatch;
+        struct xen_sysctl_test_smoc         test_smoc;
 #if defined(__i386__) || defined(__x86_64__)
         struct xen_sysctl_cpu_policy        cpu_policy;
 #endif
diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
index 1793be5b6b89..dcb42a5b64c9 100644
--- a/xen/include/xen/lib.h
+++ b/xen/include/xen/lib.h
@@ -167,6 +167,7 @@ uint64_t muldiv64(uint64_t a, uint32_t b, uint32_t c);
 #define TAINT_HVM_FEP                   (1u << 3)
 #define TAINT_MACHINE_INSECURE          (1u << 4)
 #define TAINT_CPU_OUT_OF_SPEC           (1u << 5)
+#define TAINT_ERROR_SELFTEST            (1U << 6)
 extern unsigned int tainted;
 #define TAINT_STRING_MAX_LEN            20
 extern char *print_tainted(char *str);
-- 
2.43.0

[PATCH v4 3/4] x86/livepatch: introduce a basic live patch test to gitlab CI

[Roger Pau Monne]

Introduce a basic livepatch test using the interface to run self modifying
tests.  The introduced test relies on changing a function from returning false
to returning true.

To simplify the burden of keeping a patch that can be provided to
livepatch-build-tools, introduce two new files: one containing the unpatched
test functions, and another one that contains the patched forms of such
functions.  Note that only the former is linked into the Xen image, the latter
is built but the object file is not consumed afterwards.  Do this to assert
that the file containing the patched functions continues to build.

Since livepatch testing will ensure that the functions are not patched previous
the applying the livepatch, allow the livepatch related tests to fail without
tainting the hypervisor.

Note the livepatch tests are not run as part of the self modifying checks
executed during boot, as they would obviously fail.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
Changes since v3:
 - Rebase over previous changes.

Changes since v2:
 - Clarify comment about xor vs mov instructions for return false/true
   encodings.

Changes since v1:
 - New interface & test.
---
 tools/misc/xen-livepatch.c      | 29 +++++++++++++++++++++++++++++
 xen/arch/x86/include/asm/test.h |  9 ++++++++-
 xen/arch/x86/test/Makefile      |  2 ++
 xen/arch/x86/test/smoc-lp-alt.c | 24 ++++++++++++++++++++++++
 xen/arch/x86/test/smoc-lp.c     | 24 ++++++++++++++++++++++++
 xen/arch/x86/test/smoc.c        |  4 ++++
 xen/include/public/sysctl.h     |  6 +++++-
 7 files changed, 96 insertions(+), 2 deletions(-)
 create mode 100644 xen/arch/x86/test/smoc-lp-alt.c
 create mode 100644 xen/arch/x86/test/smoc-lp.c

diff --git a/tools/misc/xen-livepatch.c b/tools/misc/xen-livepatch.c
index 5bf9d9a32b65..4ebd1b4e936d 100644
--- a/tools/misc/xen-livepatch.c
+++ b/tools/misc/xen-livepatch.c
@@ -37,6 +37,7 @@ void show_help(void)
             "  replace <name>         apply <name> patch and revert all others.\n"
             "  unload <name>          unload name <name> patch.\n"
             "  load <file> [flags]    upload and apply <file> with name as the <file> name\n"
+            "  test                   execute self modifying code livepatch hypervisor tests\n"
             "    Supported flags:\n"
             "      --nodeps           Disable inter-module buildid dependency check.\n"
             "                         Check only against hypervisor buildid.\n",
@@ -542,6 +543,33 @@ error:
     return rc;
 }
 
+static int test_func(int argc, char *argv[])
+{
+    uint32_t results = 0;
+    int rc;
+
+    if ( argc != 0 )
+    {
+        show_help();
+        return -1;
+    }
+
+    rc = xc_test_smoc(xch, XEN_SYSCTL_TEST_SMOC_LP, &results);
+    if ( rc )
+    {
+        fprintf(stderr, "test operation failed: %s\n", strerror(errno));
+        return -1;
+    }
+    if ( (results & XEN_SYSCTL_TEST_SMOC_LP) != XEN_SYSCTL_TEST_SMOC_LP )
+    {
+        fprintf(stderr, "some tests failed: %#x (expected %#x)\n",
+                results, XEN_SYSCTL_TEST_SMOC_LP);
+        return -1;
+    }
+
+    return 0;
+}
+
 /*
  * These are also functions in action_options that are called in case
  * none of the ones in main_options match.
@@ -554,6 +582,7 @@ struct {
     { "list", list_func },
     { "upload", upload_func },
     { "load", load_func },
+    { "test", test_func },
 };
 
 int main(int argc, char *argv[])
diff --git a/xen/arch/x86/include/asm/test.h b/xen/arch/x86/include/asm/test.h
index e96e709c6a52..951aaaa1f55e 100644
--- a/xen/arch/x86/include/asm/test.h
+++ b/xen/arch/x86/include/asm/test.h
@@ -3,11 +3,18 @@
 
 #include <xen/types.h>
 
+#include <public/sysctl.h>
+
 int test_smoc(uint32_t selection, uint32_t *results);
 
+#ifdef CONFIG_LIVEPATCH
+bool cf_check test_lp_insn_replacement(void);
+#endif
+
 static inline void execute_selftests(void)
 {
-    const uint32_t exec_mask = XEN_SYSCTL_TEST_SMOC_ALL;
+    const uint32_t exec_mask = XEN_SYSCTL_TEST_SMOC_ALL &
+                               ~XEN_SYSCTL_TEST_SMOC_LP;
     uint32_t result;
     int rc;
 
diff --git a/xen/arch/x86/test/Makefile b/xen/arch/x86/test/Makefile
index b504b8196659..3a5a0a98e4db 100644
--- a/xen/arch/x86/test/Makefile
+++ b/xen/arch/x86/test/Makefile
@@ -1 +1,3 @@
 obj-y += smoc.o
+obj-$(CONFIG_LIVEPATCH)   += smoc-lp.o     # for livepatch testing
+extra-$(CONFIG_LIVEPATCH) += smoc-lp-alt.o
diff --git a/xen/arch/x86/test/smoc-lp-alt.c b/xen/arch/x86/test/smoc-lp-alt.c
new file mode 100644
index 000000000000..16cf65dafdc1
--- /dev/null
+++ b/xen/arch/x86/test/smoc-lp-alt.c
@@ -0,0 +1,24 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#include <asm/test.h>
+
+/*
+ * Interesting case because `return false` can be encoded as `xor %eax, %eax`,
+ * which is shorter than `return true` which is encoded as a `mov $1, %eax`
+ * instruction (based on code generated by GCC 13.2 at -O2), and also shorter
+ * than the replacement `jmp` instruction.
+ */
+bool cf_check test_lp_insn_replacement(void)
+{
+    return true;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/x86/test/smoc-lp.c b/xen/arch/x86/test/smoc-lp.c
new file mode 100644
index 000000000000..ac32bce4d464
--- /dev/null
+++ b/xen/arch/x86/test/smoc-lp.c
@@ -0,0 +1,24 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#include <asm/test.h>
+
+/*
+ * Interesting case because `return false` can be encoded as `xor %eax, %eax`,
+ * which is shorter than `return true` which is encoded as a `mov $1, %eax`
+ * instruction (based on code generated by GCC 13.2 at -O2), and also shorter
+ * than the replacement `jmp` instruction.
+ */
+bool cf_check test_lp_insn_replacement(void)
+{
+    return false;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/x86/test/smoc.c b/xen/arch/x86/test/smoc.c
index 09db5cee9ae2..3a6091141fdf 100644
--- a/xen/arch/x86/test/smoc.c
+++ b/xen/arch/x86/test/smoc.c
@@ -27,6 +27,10 @@ int test_smoc(uint32_t selection, uint32_t *results)
     } static const tests[] = {
         { XEN_SYSCTL_TEST_SMOC_INSN_REPL, &test_insn_replacement,
           "alternative instruction replacement" },
+#ifdef CONFIG_LIVEPATCH
+        { XEN_SYSCTL_TEST_SMOC_LP_INSN, &test_lp_insn_replacement,
+          "livepatch instruction replacement" },
+#endif
     };
     unsigned int i;
 
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index d015a490da38..f12fc1e2f110 100644
--- a/xen/include/public/sysctl.h
+++ b/xen/include/public/sysctl.h
@@ -1204,7 +1204,11 @@ struct xen_sysctl_dt_overlay {
 struct xen_sysctl_test_smoc {
     uint32_t tests;     /* IN: bitmap with selected tests to execute. */
 #define XEN_SYSCTL_TEST_SMOC_INSN_REPL   (1U << 0)
-#define XEN_SYSCTL_TEST_SMOC_ALL         (XEN_SYSCTL_TEST_SMOC_INSN_REPL)
+#define XEN_SYSCTL_TEST_SMOC_LP_INSN     (1U << 1)
+#define XEN_SYSCTL_TEST_SMOC_ALL         (XEN_SYSCTL_TEST_SMOC_INSN_REPL | \
+                                         XEN_SYSCTL_TEST_SMOC_LP_INSN)
+#define XEN_SYSCTL_TEST_SMOC_LP          (XEN_SYSCTL_TEST_SMOC_LP_INSN)
+
     uint32_t results;   /* OUT: test result: 1 -> success, 0 -> failure. */
 };
 
-- 
2.43.0

[PATCH v4 4/4] automation: add x86-64 livepatching test

[Roger Pau Monne]

Introduce a new gitlab tests for livepatching, using livepatch-build-tools,
which better reflects how downstreams build live patches rather than the
in-tree tests.

The tests applies the dummy in-tree patch example, checks that the patch is
applied correctly and then reverts and unloads it.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
Changes since v2:
 - Split livepatch build into a separate script.
---
 automation/gitlab-ci/build.yaml               |  9 +++
 automation/gitlab-ci/test.yaml                |  8 +++
 automation/scripts/build-livepatch            | 27 ++++++++
 .../scripts/qemu-alpine-x86_64-livepatch.sh   | 68 +++++++++++++++++++
 4 files changed, 112 insertions(+)
 create mode 100755 automation/scripts/build-livepatch
 create mode 100755 automation/scripts/qemu-alpine-x86_64-livepatch.sh

diff --git a/automation/gitlab-ci/build.yaml b/automation/gitlab-ci/build.yaml
index 32af30ccedc9..d770bffb845e 100644
--- a/automation/gitlab-ci/build.yaml
+++ b/automation/gitlab-ci/build.yaml
@@ -358,6 +358,15 @@ alpine-3.18-gcc-debug:
   variables:
     CONTAINER: alpine:3.18
 
+alpine-3.18-gcc-livepatch:
+  extends: .gcc-x86-64-build
+  script:
+    - ./automation/scripts/build-livepatch 2>&1 | tee build.log
+  variables:
+    CONTAINER: alpine:3.18
+    EXTRA_XEN_CONFIG: |
+      CONFIG_LIVEPATCH=y
+
 debian-stretch-gcc-debug:
   extends: .gcc-x86-64-build-debug
   variables:
diff --git a/automation/gitlab-ci/test.yaml b/automation/gitlab-ci/test.yaml
index 6aabdb9d156f..58a90be5ed0e 100644
--- a/automation/gitlab-ci/test.yaml
+++ b/automation/gitlab-ci/test.yaml
@@ -459,3 +459,11 @@ qemu-smoke-ppc64le-powernv9-gcc:
   needs:
     - qemu-system-ppc64-8.1.0-ppc64-export
     - debian-bullseye-gcc-ppc64le-debug
+
+qemu-alpine-x86_64-gcc-livepatch:
+  extends: .qemu-x86-64
+  script:
+    - ./automation/scripts/qemu-alpine-x86_64-livepatch.sh 2>&1 | tee ${LOGFILE}
+  needs:
+    - *x86-64-test-needs
+    - alpine-3.18-gcc-livepatch
diff --git a/automation/scripts/build-livepatch b/automation/scripts/build-livepatch
new file mode 100755
index 000000000000..ac86b17ae5e4
--- /dev/null
+++ b/automation/scripts/build-livepatch
@@ -0,0 +1,27 @@
+#!/bin/bash -ex
+
+# Do a regular build first
+./automation/scripts/build
+
+# Build a test livepatch using livepatch-build-tools.
+
+if [[ "$XEN_TARGET_ARCH" != "x86_64" ]]; then
+    exit 1
+fi
+
+BASE=xen/arch/x86/test/smoc-lp.c
+ALT=xen/arch/x86/test/smoc-lp-alt.c
+
+[[ -f $BASE && -f $ALT ]]
+
+# git diff --no-index returns 0 if no differences, otherwise 1.
+git diff --no-index --output=test.patch $BASE $ALT && exit 1
+
+BUILDID=$(readelf -Wn xen/xen-syms | sed -n -e 's/^.*Build ID: //p')
+
+git clone https://xenbits.xen.org/git-http/livepatch-build-tools.git
+cd livepatch-build-tools
+make
+./livepatch-build -s ../ -p ../test.patch -o out -c ../xen/.config \
+    --depends $BUILDID --xen-depends $BUILDID
+cp out/test.livepatch ../binaries/test.livepatch
diff --git a/automation/scripts/qemu-alpine-x86_64-livepatch.sh b/automation/scripts/qemu-alpine-x86_64-livepatch.sh
new file mode 100755
index 000000000000..da478cac4376
--- /dev/null
+++ b/automation/scripts/qemu-alpine-x86_64-livepatch.sh
@@ -0,0 +1,68 @@
+#!/bin/bash
+
+set -ex
+
+cd binaries
+# initrd.tar.gz is Dom0 rootfs
+mkdir -p rootfs
+cd rootfs
+tar xvzf ../initrd.tar.gz
+mkdir proc
+mkdir run
+mkdir srv
+mkdir sys
+rm var/run
+cp -ar ../dist/install/* .
+cp ../test.livepatch ./root/
+cat << "EOF" >> etc/local.d/xen-lp.start
+#!/bin/bash
+
+set -ex
+
+trap poweroff EXIT
+
+export LD_LIBRARY_PATH=/usr/local/lib
+
+xen-livepatch test && exit 1 || true
+
+xen-livepatch load /root/test.livepatch
+
+# Cannot fail now
+xen-livepatch test
+
+xen-livepatch revert test
+xen-livepatch unload test
+
+xen-livepatch test && exit 1 || true
+
+echo "SUCCESS"
+EOF
+chmod +x etc/local.d/xen-lp.start
+echo "rc_verbose=yes" >> etc/rc.conf
+# rebuild Dom0 rootfs
+find . |cpio -H newc -o|gzip > ../xen-rootfs.cpio.gz
+cd ../..
+
+cat >> binaries/pxelinux.0 << EOF
+#!ipxe
+
+kernel xen console=com1 console_timestamps=boot
+module bzImage console=hvc0
+module xen-rootfs.cpio.gz
+boot
+EOF
+
+# Run the test
+rm -f smoke.serial
+timeout -k 1 360 \
+qemu-system-x86_64 \
+    -cpu qemu64,+svm \
+    -m 2G -smp 2 \
+    -monitor none -serial stdio \
+    -nographic \
+    -device virtio-net-pci,netdev=n0 \
+    -netdev user,id=n0,tftp=binaries,bootfile=/pxelinux.0 |& \
+        tee smoke.serial | sed 's/\r//'
+
+grep -q "SUCCESS" smoke.serial
+exit 0
-- 
2.43.0

Re: [PATCH v4 1/4] x86/livepatch: align functions to ensure minimal distance between entry points

[Jan Beulich]

On 15.12.2023 12:18, Roger Pau Monne wrote:
> The minimal function size requirements for livepatch are either 5 bytes (for
> jmp) or 9 bytes (for endbr + jmp) on x86, and always 4 bytes on Arm.  Ensure
> that distance between functions entry points is always at least of the minimal
> required size for livepatch instruction replacement to be successful.
> 
> Add an additional align directive to the linker script, in order to ensure that
> the next section placed after the .text.* (per-function sections) is also
> aligned to the required boundary, so that the distance of the last function
> entry point with the next symbol is also of minimal size.
> 
> Note that it's possible for the compiler to end up using a higher function
> alignment regardless of the passed value, so this change just make sure that
> the minimum required for livepatch to work is present.  Different compilers
> handle the option differently, as clang will ignore -falign-functions value
> if it's smaller than the one that would be set by the optimization level, while
> gcc seems to always honor the function alignment passed in -falign-functions.
> In order to cope with this behavior and avoid that setting -falign-functions
> results in an alignment inferior to what the optimization level would have
> selected force x86 release builds to use a function alignment of 16 bytes.
> 
> The compiler option -falign-functions is not available on at least clang 3.8,
> so introduce a Kconfig check for it and make the livepatch option depend on the
> compiler supporting the option.
> 
> The naming of the option(s) CONFIG_FUNCTION_ALIGNMENT is explicitly not
> mentioning CC in preparation for the option also being used by assembly code.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

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

Re: [PATCH v4 2/4] xen/x86: introduce self modifying code test

[Jan Beulich]

On 15.12.2023 12:18, Roger Pau Monne wrote:
> Introduce a helper to perform checks related to self modifying code, and start
> by creating a simple test to check that alternatives have been applied.
> 
> Such test is hooked into the boot process and called just after alternatives
> have been applied.  In case of failure a message is printed, and the hypervisor
> is tainted as not having passed the tests, this does require introducing a new
> taint bit (printed as 'T').
> 
> A new sysctl is also introduced to run the tests on demand.  While there are no
> current users introduced here, further changes will introduce those, and it's
> helpful to have the interface defined in the sysctl header from the start.
> 
> Note the sysctl visibility is not limited to x86, albeit the only
> implementation is for x86.  It's expected that other architectures can reuse
> the same sysctl and structure, with possibly different tests.  Leave adjusting
> those to when support for a different architecture is introduced, as the
> sysctl interface is not stable anyway.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

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

Re: [PATCH v4 1/4] x86/livepatch: align functions to ensure minimal distance between entry points

[Andrew Cooper]

On 15/12/2023 11:18 am, Roger Pau Monne wrote:
> The minimal function size requirements for livepatch are either 5 bytes (for

"for an x86 livepatch", seeing as we're touching multiple architectures
worth of files.

I know it's at the end of the sentence, but it wants to be earlier to be
clearer.

> jmp) or 9 bytes (for endbr + jmp) on x86, and always 4 bytes on Arm.  Ensure
> that distance between functions entry points is always at least of the minimal
> required size for livepatch instruction replacement to be successful.
>
> Add an additional align directive to the linker script, in order to ensure that
> the next section placed after the .text.* (per-function sections) is also
> aligned to the required boundary, so that the distance of the last function
> entry point with the next symbol is also of minimal size.
>
> Note that it's possible for the compiler to end up using a higher function
> alignment regardless of the passed value, so this change just make sure that
> the minimum required for livepatch to work is present.  Different compilers
> handle the option differently, as clang will ignore -falign-functions value
> if it's smaller than the one that would be set by the optimization level, while
> gcc seems to always honor the function alignment passed in -falign-functions.
> In order to cope with this behavior and avoid that setting -falign-functions
> results in an alignment inferior to what the optimization level would have
> selected force x86 release builds to use a function alignment of 16 bytes.

Yuck :(

The same will be true for all other architectures too?

What happens on ARM, which also picks up an explicit choice in livepatch
builds?

>
> The compiler option -falign-functions is not available on at least clang 3.8,
> so introduce a Kconfig check for it and make the livepatch option depend on the
> compiler supporting the option.
>
> The naming of the option(s) CONFIG_FUNCTION_ALIGNMENT is explicitly not
> mentioning CC in preparation for the option also being used by assembly code.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> Changes since v3:
>  - Test for compiler option with -falign-functions.
>  - Make FUNCTION_ALIGNMENT depend on CC_HAS_FUNCTION_ALIGNMENT.
>  - Set 16byte function alignment for x86 release builds.
>
> Changes since v2:
>  - Add Arm side.
>  - Align end of section in the linker script to ensure enough padding for the
>    last function.
>  - Expand commit message and subject.
>  - Rework Kconfig options.
>  - Check that the compiler supports the option.
>
> Changes since v1:
>  - New in this version.
> ---
>  xen/Kconfig              | 19 +++++++++++++++++++
>  xen/Makefile             |  3 +++
>  xen/arch/arm/livepatch.c |  2 ++
>  xen/arch/arm/xen.lds.S   |  4 ++++
>  xen/arch/x86/Kconfig     |  1 +
>  xen/arch/x86/livepatch.c |  4 ++++
>  xen/arch/x86/xen.lds.S   |  4 ++++
>  xen/common/Kconfig       |  5 ++++-
>  8 files changed, 41 insertions(+), 1 deletion(-)

xen$ git ls-files | grep xen.lds.S
arch/arm/xen.lds.S
arch/ppc/xen.lds.S
arch/riscv/xen.lds.S
arch/x86/xen.lds.S

RISC-V and PPC have the same pattern that you're patching for x86 and ARM.


> diff --git a/xen/Kconfig b/xen/Kconfig
> index 134e6e68ad84..c2cc3fe165eb 100644
> --- a/xen/Kconfig
> +++ b/xen/Kconfig
> @@ -37,6 +37,25 @@ config CC_HAS_VISIBILITY_ATTRIBUTE
>  config CC_SPLIT_SECTIONS
>  	bool
>  
> +# Set function alignment.
> +#
> +# Allow setting on a boolean basis, and then convert such selection to an
> +# integer for the build system and code to consume more easily.

# Clang >= 6.0

> +config CC_HAS_FUNCTION_ALIGNMENT
> +	def_bool $(cc-option,-falign-functions)
> +config FUNCTION_ALIGNMENT_4B
> +	bool
> +config FUNCTION_ALIGNMENT_8B
> +	bool
> +config FUNCTION_ALIGNMENT_16B
> +	bool
> +config FUNCTION_ALIGNMENT
> +	int
> +	depends on CC_HAS_FUNCTION_ALIGNMENT
> +	default 16 if FUNCTION_ALIGNMENT_16B
> +	default  8 if  FUNCTION_ALIGNMENT_8B
> +	default  4 if  FUNCTION_ALIGNMENT_4B

What value do we get here for RISCV/PPC?  Do we need another override
for them?

~Andrew

Re: [PATCH v4 2/4] xen/x86: introduce self modifying code test

[Andrew Cooper]

On 15/12/2023 11:18 am, Roger Pau Monne wrote:
> Introduce a helper to perform checks related to self modifying code, and start
> by creating a simple test to check that alternatives have been applied.
>
> Such test is hooked into the boot process and called just after alternatives
> have been applied.  In case of failure a message is printed, and the hypervisor
> is tainted as not having passed the tests, this does require introducing a new
> taint bit (printed as 'T').

We've got stub_selftest() in extable.c which currently does an ah-hoc
form of this taint via warning_add().

Nothing else comes to mind, but I would suggest breaking out the new
taint into an earlier patch, as this one is complicated enough anyway.

> diff --git a/xen/arch/x86/include/asm/test.h b/xen/arch/x86/include/asm/test.h
> new file mode 100644
> index 000000000000..e96e709c6a52
> --- /dev/null
> +++ b/xen/arch/x86/include/asm/test.h
> @@ -0,0 +1,30 @@
> +#ifndef _ASM_X86_TEST_H_
> +#define _ASM_X86_TEST_H_
> +
> +#include <xen/types.h>
> +
> +int test_smoc(uint32_t selection, uint32_t *results);
> +
> +static inline void execute_selftests(void)

IMO run_selftests() would be better, but this is already not all of our
selftests, and this particular function really doesn't warrant being
static inline.

But I'm also not sure what this is liable to contain other than
test_smoc() so I'm not sure why we want it.

> diff --git a/xen/arch/x86/test/smoc.c b/xen/arch/x86/test/smoc.c
> new file mode 100644
> index 000000000000..09db5cee9ae2
> --- /dev/null
> +++ b/xen/arch/x86/test/smoc.c
> @@ -0,0 +1,66 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#include <xen/errno.h>
> +
> +#include <asm/alternative.h>
> +#include <asm/cpufeature.h>
> +#include <asm/test.h>
> +
> +static bool cf_check test_insn_replacement(void)
> +{
> +#define EXPECTED_VALUE 2
> +    unsigned int r = ~EXPECTED_VALUE;
> +
> +    alternative_io("", "mov %1, %0", X86_FEATURE_ALWAYS,
> +                   "+r" (r), "i" (EXPECTED_VALUE));
> +
> +    return r == EXPECTED_VALUE;
> +#undef EXPECTED_VALUE
> +}
> +
> +int test_smoc(uint32_t selection, uint32_t *results)
> +{
> +    struct {
> +        unsigned int mask;
> +        bool (*test)(void);
> +        const char *name;
> +    } static const tests[] = {
> +        { XEN_SYSCTL_TEST_SMOC_INSN_REPL, &test_insn_replacement,
> +          "alternative instruction replacement" },
> +    };

Ah.  I realise I said "like XTF", but I meant "checking one thing at a
time".

While this pattern for tests[] is very convenient in XTF, it has one
major downside in Xen, and that's the proliferation of ENDBR's in the
running binary.

Also (see below), returning bool isn't ok.  In the case of a failure, we
need:

printk(XENLOG_ERR "%s() Failure: Expected $FOO, got $BAR\n");

because that's what a human needs to know in order to fix the issue, not
a generic "something failed".

> +    unsigned int i;
> +
> +    if ( selection & ~XEN_SYSCTL_TEST_SMOC_ALL )
> +        return -EINVAL;

I'm not sure this is sensible.  It's a testing hypercall, so why
shouldn't I be able to pass ~0 to mean "test everything the hypervisor
knows about" ?

> +
> +    if ( results )
> +        *results = 0;
> +
> +    for ( i = 0; i < ARRAY_SIZE(tests); i++ )
> +    {
> +        if ( !(selection & tests[i].mask) )
> +            continue;
> +
> +        if ( tests[i].test() )
> +        {
> +            if ( results )
> +                *results |= tests[i].mask;

How is results supposed to be used?

XEN_SYSCTL_TEST_SMOC_INSN_REPL covers about 15 things we want to test,
making this output mask useless.


The selftests, like the exception fixup ones, are supposed to be
guarantee pass.  Failure is an exceptional case, and is only expected to
be found with new compilers and new SMC development.

I can kind of see how an input mask might be useful, although I wouldn't
have had one myself.  With correct diagnostics, running the hypercall
multiple times isn't useful to debugging, and without correct
diagnostics, the feedback provided by this is useless.

So honestly, I think this "results" output is overengineered and doesn't
help the cases where it is actually going to matter.


Remember most of all that self-modifying code which are going to cause
failures here have a high chance of crashing Xen outright.  And we're
deliberately trying to make this happen in CI and before a breaking
change gets out into releases.

> +            continue;
> +        }
> +
> +        if ( system_state < SYS_STATE_active )
> +            printk(XENLOG_ERR "%s test failed\n", tests[i].name);

This is a test hypercall, for the purpose of running testing, in
combination with test livepatches.  Eliding the diagnostics isn't ok.

Logspam concerns aren't an issue.  If the user runs `while :; do
xen-test-smc; done` in dom0 then they get to have a full dmesg ring.

Don't let that get in the way of having a sensible time figuring out
what went wrong.

~Andrew

Re: [PATCH v4 1/4] x86/livepatch: align functions to ensure minimal distance between entry points

[Roger Pau Monné]

On Tue, Dec 19, 2023 at 07:46:11PM +0000, Andrew Cooper wrote:
> On 15/12/2023 11:18 am, Roger Pau Monne wrote:
> > The minimal function size requirements for livepatch are either 5 bytes (for
> 
> "for an x86 livepatch", seeing as we're touching multiple architectures
> worth of files.
> 
> I know it's at the end of the sentence, but it wants to be earlier to be
> clearer.
> 
> > jmp) or 9 bytes (for endbr + jmp) on x86, and always 4 bytes on Arm.  Ensure
> > that distance between functions entry points is always at least of the minimal
> > required size for livepatch instruction replacement to be successful.
> >
> > Add an additional align directive to the linker script, in order to ensure that
> > the next section placed after the .text.* (per-function sections) is also
> > aligned to the required boundary, so that the distance of the last function
> > entry point with the next symbol is also of minimal size.
> >
> > Note that it's possible for the compiler to end up using a higher function
> > alignment regardless of the passed value, so this change just make sure that
> > the minimum required for livepatch to work is present.  Different compilers
> > handle the option differently, as clang will ignore -falign-functions value
> > if it's smaller than the one that would be set by the optimization level, while
> > gcc seems to always honor the function alignment passed in -falign-functions.
> > In order to cope with this behavior and avoid that setting -falign-functions
> > results in an alignment inferior to what the optimization level would have
> > selected force x86 release builds to use a function alignment of 16 bytes.
> 
> Yuck :(
> 
> The same will be true for all other architectures too?

I would expect that for gcc I guess.

> What happens on ARM, which also picks up an explicit choice in livepatch
> builds?

Arm AFAICT seems to use a 4 byte function alignment with -O2 (both gcc
and clang), so that matches what we need to enforce for livepatch.  If
we ever need a higher alignment for livepatch reasons it would be a
multiple of the minimum one set by the compiler, so that should be
fine.

> >
> > The compiler option -falign-functions is not available on at least clang 3.8,
> > so introduce a Kconfig check for it and make the livepatch option depend on the
> > compiler supporting the option.
> >
> > The naming of the option(s) CONFIG_FUNCTION_ALIGNMENT is explicitly not
> > mentioning CC in preparation for the option also being used by assembly code.
> >
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > Changes since v3:
> >  - Test for compiler option with -falign-functions.
> >  - Make FUNCTION_ALIGNMENT depend on CC_HAS_FUNCTION_ALIGNMENT.
> >  - Set 16byte function alignment for x86 release builds.
> >
> > Changes since v2:
> >  - Add Arm side.
> >  - Align end of section in the linker script to ensure enough padding for the
> >    last function.
> >  - Expand commit message and subject.
> >  - Rework Kconfig options.
> >  - Check that the compiler supports the option.
> >
> > Changes since v1:
> >  - New in this version.
> > ---
> >  xen/Kconfig              | 19 +++++++++++++++++++
> >  xen/Makefile             |  3 +++
> >  xen/arch/arm/livepatch.c |  2 ++
> >  xen/arch/arm/xen.lds.S   |  4 ++++
> >  xen/arch/x86/Kconfig     |  1 +
> >  xen/arch/x86/livepatch.c |  4 ++++
> >  xen/arch/x86/xen.lds.S   |  4 ++++
> >  xen/common/Kconfig       |  5 ++++-
> >  8 files changed, 41 insertions(+), 1 deletion(-)
> 
> xen$ git ls-files | grep xen.lds.S
> arch/arm/xen.lds.S
> arch/ppc/xen.lds.S
> arch/riscv/xen.lds.S
> arch/x86/xen.lds.S
> 
> RISC-V and PPC have the same pattern that you're patching for x86 and ARM.

I've avoided touching those because there's no livepatch support there
(yet), and I didn't want to give the impression that the option is
supported or tested for those architectures.  I have no idea what
function alignments would be sensible for riscv or ppc.

> > diff --git a/xen/Kconfig b/xen/Kconfig
> > index 134e6e68ad84..c2cc3fe165eb 100644
> > --- a/xen/Kconfig
> > +++ b/xen/Kconfig
> > @@ -37,6 +37,25 @@ config CC_HAS_VISIBILITY_ATTRIBUTE
> >  config CC_SPLIT_SECTIONS
> >  	bool
> >  
> > +# Set function alignment.
> > +#
> > +# Allow setting on a boolean basis, and then convert such selection to an
> > +# integer for the build system and code to consume more easily.
> 
> # Clang >= 6.0
> 
> > +config CC_HAS_FUNCTION_ALIGNMENT
> > +	def_bool $(cc-option,-falign-functions)
> > +config FUNCTION_ALIGNMENT_4B
> > +	bool
> > +config FUNCTION_ALIGNMENT_8B
> > +	bool
> > +config FUNCTION_ALIGNMENT_16B
> > +	bool
> > +config FUNCTION_ALIGNMENT
> > +	int
> > +	depends on CC_HAS_FUNCTION_ALIGNMENT
> > +	default 16 if FUNCTION_ALIGNMENT_16B
> > +	default  8 if  FUNCTION_ALIGNMENT_8B
> > +	default  4 if  FUNCTION_ALIGNMENT_4B
> 
> What value do we get here for RISCV/PPC?  Do we need another override
> for them?

Hm, I wasn't planning on adding support for PPC/RISCV here, if those
arches want to use a specific function alignment they might need to
adjust the options here, I think that's a reasonable compromise, as I
don't see a need for this to be blocked on also agreeing values for
ppc or riscv.

Thanks, Roger.

Re: [PATCH v4 1/4] x86/livepatch: align functions to ensure minimal distance between entry points

[Jan Beulich]

On 20.12.2023 09:32, Roger Pau Monné wrote:
> On Tue, Dec 19, 2023 at 07:46:11PM +0000, Andrew Cooper wrote:
>> On 15/12/2023 11:18 am, Roger Pau Monne wrote:
>>> The minimal function size requirements for livepatch are either 5 bytes (for
>>
>> "for an x86 livepatch", seeing as we're touching multiple architectures
>> worth of files.
>>
>> I know it's at the end of the sentence, but it wants to be earlier to be
>> clearer.
>>
>>> jmp) or 9 bytes (for endbr + jmp) on x86, and always 4 bytes on Arm.  Ensure
>>> that distance between functions entry points is always at least of the minimal
>>> required size for livepatch instruction replacement to be successful.
>>>
>>> Add an additional align directive to the linker script, in order to ensure that
>>> the next section placed after the .text.* (per-function sections) is also
>>> aligned to the required boundary, so that the distance of the last function
>>> entry point with the next symbol is also of minimal size.
>>>
>>> Note that it's possible for the compiler to end up using a higher function
>>> alignment regardless of the passed value, so this change just make sure that
>>> the minimum required for livepatch to work is present.  Different compilers
>>> handle the option differently, as clang will ignore -falign-functions value
>>> if it's smaller than the one that would be set by the optimization level, while
>>> gcc seems to always honor the function alignment passed in -falign-functions.
>>> In order to cope with this behavior and avoid that setting -falign-functions
>>> results in an alignment inferior to what the optimization level would have
>>> selected force x86 release builds to use a function alignment of 16 bytes.
>>
>> Yuck :(
>>
>> The same will be true for all other architectures too?
> 
> I would expect that for gcc I guess.
> 
>> What happens on ARM, which also picks up an explicit choice in livepatch
>> builds?
> 
> Arm AFAICT seems to use a 4 byte function alignment with -O2 (both gcc
> and clang), so that matches what we need to enforce for livepatch.  If
> we ever need a higher alignment for livepatch reasons it would be a
> multiple of the minimum one set by the compiler, so that should be
> fine.

Thinking of it: The forcing of 16-byte alignment in release builds of x86
is based on observations with certain compiler versions, iirc. What if
future versions decide to go lower/higher for, perhaps, very good reasons?
We don't really mean to override the compiler's choice, so maybe further
probing is actually necessary?

Jan

Re: [PATCH v4 2/4] xen/x86: introduce self modifying code test

[Roger Pau Monné]

On Tue, Dec 19, 2023 at 08:31:29PM +0000, Andrew Cooper wrote:
> On 15/12/2023 11:18 am, Roger Pau Monne wrote:
> > Introduce a helper to perform checks related to self modifying code, and start
> > by creating a simple test to check that alternatives have been applied.
> >
> > Such test is hooked into the boot process and called just after alternatives
> > have been applied.  In case of failure a message is printed, and the hypervisor
> > is tainted as not having passed the tests, this does require introducing a new
> > taint bit (printed as 'T').
> 
> We've got stub_selftest() in extable.c which currently does an ah-hoc
> form of this taint via warning_add().
> 
> Nothing else comes to mind, but I would suggest breaking out the new
> taint into an earlier patch, as this one is complicated enough anyway.

I see, so introduce the taint in a previous patch and use it in
stub_selftest() failure,

> > diff --git a/xen/arch/x86/include/asm/test.h b/xen/arch/x86/include/asm/test.h
> > new file mode 100644
> > index 000000000000..e96e709c6a52
> > --- /dev/null
> > +++ b/xen/arch/x86/include/asm/test.h
> > @@ -0,0 +1,30 @@
> > +#ifndef _ASM_X86_TEST_H_
> > +#define _ASM_X86_TEST_H_
> > +
> > +#include <xen/types.h>
> > +
> > +int test_smoc(uint32_t selection, uint32_t *results);
> > +
> > +static inline void execute_selftests(void)
> 
> IMO run_selftests() would be better, but this is already not all of our
> selftests, and this particular function really doesn't warrant being
> static inline.
> 
> But I'm also not sure what this is liable to contain other than
> test_smoc() so I'm not sure why we want it.

This was requested by Jan, he was concerned that more of such tests
would appear.  It's new in v4 IIRC.

> > diff --git a/xen/arch/x86/test/smoc.c b/xen/arch/x86/test/smoc.c
> > new file mode 100644
> > index 000000000000..09db5cee9ae2
> > --- /dev/null
> > +++ b/xen/arch/x86/test/smoc.c
> > @@ -0,0 +1,66 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +
> > +#include <xen/errno.h>
> > +
> > +#include <asm/alternative.h>
> > +#include <asm/cpufeature.h>
> > +#include <asm/test.h>
> > +
> > +static bool cf_check test_insn_replacement(void)
> > +{
> > +#define EXPECTED_VALUE 2
> > +    unsigned int r = ~EXPECTED_VALUE;
> > +
> > +    alternative_io("", "mov %1, %0", X86_FEATURE_ALWAYS,
> > +                   "+r" (r), "i" (EXPECTED_VALUE));
> > +
> > +    return r == EXPECTED_VALUE;
> > +#undef EXPECTED_VALUE
> > +}
> > +
> > +int test_smoc(uint32_t selection, uint32_t *results)
> > +{
> > +    struct {
> > +        unsigned int mask;
> > +        bool (*test)(void);
> > +        const char *name;
> > +    } static const tests[] = {
> > +        { XEN_SYSCTL_TEST_SMOC_INSN_REPL, &test_insn_replacement,
> > +          "alternative instruction replacement" },
> > +    };
> 
> Ah.  I realise I said "like XTF", but I meant "checking one thing at a
> time".
> 
> While this pattern for tests[] is very convenient in XTF, it has one
> major downside in Xen, and that's the proliferation of ENDBR's in the
> running binary.

But for the livepatch case for example it's interesting to patch
functions that have the ENDBR prefix.

I do like having all the tests in an array, as then adding new ones is
trivial.

> Also (see below), returning bool isn't ok.  In the case of a failure, we
> need:
> 
> printk(XENLOG_ERR "%s() Failure: Expected $FOO, got $BAR\n");

There's already a message printed below, that's currently limited
to system_state < SYS_STATE_active, but I would be fine with printing
unconditionally that prints which test failed in a human readable
form:

printk(XENLOG_ERR "%s test failed\n", tests[i].name);

So that would print:

"alternative instruction replacement test failed"

on the Xen dmesg.

On one of the first versions test functions did return a value, but I
ended up switching to this boolean version because I didn't see much
value in returning anything that's not success or failure from the
tests.

I can switch back to returning a value, and then the array of tests
will also store the expected returned value.

> because that's what a human needs to know in order to fix the issue, not
> a generic "something failed".
> 
> > +    unsigned int i;
> > +
> > +    if ( selection & ~XEN_SYSCTL_TEST_SMOC_ALL )
> > +        return -EINVAL;
> 
> I'm not sure this is sensible.  It's a testing hypercall, so why
> shouldn't I be able to pass ~0 to mean "test everything the hypervisor
> knows about" ?

Well, for one livepatch tests will fail if the livepatch hasn't been
applied yet.

> > +
> > +    if ( results )
> > +        *results = 0;
> > +
> > +    for ( i = 0; i < ARRAY_SIZE(tests); i++ )
> > +    {
> > +        if ( !(selection & tests[i].mask) )
> > +            continue;
> > +
> > +        if ( tests[i].test() )
> > +        {
> > +            if ( results )
> > +                *results |= tests[i].mask;
> 
> How is results supposed to be used?
> 
> XEN_SYSCTL_TEST_SMOC_INSN_REPL covers about 15 things we want to test,
> making this output mask useless.

The output mask just maps the input tests into output results.

For example given the case you want to execute all tests (~0), and the
livepatch replacements haven't been applied yet, the altinstructions
test will succeed, but the livepatch ones will fail (as expected), we
need a way to report this back to the caller.

> 
> The selftests, like the exception fixup ones, are supposed to be
> guarantee pass.  Failure is an exceptional case, and is only expected to
> be found with new compilers and new SMC development.

Livepatch tests (at least the one I have implemented in patch 3) is
expected to fail until a livepatch is applied to make it succeed.  We
do care about checking that it first fails, then we upload the
livepatch and it succeeds, and that reverting the livepatch makes it
fail again.

> I can kind of see how an input mask might be useful, although I wouldn't
> have had one myself.  With correct diagnostics, running the hypercall
> multiple times isn't useful to debugging, and without correct
> diagnostics, the feedback provided by this is useless.
> 
> So honestly, I think this "results" output is overengineered and doesn't
> help the cases where it is actually going to matter.

So for altinstructions it's true that the expectation is for them to
always succeed, that's not the case for livepatch ones, where it's
useful to explicitly test for failure, hence we need a fine grained
way to report failure of specific tests.

> 
> Remember most of all that self-modifying code which are going to cause
> failures here have a high chance of crashing Xen outright.  And we're
> deliberately trying to make this happen in CI and before a breaking
> change gets out into releases.
> 
> > +            continue;
> > +        }
> > +
> > +        if ( system_state < SYS_STATE_active )
> > +            printk(XENLOG_ERR "%s test failed\n", tests[i].name);
> 
> This is a test hypercall, for the purpose of running testing, in
> combination with test livepatches.  Eliding the diagnostics isn't ok.
> 
> Logspam concerns aren't an issue.  If the user runs `while :; do
> xen-test-smc; done` in dom0 then they get to have a full dmesg ring.
> 
> Don't let that get in the way of having a sensible time figuring out
> what went wrong.

This was requested by Jan, and indeed my original intention was to
unconditionally print the messages, as I think they are helpful.

Thanks, Roger.

Re: [PATCH v4 2/4] xen/x86: introduce self modifying code test

[Jan Beulich]

On 19.12.2023 21:31, Andrew Cooper wrote:
> On 15/12/2023 11:18 am, Roger Pau Monne wrote:
>> +        if ( system_state < SYS_STATE_active )
>> +            printk(XENLOG_ERR "%s test failed\n", tests[i].name);
> 
> This is a test hypercall, for the purpose of running testing, in
> combination with test livepatches.  Eliding the diagnostics isn't ok.
> 
> Logspam concerns aren't an issue.  If the user runs `while :; do
> xen-test-smc; done` in dom0 then they get to have a full dmesg ring.
> 
> Don't let that get in the way of having a sensible time figuring out
> what went wrong.

Since it was me who asked to suppress this when invoked through sysctl:
I can live with an unconditional printk() here, but I think this goes
against the "what can be done in user space should be done there"
principle: If enough information is propagated back, user space should
be able to provide all necessary output without even a need for the
observer to run "xl dmesg".

Jan

Re: [PATCH v4 2/4] xen/x86: introduce self modifying code test

[Jan Beulich]

On 20.12.2023 10:08, Roger Pau Monné wrote:
> On Tue, Dec 19, 2023 at 08:31:29PM +0000, Andrew Cooper wrote:
>> On 15/12/2023 11:18 am, Roger Pau Monne wrote:
>>> --- /dev/null
>>> +++ b/xen/arch/x86/include/asm/test.h
>>> @@ -0,0 +1,30 @@
>>> +#ifndef _ASM_X86_TEST_H_
>>> +#define _ASM_X86_TEST_H_
>>> +
>>> +#include <xen/types.h>
>>> +
>>> +int test_smoc(uint32_t selection, uint32_t *results);
>>> +
>>> +static inline void execute_selftests(void)
>>
>> IMO run_selftests() would be better, but this is already not all of our
>> selftests, and this particular function really doesn't warrant being
>> static inline.
>>
>> But I'm also not sure what this is liable to contain other than
>> test_smoc() so I'm not sure why we want it.
> 
> This was requested by Jan, he was concerned that more of such tests
> would appear.  It's new in v4 IIRC.

If the two of you want it without such a wrapper, so be it. I will admit
I was a little puzzled to find it implemented as inline function, but I
didn't see a strong need to comment on that.

Jan

Re: [PATCH v4 1/4] x86/livepatch: align functions to ensure minimal distance between entry points

[Roger Pau Monné]

On Wed, Dec 20, 2023 at 10:03:51AM +0100, Jan Beulich wrote:
> On 20.12.2023 09:32, Roger Pau Monné wrote:
> > On Tue, Dec 19, 2023 at 07:46:11PM +0000, Andrew Cooper wrote:
> >> On 15/12/2023 11:18 am, Roger Pau Monne wrote:
> >>> The minimal function size requirements for livepatch are either 5 bytes (for
> >>
> >> "for an x86 livepatch", seeing as we're touching multiple architectures
> >> worth of files.
> >>
> >> I know it's at the end of the sentence, but it wants to be earlier to be
> >> clearer.
> >>
> >>> jmp) or 9 bytes (for endbr + jmp) on x86, and always 4 bytes on Arm.  Ensure
> >>> that distance between functions entry points is always at least of the minimal
> >>> required size for livepatch instruction replacement to be successful.
> >>>
> >>> Add an additional align directive to the linker script, in order to ensure that
> >>> the next section placed after the .text.* (per-function sections) is also
> >>> aligned to the required boundary, so that the distance of the last function
> >>> entry point with the next symbol is also of minimal size.
> >>>
> >>> Note that it's possible for the compiler to end up using a higher function
> >>> alignment regardless of the passed value, so this change just make sure that
> >>> the minimum required for livepatch to work is present.  Different compilers
> >>> handle the option differently, as clang will ignore -falign-functions value
> >>> if it's smaller than the one that would be set by the optimization level, while
> >>> gcc seems to always honor the function alignment passed in -falign-functions.
> >>> In order to cope with this behavior and avoid that setting -falign-functions
> >>> results in an alignment inferior to what the optimization level would have
> >>> selected force x86 release builds to use a function alignment of 16 bytes.
> >>
> >> Yuck :(
> >>
> >> The same will be true for all other architectures too?
> > 
> > I would expect that for gcc I guess.
> > 
> >> What happens on ARM, which also picks up an explicit choice in livepatch
> >> builds?
> > 
> > Arm AFAICT seems to use a 4 byte function alignment with -O2 (both gcc
> > and clang), so that matches what we need to enforce for livepatch.  If
> > we ever need a higher alignment for livepatch reasons it would be a
> > multiple of the minimum one set by the compiler, so that should be
> > fine.
> 
> Thinking of it: The forcing of 16-byte alignment in release builds of x86
> is based on observations with certain compiler versions, iirc. What if
> future versions decide to go lower/higher for, perhaps, very good reasons?
> We don't really mean to override the compiler's choice, so maybe further
> probing is actually necessary?

We do override the default (on gcc) when using livepatch anyway, so we
might as well be consistent and attempt to provide a value that's
reasonable?

Linux currently uses 16-byte also on x86 and set in Kconfig, and all
compiler versions I've tested use 16-bytes with -O2, so I think it's
unlikely to change overnight (and a lot of software will still use 16
anyway).

If a more suitable value is needed in the future we can always adjust,
that's the whole point of having it in Kconfig.  We can also select a
better value even in compilers that might not know about it.

Thanks, Roger.

Re: [PATCH v4 2/4] xen/x86: introduce self modifying code test

[Roger Pau Monné]

On Wed, Dec 20, 2023 at 10:12:15AM +0100, Jan Beulich wrote:
> On 20.12.2023 10:08, Roger Pau Monné wrote:
> > On Tue, Dec 19, 2023 at 08:31:29PM +0000, Andrew Cooper wrote:
> >> On 15/12/2023 11:18 am, Roger Pau Monne wrote:
> >>> --- /dev/null
> >>> +++ b/xen/arch/x86/include/asm/test.h
> >>> @@ -0,0 +1,30 @@
> >>> +#ifndef _ASM_X86_TEST_H_
> >>> +#define _ASM_X86_TEST_H_
> >>> +
> >>> +#include <xen/types.h>
> >>> +
> >>> +int test_smoc(uint32_t selection, uint32_t *results);
> >>> +
> >>> +static inline void execute_selftests(void)
> >>
> >> IMO run_selftests() would be better, but this is already not all of our
> >> selftests, and this particular function really doesn't warrant being
> >> static inline.
> >>
> >> But I'm also not sure what this is liable to contain other than
> >> test_smoc() so I'm not sure why we want it.
> > 
> > This was requested by Jan, he was concerned that more of such tests
> > would appear.  It's new in v4 IIRC.
> 
> If the two of you want it without such a wrapper, so be it. I will admit
> I was a little puzzled to find it implemented as inline function, but I
> didn't see a strong need to comment on that.

It felt a bit misplaced to put such generic selftest function in a
smoc.c file, and I didn't feel like introducing a new test.c file just
for that.  I don't have a strong opinion however, if you think it's
better to go in smoc.c I'm happy with that.

Thanks, Roger.

Re: [PATCH v4 2/4] xen/x86: introduce self modifying code test

[Jan Beulich]

On 20.12.2023 10:25, Roger Pau Monné wrote:
> On Wed, Dec 20, 2023 at 10:12:15AM +0100, Jan Beulich wrote:
>> On 20.12.2023 10:08, Roger Pau Monné wrote:
>>> On Tue, Dec 19, 2023 at 08:31:29PM +0000, Andrew Cooper wrote:
>>>> On 15/12/2023 11:18 am, Roger Pau Monne wrote:
>>>>> --- /dev/null
>>>>> +++ b/xen/arch/x86/include/asm/test.h
>>>>> @@ -0,0 +1,30 @@
>>>>> +#ifndef _ASM_X86_TEST_H_
>>>>> +#define _ASM_X86_TEST_H_
>>>>> +
>>>>> +#include <xen/types.h>
>>>>> +
>>>>> +int test_smoc(uint32_t selection, uint32_t *results);
>>>>> +
>>>>> +static inline void execute_selftests(void)
>>>>
>>>> IMO run_selftests() would be better, but this is already not all of our
>>>> selftests, and this particular function really doesn't warrant being
>>>> static inline.
>>>>
>>>> But I'm also not sure what this is liable to contain other than
>>>> test_smoc() so I'm not sure why we want it.
>>>
>>> This was requested by Jan, he was concerned that more of such tests
>>> would appear.  It's new in v4 IIRC.
>>
>> If the two of you want it without such a wrapper, so be it. I will admit
>> I was a little puzzled to find it implemented as inline function, but I
>> didn't see a strong need to comment on that.
> 
> It felt a bit misplaced to put such generic selftest function in a
> smoc.c file, and I didn't feel like introducing a new test.c file just
> for that.  I don't have a strong opinion however, if you think it's
> better to go in smoc.c I'm happy with that.

My view: smoc.c would be wrong. Then it's better to have no wrapper (for
now).

Jan

Re: [PATCH v4 2/4] xen/x86: introduce self modifying code test

[Roger Pau Monné]

On Wed, Dec 20, 2023 at 10:09:12AM +0100, Jan Beulich wrote:
> On 19.12.2023 21:31, Andrew Cooper wrote:
> > On 15/12/2023 11:18 am, Roger Pau Monne wrote:
> >> +        if ( system_state < SYS_STATE_active )
> >> +            printk(XENLOG_ERR "%s test failed\n", tests[i].name);
> > 
> > This is a test hypercall, for the purpose of running testing, in
> > combination with test livepatches.  Eliding the diagnostics isn't ok.
> > 
> > Logspam concerns aren't an issue.  If the user runs `while :; do
> > xen-test-smc; done` in dom0 then they get to have a full dmesg ring.
> > 
> > Don't let that get in the way of having a sensible time figuring out
> > what went wrong.
> 
> Since it was me who asked to suppress this when invoked through sysctl:
> I can live with an unconditional printk() here, but I think this goes
> against the "what can be done in user space should be done there"
> principle: If enough information is propagated back, user space should
> be able to provide all necessary output without even a need for the
> observer to run "xl dmesg".

But propagating such information to user-space is also not trivial,
and involves more logic in the hypervisor.

I think the point of "what can be done in user space should be done
there" is to avoid adding unnecessary code to Xen, but in this case
printing such detailed information in user-space would require adding
more code to Xen in order to propagate it.

Thanks, Roger.