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

[PATCH v2 0/5] 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/1087447618

For the gitlab CI Alpine build the following series is required for
livepatch-build-tools:

https://lore.kernel.org/xen-devel/20231128092152.35039-1-roger.pau@citrix.com/

Roger Pau Monne (5):
  x86/livepatch: set function alignment to ensure minimal function size
  automation/alpine: add elfutils-dev
  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/build/alpine/3.18.dockerfile       |  2 +
 automation/gitlab-ci/build.yaml               |  8 ++
 automation/gitlab-ci/test.yaml                |  8 ++
 automation/scripts/build                      | 21 +++++
 .../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/arch/x86/Kconfig                          |  6 ++
 xen/arch/x86/Makefile                         |  5 ++
 xen/arch/x86/include/asm/test-smc.h           | 20 +++++
 xen/arch/x86/livepatch.c                      |  4 +
 xen/arch/x86/setup.c                          |  3 +
 xen/arch/x86/sysctl.c                         |  7 ++
 xen/arch/x86/test-smc-lp-alt.c                | 23 ++++++
 xen/arch/x86/test-smc-lp.c                    | 23 ++++++
 xen/arch/x86/test-smc.c                       | 77 +++++++++++++++++++
 xen/common/kernel.c                           |  5 +-
 xen/include/public/sysctl.h                   | 13 ++++
 xen/include/xen/lib.h                         |  1 +
 20 files changed, 337 insertions(+), 2 deletions(-)
 create mode 100755 automation/scripts/qemu-alpine-x86_64-livepatch.sh
 create mode 100644 xen/arch/x86/include/asm/test-smc.h
 create mode 100644 xen/arch/x86/test-smc-lp-alt.c
 create mode 100644 xen/arch/x86/test-smc-lp.c
 create mode 100644 xen/arch/x86/test-smc.c

-- 
2.43.0

[PATCH v2 1/5] x86/livepatch: set function alignment to ensure minimal function size

[Roger Pau Monne]

The minimal function size requirements for livepatch are either 5 bytes (for
jmp) or 9 bytes (for endbr + jmp).  Ensure that functions are always at least
that size by requesting the compiled to align the functions to 8 or 16 bytes,
depending on whether Xen is build with IBT support.

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.

Since the option (-falign-functions) is supported by both minimal required
compiler versions of clang and gcc there's no need to add a test to check for
its presence.

The alignment is currently only implemented for livepatch on x86, I'm unsure
whether ARM has a mandatory function alignment high enough to cover for the
space required by the replacement instruction(s).

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
Changes since v1:
 - New in this version.
---
 xen/arch/x86/Kconfig     | 6 ++++++
 xen/arch/x86/Makefile    | 2 ++
 xen/arch/x86/livepatch.c | 4 ++++
 3 files changed, 12 insertions(+)

diff --git a/xen/arch/x86/Kconfig b/xen/arch/x86/Kconfig
index 1acdffc51c22..612a4acf079b 100644
--- a/xen/arch/x86/Kconfig
+++ b/xen/arch/x86/Kconfig
@@ -47,6 +47,12 @@ config HAS_CC_CET_IBT
 	# Retpoline check to work around https://gcc.gnu.org/bugzilla/show_bug.cgi?id=93654
 	def_bool $(cc-option,-fcf-protection=branch -mmanual-endbr -mindirect-branch=thunk-extern) && $(as-instr,endbr64)
 
+# Set function alignment to ensure enough padding available
+config CC_FUNCTION_ALIGNMENT
+	int
+	default 16 if LIVEPATCH && XEN_IBT
+	default 8 if LIVEPATCH
+
 menu "Architecture Features"
 
 source "arch/Kconfig"
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index f3abdf9cd111..f629157086d0 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -82,6 +82,8 @@ obj-$(CONFIG_COMPAT) += x86_64/platform_hypercall.o
 obj-y += sysctl.o
 endif
 
+CFLAGS-$(CONFIG_LIVEPATCH) += -falign-functions=$(CONFIG_CC_FUNCTION_ALIGNMENT)
+
 extra-y += asm-macros.i
 extra-y += xen.lds
 
diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c
index ee539f001b73..4a6ba09e0ec5 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_CC_FUNCTION_ALIGNMENT);
+
         if ( is_endbr64(func->old_addr) || is_endbr64_poison(func->old_addr) )
             needed += ENDBR64_LEN;
 
-- 
2.43.0

[PATCH v2 2/5] automation/alpine: add elfutils-dev

[Roger Pau Monne]

In preparation for adding some livepatch-build-tools test update the Alpine
container to also install elfutils-dev.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 automation/build/alpine/3.18.dockerfile | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/automation/build/alpine/3.18.dockerfile b/automation/build/alpine/3.18.dockerfile
index 4ae9cb5e9e30..aac2d8cc82d9 100644
--- a/automation/build/alpine/3.18.dockerfile
+++ b/automation/build/alpine/3.18.dockerfile
@@ -47,3 +47,5 @@ RUN apk --no-cache add \
   libcap-ng-dev \
   ninja \
   pixman-dev \
+  # livepatch-tools deps
+  elfutils-dev
-- 
2.43.0

[PATCH v2 3/5] 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 'A').

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.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
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-smc.h | 18 ++++++++
 xen/arch/x86/setup.c                |  3 ++
 xen/arch/x86/sysctl.c               |  7 +++
 xen/arch/x86/test-smc.c             | 68 +++++++++++++++++++++++++++++
 xen/common/kernel.c                 |  5 ++-
 xen/include/public/sysctl.h         |  9 ++++
 xen/include/xen/lib.h               |  1 +
 10 files changed, 126 insertions(+), 2 deletions(-)
 create mode 100644 xen/arch/x86/include/asm/test-smc.h
 create mode 100644 xen/arch/x86/test-smc.c

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 2ef8b4e05422..0f87ffa4affd 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_smc(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..7f7ece589cc2 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_smc(xc_interface *xch, uint32_t tests, uint32_t *result)
+{
+    struct xen_sysctl sysctl = {
+        .cmd = XEN_SYSCTL_test_smc,
+        .u.smc.tests = tests,
+    };
+    int rc = do_sysctl(xch, &sysctl);
+
+    if ( !rc )
+        *result = sysctl.u.smc.results;
+
+    return rc;
+}
+
 /*
  * Local variables:
  * mode: C
diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
index f629157086d0..bdd2183a2fd7 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -65,6 +65,7 @@ obj-y += smpboot.o
 obj-y += spec_ctrl.o
 obj-y += srat.o
 obj-y += string.o
+obj-y += test-smc.o
 obj-y += time.o
 obj-y += traps.o
 obj-y += tsx.o
diff --git a/xen/arch/x86/include/asm/test-smc.h b/xen/arch/x86/include/asm/test-smc.h
new file mode 100644
index 000000000000..18b23dbdbf2d
--- /dev/null
+++ b/xen/arch/x86/include/asm/test-smc.h
@@ -0,0 +1,18 @@
+#ifndef _ASM_X86_TEST_SMC_H_
+#define _ASM_X86_TEST_SMC_H_
+
+#include <xen/types.h>
+
+int test_smc(uint32_t selection, uint32_t *results);
+
+#endif	/* _ASM_X86_TEST_SMC_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 f6b8a3efd752..1f90d30204fe 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-smc.h>
 
 /* opt_nosmp: If true, secondary processors are ignored. */
 static bool __initdata opt_nosmp;
@@ -1952,6 +1953,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
 
     alternative_branches();
 
+    test_smc(XEN_SYSCTL_TEST_SMC_ALL, NULL);
+
     /*
      * 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..77d091f4bd59 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-smc.h>
 #include <asm/numa.h>
 #include <xen/nodemask.h>
 #include <xen/cpu.h>
@@ -423,6 +424,12 @@ long arch_do_sysctl(
         break;
     }
 
+    case XEN_SYSCTL_test_smc:
+        ret = test_smc(sysctl->u.smc.tests, &sysctl->u.smc.results);
+        if ( !ret && __copy_field_to_guest(u_sysctl, sysctl, u.smc.results) )
+            ret = -EFAULT;
+        break;
+
     default:
         ret = -ENOSYS;
         break;
diff --git a/xen/arch/x86/test-smc.c b/xen/arch/x86/test-smc.c
new file mode 100644
index 000000000000..8916c185d60a
--- /dev/null
+++ b/xen/arch/x86/test-smc.c
@@ -0,0 +1,68 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#include <xen/errno.h>
+
+#include <asm/alternative.h>
+#include <asm/cpufeature.h>
+#include <asm/test-smc.h>
+
+static bool cf_check test_insn_replacement(void)
+{
+#define EXPECTED_VALUE 2
+    unsigned int r = ~EXPECTED_VALUE;
+
+    alternative_io("", "mov $" STR(EXPECTED_VALUE) ", %0",
+                   X86_FEATURE_ALWAYS, "=r"(r));
+
+    return r == EXPECTED_VALUE;
+#undef EXPECTED_VALUE
+}
+
+int test_smc(uint32_t selection, uint32_t *results)
+{
+    struct {
+        unsigned int mask;
+        bool (*test)(void);
+        const char *name;
+    } static const tests[] = {
+        { XEN_SYSCTL_TEST_SMC_INSN_REPL, &test_insn_replacement,
+          "alternative instruction replacement" },
+    };
+    unsigned int i;
+
+    if ( selection & ~XEN_SYSCTL_TEST_SMC_ALL )
+        return -EINVAL;
+
+    if ( results )
+        *results = 0;
+
+    printk(XENLOG_INFO "Checking Self Modify Code\n");
+
+    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;
+        }
+
+        add_taint(TAINT_ERROR_SMC);
+        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..fed7ed0d587f 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_SMC ? 'A' : ' ');
     }
     else
     {
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 9b19679caeb1..94287009387c 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_smc {
+    uint32_t tests;     /* IN: bitmap with selected tests to execute. */
+#define XEN_SYSCTL_TEST_SMC_INSN_REPL   (1U << 0)
+#define XEN_SYSCTL_TEST_SMC_ALL         (XEN_SYSCTL_TEST_SMC_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_smc                      31
     uint32_t interface_version; /* XEN_SYSCTL_INTERFACE_VERSION */
     union {
         struct xen_sysctl_readconsole       readconsole;
@@ -1261,6 +1269,7 @@ struct xen_sysctl {
         struct xen_sysctl_livepatch_op      livepatch;
 #if defined(__i386__) || defined(__x86_64__)
         struct xen_sysctl_cpu_policy        cpu_policy;
+        struct xen_sysctl_test_smc          smc;
 #endif
 
 #if defined(__arm__) || defined (__aarch64__)
diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
index 1793be5b6b89..1bec6a01b18a 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_SMC                 (1U << 6)
 extern unsigned int tainted;
 #define TAINT_STRING_MAX_LEN            20
 extern char *print_tainted(char *str);
-- 
2.43.0

[PATCH v2 4/5] 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>
---
Changes since v1:
 - New interface & test.
---
 tools/misc/xen-livepatch.c          | 29 +++++++++++++++++++++++++++++
 xen/arch/x86/Makefile               |  2 ++
 xen/arch/x86/include/asm/test-smc.h |  2 ++
 xen/arch/x86/setup.c                |  2 +-
 xen/arch/x86/test-smc-lp-alt.c      | 23 +++++++++++++++++++++++
 xen/arch/x86/test-smc-lp.c          | 23 +++++++++++++++++++++++
 xen/arch/x86/test-smc.c             | 11 ++++++++++-
 xen/include/public/sysctl.h         |  6 +++++-
 8 files changed, 95 insertions(+), 3 deletions(-)
 create mode 100644 xen/arch/x86/test-smc-lp-alt.c
 create mode 100644 xen/arch/x86/test-smc-lp.c

diff --git a/tools/misc/xen-livepatch.c b/tools/misc/xen-livepatch.c
index 5bf9d9a32b65..fb396f46aaac 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_smc(xch, XEN_SYSCTL_TEST_SMC_LP, &results);
+    if ( rc )
+    {
+        fprintf(stderr, "test operation failed: %s\n", strerror(errno));
+        return -1;
+    }
+    if ( (results & XEN_SYSCTL_TEST_SMC_LP) != XEN_SYSCTL_TEST_SMC_LP )
+    {
+        fprintf(stderr, "some tests failed: %#x (expected %#x)\n",
+                results, XEN_SYSCTL_TEST_SMC_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/Makefile b/xen/arch/x86/Makefile
index bdd2183a2fd7..71cb22e080b8 100644
--- a/xen/arch/x86/Makefile
+++ b/xen/arch/x86/Makefile
@@ -66,6 +66,8 @@ obj-y += spec_ctrl.o
 obj-y += srat.o
 obj-y += string.o
 obj-y += test-smc.o
+obj-$(CONFIG_LIVEPATCH)   += test-smc-lp.o     # for livepatch testing
+extra-$(CONFIG_LIVEPATCH) += test-smc-lp-alt.o
 obj-y += time.o
 obj-y += traps.o
 obj-y += tsx.o
diff --git a/xen/arch/x86/include/asm/test-smc.h b/xen/arch/x86/include/asm/test-smc.h
index 18b23dbdbf2d..6013e4daf7f8 100644
--- a/xen/arch/x86/include/asm/test-smc.h
+++ b/xen/arch/x86/include/asm/test-smc.h
@@ -5,6 +5,8 @@
 
 int test_smc(uint32_t selection, uint32_t *results);
 
+bool cf_check test_lp_insn_replacement(void);
+
 #endif	/* _ASM_X86_TEST_SMC_H_ */
 
 /*
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 1f90d30204fe..8bfb394909b4 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1953,7 +1953,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
 
     alternative_branches();
 
-    test_smc(XEN_SYSCTL_TEST_SMC_ALL, NULL);
+    test_smc(XEN_SYSCTL_TEST_SMC_ALL & ~XEN_SYSCTL_TEST_SMC_LP, NULL);
 
     /*
      * NB: when running as a PV shim VCPUOP_up/down is wired to the shim
diff --git a/xen/arch/x86/test-smc-lp-alt.c b/xen/arch/x86/test-smc-lp-alt.c
new file mode 100644
index 000000000000..7bde547a950d
--- /dev/null
+++ b/xen/arch/x86/test-smc-lp-alt.c
@@ -0,0 +1,23 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#include <asm/test-smc.h>
+
+/*
+ * Interesting case because `return false` can be encoded as an xor
+ * instruction, which is shorter than `return true` which is a mov instruction,
+ * and also shorter than a 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-smc-lp.c b/xen/arch/x86/test-smc-lp.c
new file mode 100644
index 000000000000..0ae776053a42
--- /dev/null
+++ b/xen/arch/x86/test-smc-lp.c
@@ -0,0 +1,23 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#include <asm/test-smc.h>
+
+/*
+ * Interesting case because `return false` can be encoded as an xor
+ * instruction, which is shorter than `return true` which is a mov instruction,
+ * and also shorter than a 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-smc.c b/xen/arch/x86/test-smc.c
index 8916c185d60a..1967016a229f 100644
--- a/xen/arch/x86/test-smc.c
+++ b/xen/arch/x86/test-smc.c
@@ -27,6 +27,10 @@ int test_smc(uint32_t selection, uint32_t *results)
     } static const tests[] = {
         { XEN_SYSCTL_TEST_SMC_INSN_REPL, &test_insn_replacement,
           "alternative instruction replacement" },
+#ifdef CONFIG_LIVEPATCH
+        { XEN_SYSCTL_TEST_SMC_LP_INSN, &test_lp_insn_replacement,
+          "livepatch instruction replacement" },
+#endif
     };
     unsigned int i;
 
@@ -50,7 +54,12 @@ int test_smc(uint32_t selection, uint32_t *results)
             continue;
         }
 
-        add_taint(TAINT_ERROR_SMC);
+        /*
+         * livepatch related tests don't taint the hypervisor because we also
+         * want to check the failing case.
+         */
+        if ( !(tests[i].mask & XEN_SYSCTL_TEST_SMC_LP) )
+            add_taint(TAINT_ERROR_SMC);
         printk(XENLOG_ERR "%s test failed\n", tests[i].name);
     }
 
diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
index 94287009387c..c87878e72a42 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_smc {
     uint32_t tests;     /* IN: bitmap with selected tests to execute. */
 #define XEN_SYSCTL_TEST_SMC_INSN_REPL   (1U << 0)
-#define XEN_SYSCTL_TEST_SMC_ALL         (XEN_SYSCTL_TEST_SMC_INSN_REPL)
+#define XEN_SYSCTL_TEST_SMC_LP_INSN     (1U << 1)
+#define XEN_SYSCTL_TEST_SMC_ALL         (XEN_SYSCTL_TEST_SMC_INSN_REPL | \
+                                         XEN_SYSCTL_TEST_SMC_LP_INSN)
+#define XEN_SYSCTL_TEST_SMC_LP          (XEN_SYSCTL_TEST_SMC_LP_INSN)
+
     uint32_t results;   /* OUT: test result: 1 -> success, 0 -> failure. */
 };
 
-- 
2.43.0

[PATCH v2 5/5] 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>
---
 automation/gitlab-ci/build.yaml               |  8 +++
 automation/gitlab-ci/test.yaml                |  8 +++
 automation/scripts/build                      | 21 ++++++
 .../scripts/qemu-alpine-x86_64-livepatch.sh   | 68 +++++++++++++++++++
 4 files changed, 105 insertions(+)
 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..22026df51b87 100644
--- a/automation/gitlab-ci/build.yaml
+++ b/automation/gitlab-ci/build.yaml
@@ -358,6 +358,14 @@ alpine-3.18-gcc-debug:
   variables:
     CONTAINER: alpine:3.18
 
+alpine-3.18-gcc-livepatch:
+  extends: .gcc-x86-64-build
+  variables:
+    CONTAINER: alpine:3.18
+    LIVEPATCH: y
+    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 b/automation/scripts/build
index b3c71fb6fb60..0a0a6dceb08c 100755
--- a/automation/scripts/build
+++ b/automation/scripts/build
@@ -103,3 +103,24 @@ else
     cp -r dist binaries/
     if [[ -f xen/xen ]] ; then cp xen/xen binaries/xen; fi
 fi
+
+if [[ "$LIVEPATCH" == "y" ]]; then
+    # Build a test livepatch using livepatch-build-tools.
+
+    if [[ "$XEN_TARGET_ARCH" != "x86_64" ]]; then
+        exit 1
+    fi
+
+    # git diff --no-index returns 0 if no differences, otherwise 1.
+    git diff --no-index --output=test.patch xen/arch/x86/test-smc-lp.c \
+                                            xen/arch/x86/test-smc-lp-alt.c && 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
+fi
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 v2 2/5] automation/alpine: add elfutils-dev

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 769 bytes --]

On Tue, 28 Nov 2023, Roger Pau Monne wrote:
> In preparation for adding some livepatch-build-tools test update the Alpine
> container to also install elfutils-dev.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

Acked-by: Stefano Stabellini <sstabellini@kernel.org>

> ---
>  automation/build/alpine/3.18.dockerfile | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/automation/build/alpine/3.18.dockerfile b/automation/build/alpine/3.18.dockerfile
> index 4ae9cb5e9e30..aac2d8cc82d9 100644
> --- a/automation/build/alpine/3.18.dockerfile
> +++ b/automation/build/alpine/3.18.dockerfile
> @@ -47,3 +47,5 @@ RUN apk --no-cache add \
>    libcap-ng-dev \
>    ninja \
>    pixman-dev \
> +  # livepatch-tools deps
> +  elfutils-dev
> -- 
> 2.43.0
> 

Re: [PATCH v2 3/5] xen/x86: introduce self modifying code test

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 10149 bytes --]

On Tue, 28 Nov 2023, 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 'A').
> 
> 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.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
> 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-smc.h | 18 ++++++++
>  xen/arch/x86/setup.c                |  3 ++
>  xen/arch/x86/sysctl.c               |  7 +++
>  xen/arch/x86/test-smc.c             | 68 +++++++++++++++++++++++++++++

If possible, could we name this differently?

I am asking because of these:
https://documentation-service.arm.com/static/5f8ea482f86e16515cdbe3c6?token=
https://gitlab.com/xen-project/xen/-/blob/staging/xen/arch/arm/vsmc.c?ref_type=heads



>  xen/common/kernel.c                 |  5 ++-
>  xen/include/public/sysctl.h         |  9 ++++
>  xen/include/xen/lib.h               |  1 +
>  10 files changed, 126 insertions(+), 2 deletions(-)
>  create mode 100644 xen/arch/x86/include/asm/test-smc.h
>  create mode 100644 xen/arch/x86/test-smc.c
> 
> diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
> index 2ef8b4e05422..0f87ffa4affd 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_smc(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..7f7ece589cc2 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_smc(xc_interface *xch, uint32_t tests, uint32_t *result)
> +{
> +    struct xen_sysctl sysctl = {
> +        .cmd = XEN_SYSCTL_test_smc,
> +        .u.smc.tests = tests,
> +    };
> +    int rc = do_sysctl(xch, &sysctl);
> +
> +    if ( !rc )
> +        *result = sysctl.u.smc.results;
> +
> +    return rc;
> +}
> +
>  /*
>   * Local variables:
>   * mode: C
> diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> index f629157086d0..bdd2183a2fd7 100644
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -65,6 +65,7 @@ obj-y += smpboot.o
>  obj-y += spec_ctrl.o
>  obj-y += srat.o
>  obj-y += string.o
> +obj-y += test-smc.o
>  obj-y += time.o
>  obj-y += traps.o
>  obj-y += tsx.o
> diff --git a/xen/arch/x86/include/asm/test-smc.h b/xen/arch/x86/include/asm/test-smc.h
> new file mode 100644
> index 000000000000..18b23dbdbf2d
> --- /dev/null
> +++ b/xen/arch/x86/include/asm/test-smc.h
> @@ -0,0 +1,18 @@
> +#ifndef _ASM_X86_TEST_SMC_H_
> +#define _ASM_X86_TEST_SMC_H_
> +
> +#include <xen/types.h>
> +
> +int test_smc(uint32_t selection, uint32_t *results);
> +
> +#endif	/* _ASM_X86_TEST_SMC_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 f6b8a3efd752..1f90d30204fe 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-smc.h>
>  
>  /* opt_nosmp: If true, secondary processors are ignored. */
>  static bool __initdata opt_nosmp;
> @@ -1952,6 +1953,8 @@ void __init noreturn __start_xen(unsigned long mbi_p)
>  
>      alternative_branches();
>  
> +    test_smc(XEN_SYSCTL_TEST_SMC_ALL, NULL);
> +
>      /*
>       * 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..77d091f4bd59 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-smc.h>
>  #include <asm/numa.h>
>  #include <xen/nodemask.h>
>  #include <xen/cpu.h>
> @@ -423,6 +424,12 @@ long arch_do_sysctl(
>          break;
>      }
>  
> +    case XEN_SYSCTL_test_smc:
> +        ret = test_smc(sysctl->u.smc.tests, &sysctl->u.smc.results);
> +        if ( !ret && __copy_field_to_guest(u_sysctl, sysctl, u.smc.results) )
> +            ret = -EFAULT;
> +        break;
> +
>      default:
>          ret = -ENOSYS;
>          break;
> diff --git a/xen/arch/x86/test-smc.c b/xen/arch/x86/test-smc.c
> new file mode 100644
> index 000000000000..8916c185d60a
> --- /dev/null
> +++ b/xen/arch/x86/test-smc.c
> @@ -0,0 +1,68 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#include <xen/errno.h>
> +
> +#include <asm/alternative.h>
> +#include <asm/cpufeature.h>
> +#include <asm/test-smc.h>
> +
> +static bool cf_check test_insn_replacement(void)
> +{
> +#define EXPECTED_VALUE 2
> +    unsigned int r = ~EXPECTED_VALUE;
> +
> +    alternative_io("", "mov $" STR(EXPECTED_VALUE) ", %0",
> +                   X86_FEATURE_ALWAYS, "=r"(r));
> +
> +    return r == EXPECTED_VALUE;
> +#undef EXPECTED_VALUE
> +}
> +
> +int test_smc(uint32_t selection, uint32_t *results)
> +{
> +    struct {
> +        unsigned int mask;
> +        bool (*test)(void);
> +        const char *name;
> +    } static const tests[] = {
> +        { XEN_SYSCTL_TEST_SMC_INSN_REPL, &test_insn_replacement,
> +          "alternative instruction replacement" },
> +    };
> +    unsigned int i;
> +
> +    if ( selection & ~XEN_SYSCTL_TEST_SMC_ALL )
> +        return -EINVAL;
> +
> +    if ( results )
> +        *results = 0;
> +
> +    printk(XENLOG_INFO "Checking Self Modify Code\n");
> +
> +    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;
> +        }
> +
> +        add_taint(TAINT_ERROR_SMC);
> +        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..fed7ed0d587f 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_SMC ? 'A' : ' ');
>      }
>      else
>      {
> diff --git a/xen/include/public/sysctl.h b/xen/include/public/sysctl.h
> index 9b19679caeb1..94287009387c 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_smc {
> +    uint32_t tests;     /* IN: bitmap with selected tests to execute. */
> +#define XEN_SYSCTL_TEST_SMC_INSN_REPL   (1U << 0)
> +#define XEN_SYSCTL_TEST_SMC_ALL         (XEN_SYSCTL_TEST_SMC_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_smc                      31
>      uint32_t interface_version; /* XEN_SYSCTL_INTERFACE_VERSION */
>      union {
>          struct xen_sysctl_readconsole       readconsole;
> @@ -1261,6 +1269,7 @@ struct xen_sysctl {
>          struct xen_sysctl_livepatch_op      livepatch;
>  #if defined(__i386__) || defined(__x86_64__)
>          struct xen_sysctl_cpu_policy        cpu_policy;
> +        struct xen_sysctl_test_smc          smc;
>  #endif
>  
>  #if defined(__arm__) || defined (__aarch64__)
> diff --git a/xen/include/xen/lib.h b/xen/include/xen/lib.h
> index 1793be5b6b89..1bec6a01b18a 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_SMC                 (1U << 6)
>  extern unsigned int tainted;
>  #define TAINT_STRING_MAX_LEN            20
>  extern char *print_tainted(char *str);
> -- 
> 2.43.0
> 

Re: [PATCH v2 5/5] automation: add x86-64 livepatching test

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 5194 bytes --]

On Tue, 28 Nov 2023, Roger Pau Monne wrote:
> 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>
> ---
>  automation/gitlab-ci/build.yaml               |  8 +++
>  automation/gitlab-ci/test.yaml                |  8 +++
>  automation/scripts/build                      | 21 ++++++
>  .../scripts/qemu-alpine-x86_64-livepatch.sh   | 68 +++++++++++++++++++
>  4 files changed, 105 insertions(+)
>  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..22026df51b87 100644
> --- a/automation/gitlab-ci/build.yaml
> +++ b/automation/gitlab-ci/build.yaml
> @@ -358,6 +358,14 @@ alpine-3.18-gcc-debug:
>    variables:
>      CONTAINER: alpine:3.18
>  
> +alpine-3.18-gcc-livepatch:
> +  extends: .gcc-x86-64-build
> +  variables:
> +    CONTAINER: alpine:3.18
> +    LIVEPATCH: y
> +    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 b/automation/scripts/build
> index b3c71fb6fb60..0a0a6dceb08c 100755
> --- a/automation/scripts/build
> +++ b/automation/scripts/build
> @@ -103,3 +103,24 @@ else
>      cp -r dist binaries/
>      if [[ -f xen/xen ]] ; then cp xen/xen binaries/xen; fi
>  fi
> +
> +if [[ "$LIVEPATCH" == "y" ]]; then
> +    # Build a test livepatch using livepatch-build-tools.
> +
> +    if [[ "$XEN_TARGET_ARCH" != "x86_64" ]]; then
> +        exit 1
> +    fi
> +
> +    # git diff --no-index returns 0 if no differences, otherwise 1.
> +    git diff --no-index --output=test.patch xen/arch/x86/test-smc-lp.c \
> +                                            xen/arch/x86/test-smc-lp-alt.c && 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
> +fi

I realize this is a matter of taste but if possible I would move this to
qemu-alpine-x86_64-livepatch.sh not to make the build script too
complex.

Otherwise, plase create automation/scripts/livepatch and move this code
there. You can call automation/scripts/livepatch from
automation/scripts/build.

Other than that, this is great! I'll let other review the livepatch
specific changes in this series



> 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 v2 1/5] x86/livepatch: set function alignment to ensure minimal function size

[Jan Beulich]

On 28.11.2023 11:03, Roger Pau Monne wrote:
> The minimal function size requirements for livepatch are either 5 bytes (for
> jmp) or 9 bytes (for endbr + jmp).  Ensure that functions are always at least
> that size by requesting the compiled to align the functions to 8 or 16 bytes,
> depending on whether Xen is build with IBT support.

How is alignment going to enforce minimum function size? If a function is
last in a section, there may not be any padding added (ahead of linking at
least). The trailing padding also isn't part of the function.

> 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.
> 
> Since the option (-falign-functions) is supported by both minimal required
> compiler versions of clang and gcc there's no need to add a test to check for
> its presence.
> 
> The alignment is currently only implemented for livepatch on x86, I'm unsure
> whether ARM has a mandatory function alignment high enough to cover for the
> space required by the replacement instruction(s).

Indeed I was wondering whether this shouldn't be generalized, if indeed
required.

Jan

Re: [PATCH v2 3/5] xen/x86: introduce self modifying code test

[Jan Beulich]

On 28.11.2023 11:03, Roger Pau Monne wrote:
> --- /dev/null
> +++ b/xen/arch/x86/test-smc.c
> @@ -0,0 +1,68 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#include <xen/errno.h>
> +
> +#include <asm/alternative.h>
> +#include <asm/cpufeature.h>
> +#include <asm/test-smc.h>
> +
> +static bool cf_check test_insn_replacement(void)
> +{
> +#define EXPECTED_VALUE 2
> +    unsigned int r = ~EXPECTED_VALUE;

The compiler is permitted to elide the initializer unless ...

> +    alternative_io("", "mov $" STR(EXPECTED_VALUE) ", %0",
> +                   X86_FEATURE_ALWAYS, "=r"(r));

... you use "+r" here. Also (nit) there's a blank missing between that
string and the opening parethesis. Also what's wrong with passing
EXPECTED_VALUE in as an "i" constraint input operand?

> +    return r == EXPECTED_VALUE;
> +#undef EXPECTED_VALUE
> +}
> +
> +int test_smc(uint32_t selection, uint32_t *results)
> +{
> +    struct {
> +        unsigned int mask;
> +        bool (*test)(void);
> +        const char *name;
> +    } static const tests[] = {
> +        { XEN_SYSCTL_TEST_SMC_INSN_REPL, &test_insn_replacement,
> +          "alternative instruction replacement" },
> +    };
> +    unsigned int i;
> +
> +    if ( selection & ~XEN_SYSCTL_TEST_SMC_ALL )
> +        return -EINVAL;
> +
> +    if ( results )
> +        *results = 0;
> +
> +    printk(XENLOG_INFO "Checking Self Modify Code\n");
> +
> +    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;
> +        }
> +
> +        add_taint(TAINT_ERROR_SMC);
> +        printk(XENLOG_ERR "%s test failed\n", tests[i].name);
> +    }
> +
> +    return 0;
> +}

I think I need to look at the next patch to make sense of this.

> @@ -1261,6 +1269,7 @@ struct xen_sysctl {
>          struct xen_sysctl_livepatch_op      livepatch;
>  #if defined(__i386__) || defined(__x86_64__)
>          struct xen_sysctl_cpu_policy        cpu_policy;
> +        struct xen_sysctl_test_smc          smc;

Imo the field name would better be test_smc (leaving aside Stefano's comment).

Jan

Re: [PATCH v2 1/5] x86/livepatch: set function alignment to ensure minimal function size

[Roger Pau Monné]

On Thu, Nov 30, 2023 at 05:55:07PM +0100, Jan Beulich wrote:
> On 28.11.2023 11:03, Roger Pau Monne wrote:
> > The minimal function size requirements for livepatch are either 5 bytes (for
> > jmp) or 9 bytes (for endbr + jmp).  Ensure that functions are always at least
> > that size by requesting the compiled to align the functions to 8 or 16 bytes,
> > depending on whether Xen is build with IBT support.
> 
> How is alignment going to enforce minimum function size? If a function is
> last in a section, there may not be any padding added (ahead of linking at
> least). The trailing padding also isn't part of the function.

If each function lives in it's own section (by using
-ffunction-sections), and each section is aligned, then I think we can
guarantee that there will always be enough padding space?

Even the last function/section on the .text block would still be
aligned, and as long as the function alignment <= SECTION_ALIGN
there will be enough padding left.  I should add some build time
assert that CONFIG_CC_FUNCTION_ALIGNMENT <= SECTION_ALIGN.

While the trailing padding is not part of the function itself, I don't
see issues in overwriting it with the replacement function code.

> > 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.
> > 
> > Since the option (-falign-functions) is supported by both minimal required
> > compiler versions of clang and gcc there's no need to add a test to check for
> > its presence.
> > 
> > The alignment is currently only implemented for livepatch on x86, I'm unsure
> > whether ARM has a mandatory function alignment high enough to cover for the
> > space required by the replacement instruction(s).
> 
> Indeed I was wondering whether this shouldn't be generalized, if indeed
> required.

Well, more than required it's nice to have in order to avoid failures.
The checks in the livepatch code will trigger if there's not enough
space to apply the replacement code.

Thanks, Roger.

Re: [PATCH v2 1/5] x86/livepatch: set function alignment to ensure minimal function size

[Jan Beulich]

On 30.11.2023 18:37, Roger Pau Monné wrote:
> On Thu, Nov 30, 2023 at 05:55:07PM +0100, Jan Beulich wrote:
>> On 28.11.2023 11:03, Roger Pau Monne wrote:
>>> The minimal function size requirements for livepatch are either 5 bytes (for
>>> jmp) or 9 bytes (for endbr + jmp).  Ensure that functions are always at least
>>> that size by requesting the compiled to align the functions to 8 or 16 bytes,
>>> depending on whether Xen is build with IBT support.
>>
>> How is alignment going to enforce minimum function size? If a function is
>> last in a section, there may not be any padding added (ahead of linking at
>> least). The trailing padding also isn't part of the function.
> 
> If each function lives in it's own section (by using
> -ffunction-sections), and each section is aligned, then I think we can
> guarantee that there will always be enough padding space?
> 
> Even the last function/section on the .text block would still be
> aligned, and as long as the function alignment <= SECTION_ALIGN
> there will be enough padding left.  I should add some build time
> assert that CONFIG_CC_FUNCTION_ALIGNMENT <= SECTION_ALIGN.

I'm not sure of there being a requirement for a section to be padded to
its alignment. If the following section has smaller alignment, it could
be made start earlier. Of course our linker scripts might guarantee
this ...

> While the trailing padding is not part of the function itself, I don't
> see issues in overwriting it with the replacement function code.

If it's really padding, yes.

Jan

Re: [PATCH v2 1/5] x86/livepatch: set function alignment to ensure minimal function size

[Roger Pau Monné]

On Fri, Dec 01, 2023 at 07:53:29AM +0100, Jan Beulich wrote:
> On 30.11.2023 18:37, Roger Pau Monné wrote:
> > On Thu, Nov 30, 2023 at 05:55:07PM +0100, Jan Beulich wrote:
> >> On 28.11.2023 11:03, Roger Pau Monne wrote:
> >>> The minimal function size requirements for livepatch are either 5 bytes (for
> >>> jmp) or 9 bytes (for endbr + jmp).  Ensure that functions are always at least
> >>> that size by requesting the compiled to align the functions to 8 or 16 bytes,
> >>> depending on whether Xen is build with IBT support.
> >>
> >> How is alignment going to enforce minimum function size? If a function is
> >> last in a section, there may not be any padding added (ahead of linking at
> >> least). The trailing padding also isn't part of the function.
> > 
> > If each function lives in it's own section (by using
> > -ffunction-sections), and each section is aligned, then I think we can
> > guarantee that there will always be enough padding space?
> > 
> > Even the last function/section on the .text block would still be
> > aligned, and as long as the function alignment <= SECTION_ALIGN
> > there will be enough padding left.  I should add some build time
> > assert that CONFIG_CC_FUNCTION_ALIGNMENT <= SECTION_ALIGN.
> 
> I'm not sure of there being a requirement for a section to be padded to
> its alignment. If the following section has smaller alignment, it could
> be made start earlier. Of course our linker scripts might guarantee
> this ...

I do think so, given our linker script arrangements for the .text
section:

DECL_SECTION(.text) {
    [...]
} PHDR(text) = 0x9090

. = ALIGN(SECTION_ALIGN);

The end of the text section is aligned to SECTION_ALIGN, so as long as
SECTION_ALIGN >= CONFIG_CC_FUNCTION_ALIGNMENT the alignment should
guarantee a minimal function size.

Do you think it would be clearer if I add the following paragraph:

"Given the Xen linker script arrangement of the .text section, we can
ensure that when all functions are aligned to the given boundary the
function size will always be a multiple of such alignment, even for
the last function in .text, as the linker script aligns the end of the
section to SECTION_ALIGN."

Thanks, Roger.

Re: [PATCH v2 1/5] x86/livepatch: set function alignment to ensure minimal function size

[Jan Beulich]

On 01.12.2023 09:50, Roger Pau Monné wrote:
> On Fri, Dec 01, 2023 at 07:53:29AM +0100, Jan Beulich wrote:
>> On 30.11.2023 18:37, Roger Pau Monné wrote:
>>> On Thu, Nov 30, 2023 at 05:55:07PM +0100, Jan Beulich wrote:
>>>> On 28.11.2023 11:03, Roger Pau Monne wrote:
>>>>> The minimal function size requirements for livepatch are either 5 bytes (for
>>>>> jmp) or 9 bytes (for endbr + jmp).  Ensure that functions are always at least
>>>>> that size by requesting the compiled to align the functions to 8 or 16 bytes,
>>>>> depending on whether Xen is build with IBT support.
>>>>
>>>> How is alignment going to enforce minimum function size? If a function is
>>>> last in a section, there may not be any padding added (ahead of linking at
>>>> least). The trailing padding also isn't part of the function.
>>>
>>> If each function lives in it's own section (by using
>>> -ffunction-sections), and each section is aligned, then I think we can
>>> guarantee that there will always be enough padding space?
>>>
>>> Even the last function/section on the .text block would still be
>>> aligned, and as long as the function alignment <= SECTION_ALIGN
>>> there will be enough padding left.  I should add some build time
>>> assert that CONFIG_CC_FUNCTION_ALIGNMENT <= SECTION_ALIGN.
>>
>> I'm not sure of there being a requirement for a section to be padded to
>> its alignment. If the following section has smaller alignment, it could
>> be made start earlier. Of course our linker scripts might guarantee
>> this ...
> 
> I do think so, given our linker script arrangements for the .text
> section:
> 
> DECL_SECTION(.text) {
>     [...]
> } PHDR(text) = 0x9090
> 
> . = ALIGN(SECTION_ALIGN);
> 
> The end of the text section is aligned to SECTION_ALIGN, so as long as
> SECTION_ALIGN >= CONFIG_CC_FUNCTION_ALIGNMENT the alignment should
> guarantee a minimal function size.
> 
> Do you think it would be clearer if I add the following paragraph:
> 
> "Given the Xen linker script arrangement of the .text section, we can
> ensure that when all functions are aligned to the given boundary the
> function size will always be a multiple of such alignment, even for
> the last function in .text, as the linker script aligns the end of the
> section to SECTION_ALIGN."

I think this would be useful to have there. Beyond that, assembly code
also needs considering btw.

Jan

Re: [PATCH v2 1/5] x86/livepatch: set function alignment to ensure minimal function size

[Roger Pau Monné]

On Fri, Dec 01, 2023 at 10:41:45AM +0100, Jan Beulich wrote:
> On 01.12.2023 09:50, Roger Pau Monné wrote:
> > On Fri, Dec 01, 2023 at 07:53:29AM +0100, Jan Beulich wrote:
> >> On 30.11.2023 18:37, Roger Pau Monné wrote:
> >>> On Thu, Nov 30, 2023 at 05:55:07PM +0100, Jan Beulich wrote:
> >>>> On 28.11.2023 11:03, Roger Pau Monne wrote:
> >>>>> The minimal function size requirements for livepatch are either 5 bytes (for
> >>>>> jmp) or 9 bytes (for endbr + jmp).  Ensure that functions are always at least
> >>>>> that size by requesting the compiled to align the functions to 8 or 16 bytes,
> >>>>> depending on whether Xen is build with IBT support.
> >>>>
> >>>> How is alignment going to enforce minimum function size? If a function is
> >>>> last in a section, there may not be any padding added (ahead of linking at
> >>>> least). The trailing padding also isn't part of the function.
> >>>
> >>> If each function lives in it's own section (by using
> >>> -ffunction-sections), and each section is aligned, then I think we can
> >>> guarantee that there will always be enough padding space?
> >>>
> >>> Even the last function/section on the .text block would still be
> >>> aligned, and as long as the function alignment <= SECTION_ALIGN
> >>> there will be enough padding left.  I should add some build time
> >>> assert that CONFIG_CC_FUNCTION_ALIGNMENT <= SECTION_ALIGN.
> >>
> >> I'm not sure of there being a requirement for a section to be padded to
> >> its alignment. If the following section has smaller alignment, it could
> >> be made start earlier. Of course our linker scripts might guarantee
> >> this ...
> > 
> > I do think so, given our linker script arrangements for the .text
> > section:
> > 
> > DECL_SECTION(.text) {
> >     [...]
> > } PHDR(text) = 0x9090
> > 
> > . = ALIGN(SECTION_ALIGN);
> > 
> > The end of the text section is aligned to SECTION_ALIGN, so as long as
> > SECTION_ALIGN >= CONFIG_CC_FUNCTION_ALIGNMENT the alignment should
> > guarantee a minimal function size.
> > 
> > Do you think it would be clearer if I add the following paragraph:
> > 
> > "Given the Xen linker script arrangement of the .text section, we can
> > ensure that when all functions are aligned to the given boundary the
> > function size will always be a multiple of such alignment, even for
> > the last function in .text, as the linker script aligns the end of the
> > section to SECTION_ALIGN."
> 
> I think this would be useful to have there. Beyond that, assembly code
> also needs considering btw.

Assembly will get dealt with once we start to also have separate
sections for each assembly function.  We cannot patch assembly code at
the moment anyway, due to lack of debug symbols.

Thanks, Roger.

Re: [PATCH v2 1/5] x86/livepatch: set function alignment to ensure minimal function size

[Jan Beulich]

On 01.12.2023 11:21, Roger Pau Monné wrote:
> On Fri, Dec 01, 2023 at 10:41:45AM +0100, Jan Beulich wrote:
>> On 01.12.2023 09:50, Roger Pau Monné wrote:
>>> On Fri, Dec 01, 2023 at 07:53:29AM +0100, Jan Beulich wrote:
>>>> On 30.11.2023 18:37, Roger Pau Monné wrote:
>>>>> On Thu, Nov 30, 2023 at 05:55:07PM +0100, Jan Beulich wrote:
>>>>>> On 28.11.2023 11:03, Roger Pau Monne wrote:
>>>>>>> The minimal function size requirements for livepatch are either 5 bytes (for
>>>>>>> jmp) or 9 bytes (for endbr + jmp).  Ensure that functions are always at least
>>>>>>> that size by requesting the compiled to align the functions to 8 or 16 bytes,
>>>>>>> depending on whether Xen is build with IBT support.
>>>>>>
>>>>>> How is alignment going to enforce minimum function size? If a function is
>>>>>> last in a section, there may not be any padding added (ahead of linking at
>>>>>> least). The trailing padding also isn't part of the function.
>>>>>
>>>>> If each function lives in it's own section (by using
>>>>> -ffunction-sections), and each section is aligned, then I think we can
>>>>> guarantee that there will always be enough padding space?
>>>>>
>>>>> Even the last function/section on the .text block would still be
>>>>> aligned, and as long as the function alignment <= SECTION_ALIGN
>>>>> there will be enough padding left.  I should add some build time
>>>>> assert that CONFIG_CC_FUNCTION_ALIGNMENT <= SECTION_ALIGN.
>>>>
>>>> I'm not sure of there being a requirement for a section to be padded to
>>>> its alignment. If the following section has smaller alignment, it could
>>>> be made start earlier. Of course our linker scripts might guarantee
>>>> this ...
>>>
>>> I do think so, given our linker script arrangements for the .text
>>> section:
>>>
>>> DECL_SECTION(.text) {
>>>     [...]
>>> } PHDR(text) = 0x9090
>>>
>>> . = ALIGN(SECTION_ALIGN);
>>>
>>> The end of the text section is aligned to SECTION_ALIGN, so as long as
>>> SECTION_ALIGN >= CONFIG_CC_FUNCTION_ALIGNMENT the alignment should
>>> guarantee a minimal function size.
>>>
>>> Do you think it would be clearer if I add the following paragraph:
>>>
>>> "Given the Xen linker script arrangement of the .text section, we can
>>> ensure that when all functions are aligned to the given boundary the
>>> function size will always be a multiple of such alignment, even for
>>> the last function in .text, as the linker script aligns the end of the
>>> section to SECTION_ALIGN."
>>
>> I think this would be useful to have there. Beyond that, assembly code
>> also needs considering btw.
> 
> Assembly will get dealt with once we start to also have separate
> sections for each assembly function.  We cannot patch assembly code at
> the moment anyway, due to lack of debug symbols.

Well, yes, that's one part of it. The other is that some .text coming
from an assembly source may follow one coming from some C source, and
if the assembly one then isn't properly aligned, padding space again
wouldn't necessarily be large enough. This may be alright now (where
.text is the only thing that can come from .S and would be linked
ahead of all .text.*, being the only thing that can come from .c), but
it might subtly when assembly code is also switched to per-function
sections (you may recall that a patch to this effect is already
pending: "common: honor CONFIG_CC_SPLIT_SECTIONS also for assembly
functions").

Jan

Re: [PATCH v2 1/5] x86/livepatch: set function alignment to ensure minimal function size

[Roger Pau Monné]

On Fri, Dec 01, 2023 at 11:59:09AM +0100, Jan Beulich wrote:
> On 01.12.2023 11:21, Roger Pau Monné wrote:
> > On Fri, Dec 01, 2023 at 10:41:45AM +0100, Jan Beulich wrote:
> >> On 01.12.2023 09:50, Roger Pau Monné wrote:
> >>> On Fri, Dec 01, 2023 at 07:53:29AM +0100, Jan Beulich wrote:
> >>>> On 30.11.2023 18:37, Roger Pau Monné wrote:
> >>>>> On Thu, Nov 30, 2023 at 05:55:07PM +0100, Jan Beulich wrote:
> >>>>>> On 28.11.2023 11:03, Roger Pau Monne wrote:
> >>>>>>> The minimal function size requirements for livepatch are either 5 bytes (for
> >>>>>>> jmp) or 9 bytes (for endbr + jmp).  Ensure that functions are always at least
> >>>>>>> that size by requesting the compiled to align the functions to 8 or 16 bytes,
> >>>>>>> depending on whether Xen is build with IBT support.
> >>>>>>
> >>>>>> How is alignment going to enforce minimum function size? If a function is
> >>>>>> last in a section, there may not be any padding added (ahead of linking at
> >>>>>> least). The trailing padding also isn't part of the function.
> >>>>>
> >>>>> If each function lives in it's own section (by using
> >>>>> -ffunction-sections), and each section is aligned, then I think we can
> >>>>> guarantee that there will always be enough padding space?
> >>>>>
> >>>>> Even the last function/section on the .text block would still be
> >>>>> aligned, and as long as the function alignment <= SECTION_ALIGN
> >>>>> there will be enough padding left.  I should add some build time
> >>>>> assert that CONFIG_CC_FUNCTION_ALIGNMENT <= SECTION_ALIGN.
> >>>>
> >>>> I'm not sure of there being a requirement for a section to be padded to
> >>>> its alignment. If the following section has smaller alignment, it could
> >>>> be made start earlier. Of course our linker scripts might guarantee
> >>>> this ...
> >>>
> >>> I do think so, given our linker script arrangements for the .text
> >>> section:
> >>>
> >>> DECL_SECTION(.text) {
> >>>     [...]
> >>> } PHDR(text) = 0x9090
> >>>
> >>> . = ALIGN(SECTION_ALIGN);
> >>>
> >>> The end of the text section is aligned to SECTION_ALIGN, so as long as
> >>> SECTION_ALIGN >= CONFIG_CC_FUNCTION_ALIGNMENT the alignment should
> >>> guarantee a minimal function size.
> >>>
> >>> Do you think it would be clearer if I add the following paragraph:
> >>>
> >>> "Given the Xen linker script arrangement of the .text section, we can
> >>> ensure that when all functions are aligned to the given boundary the
> >>> function size will always be a multiple of such alignment, even for
> >>> the last function in .text, as the linker script aligns the end of the
> >>> section to SECTION_ALIGN."
> >>
> >> I think this would be useful to have there. Beyond that, assembly code
> >> also needs considering btw.
> > 
> > Assembly will get dealt with once we start to also have separate
> > sections for each assembly function.  We cannot patch assembly code at
> > the moment anyway, due to lack of debug symbols.
> 
> Well, yes, that's one part of it. The other is that some .text coming
> from an assembly source may follow one coming from some C source, and
> if the assembly one then isn't properly aligned, padding space again
> wouldn't necessarily be large enough. This may be alright now (where
> .text is the only thing that can come from .S and would be linked
> ahead of all .text.*, being the only thing that can come from .c),

What about adding:

#ifdef CONFIG_CC_SPLIT_SECTIONS
       *(.text.*)
#endif
#ifdef CONFIG_CC_FUNCTION_ALIGNMENT
       /* Ensure enough padding regardless of next section alignment. */
       . = ALIGN(CONFIG_CC_FUNCTION_ALIGNMENT)
#endif

In order to assert that the end of .text.* is also aligned?

> but
> it might subtly when assembly code is also switched to per-function
> sections (you may recall that a patch to this effect is already
> pending: "common: honor CONFIG_CC_SPLIT_SECTIONS also for assembly
> functions").

Yes, I think such patch should also honor the required alignment
specified in CONFIG_CC_FUNCTION_ALIGNMENT.

Thanks, Roger.

Re: [PATCH v2 3/5] xen/x86: introduce self modifying code test

[Roger Pau Monné]

On Wed, Nov 29, 2023 at 06:58:38PM -0800, Stefano Stabellini wrote:
> On Tue, 28 Nov 2023, 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 'A').
> > 
> > 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.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> > 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-smc.h | 18 ++++++++
> >  xen/arch/x86/setup.c                |  3 ++
> >  xen/arch/x86/sysctl.c               |  7 +++
> >  xen/arch/x86/test-smc.c             | 68 +++++++++++++++++++++++++++++
> 
> If possible, could we name this differently?

Wikipedia also suggests 'smoc' as an alternative acronym for
self-modifying code, would test-smoc be OK?

Thanks, Roger.

Re: [PATCH v2 3/5] xen/x86: introduce self modifying code test

[Roger Pau Monné]

On Thu, Nov 30, 2023 at 06:02:55PM +0100, Jan Beulich wrote:
> On 28.11.2023 11:03, Roger Pau Monne wrote:
> > --- /dev/null
> > +++ b/xen/arch/x86/test-smc.c
> > @@ -0,0 +1,68 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +
> > +#include <xen/errno.h>
> > +
> > +#include <asm/alternative.h>
> > +#include <asm/cpufeature.h>
> > +#include <asm/test-smc.h>
> > +
> > +static bool cf_check test_insn_replacement(void)
> > +{
> > +#define EXPECTED_VALUE 2
> > +    unsigned int r = ~EXPECTED_VALUE;
> 
> The compiler is permitted to elide the initializer unless ...
> 
> > +    alternative_io("", "mov $" STR(EXPECTED_VALUE) ", %0",
> > +                   X86_FEATURE_ALWAYS, "=r"(r));
> 
> ... you use "+r" here.

I see, '=' assumes the operand is always written to, which is not the
case if alternative is not applied.

> Also (nit) there's a blank missing between that
> string and the opening parethesis. Also what's wrong with passing
> EXPECTED_VALUE in as an "i" constraint input operand?

Me not knowing enough inline assembly I think, that's what's wrong.

> > @@ -1261,6 +1269,7 @@ struct xen_sysctl {
> >          struct xen_sysctl_livepatch_op      livepatch;
> >  #if defined(__i386__) || defined(__x86_64__)
> >          struct xen_sysctl_cpu_policy        cpu_policy;
> > +        struct xen_sysctl_test_smc          smc;
> 
> Imo the field name would better be test_smc (leaving aside Stefano's comment).

Right, will see what Stefano thinks about using test_smoc.

Thanks, Roger.

Re: [PATCH v2 1/5] x86/livepatch: set function alignment to ensure minimal function size

[Jan Beulich]

On 01.12.2023 12:31, Roger Pau Monné wrote:
> On Fri, Dec 01, 2023 at 11:59:09AM +0100, Jan Beulich wrote:
>> On 01.12.2023 11:21, Roger Pau Monné wrote:
>>> On Fri, Dec 01, 2023 at 10:41:45AM +0100, Jan Beulich wrote:
>>>> On 01.12.2023 09:50, Roger Pau Monné wrote:
>>>>> On Fri, Dec 01, 2023 at 07:53:29AM +0100, Jan Beulich wrote:
>>>>>> On 30.11.2023 18:37, Roger Pau Monné wrote:
>>>>>>> On Thu, Nov 30, 2023 at 05:55:07PM +0100, Jan Beulich wrote:
>>>>>>>> On 28.11.2023 11:03, Roger Pau Monne wrote:
>>>>>>>>> The minimal function size requirements for livepatch are either 5 bytes (for
>>>>>>>>> jmp) or 9 bytes (for endbr + jmp).  Ensure that functions are always at least
>>>>>>>>> that size by requesting the compiled to align the functions to 8 or 16 bytes,
>>>>>>>>> depending on whether Xen is build with IBT support.
>>>>>>>>
>>>>>>>> How is alignment going to enforce minimum function size? If a function is
>>>>>>>> last in a section, there may not be any padding added (ahead of linking at
>>>>>>>> least). The trailing padding also isn't part of the function.
>>>>>>>
>>>>>>> If each function lives in it's own section (by using
>>>>>>> -ffunction-sections), and each section is aligned, then I think we can
>>>>>>> guarantee that there will always be enough padding space?
>>>>>>>
>>>>>>> Even the last function/section on the .text block would still be
>>>>>>> aligned, and as long as the function alignment <= SECTION_ALIGN
>>>>>>> there will be enough padding left.  I should add some build time
>>>>>>> assert that CONFIG_CC_FUNCTION_ALIGNMENT <= SECTION_ALIGN.
>>>>>>
>>>>>> I'm not sure of there being a requirement for a section to be padded to
>>>>>> its alignment. If the following section has smaller alignment, it could
>>>>>> be made start earlier. Of course our linker scripts might guarantee
>>>>>> this ...
>>>>>
>>>>> I do think so, given our linker script arrangements for the .text
>>>>> section:
>>>>>
>>>>> DECL_SECTION(.text) {
>>>>>     [...]
>>>>> } PHDR(text) = 0x9090
>>>>>
>>>>> . = ALIGN(SECTION_ALIGN);
>>>>>
>>>>> The end of the text section is aligned to SECTION_ALIGN, so as long as
>>>>> SECTION_ALIGN >= CONFIG_CC_FUNCTION_ALIGNMENT the alignment should
>>>>> guarantee a minimal function size.
>>>>>
>>>>> Do you think it would be clearer if I add the following paragraph:
>>>>>
>>>>> "Given the Xen linker script arrangement of the .text section, we can
>>>>> ensure that when all functions are aligned to the given boundary the
>>>>> function size will always be a multiple of such alignment, even for
>>>>> the last function in .text, as the linker script aligns the end of the
>>>>> section to SECTION_ALIGN."
>>>>
>>>> I think this would be useful to have there. Beyond that, assembly code
>>>> also needs considering btw.
>>>
>>> Assembly will get dealt with once we start to also have separate
>>> sections for each assembly function.  We cannot patch assembly code at
>>> the moment anyway, due to lack of debug symbols.
>>
>> Well, yes, that's one part of it. The other is that some .text coming
>> from an assembly source may follow one coming from some C source, and
>> if the assembly one then isn't properly aligned, padding space again
>> wouldn't necessarily be large enough. This may be alright now (where
>> .text is the only thing that can come from .S and would be linked
>> ahead of all .text.*, being the only thing that can come from .c),
> 
> What about adding:
> 
> #ifdef CONFIG_CC_SPLIT_SECTIONS
>        *(.text.*)
> #endif
> #ifdef CONFIG_CC_FUNCTION_ALIGNMENT
>        /* Ensure enough padding regardless of next section alignment. */
>        . = ALIGN(CONFIG_CC_FUNCTION_ALIGNMENT)
> #endif
> 
> In order to assert that the end of .text.* is also aligned?

Probably.

>> but
>> it might subtly when assembly code is also switched to per-function
>> sections (you may recall that a patch to this effect is already
>> pending: "common: honor CONFIG_CC_SPLIT_SECTIONS also for assembly
>> functions").
> 
> Yes, I think such patch should also honor the required alignment
> specified in CONFIG_CC_FUNCTION_ALIGNMENT.

I've added a note for myself to that patch, to adjust once yours has
landed (which given the state of my series is likely going to be much
earlier).

Jan

Re: [PATCH v2 3/5] xen/x86: introduce self modifying code test

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 673 bytes --]

On Fri, 1 Dec 2023, Roger Pau Monné wrote:
> > > @@ -1261,6 +1269,7 @@ struct xen_sysctl {
> > >          struct xen_sysctl_livepatch_op      livepatch;
> > >  #if defined(__i386__) || defined(__x86_64__)
> > >          struct xen_sysctl_cpu_policy        cpu_policy;
> > > +        struct xen_sysctl_test_smc          smc;
> > 
> > Imo the field name would better be test_smc (leaving aside Stefano's comment).
> 
> Right, will see what Stefano thinks about using test_smoc.


If you meant "test_smoc", that's totally fine.

If you meant "test_smc" I think that must be a test for the virtual SMC
interface that Xen exposes, right?  Good we need some tests for vsmc.c
;-)

Re: [PATCH v2 4/5] x86/livepatch: introduce a basic live patch test to gitlab CI

[Jan Beulich]

On 28.11.2023 11:03, Roger Pau Monne wrote:
> 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>
> ---
> Changes since v1:
>  - New interface & test.
> ---
>  tools/misc/xen-livepatch.c          | 29 +++++++++++++++++++++++++++++
>  xen/arch/x86/Makefile               |  2 ++
>  xen/arch/x86/include/asm/test-smc.h |  2 ++
>  xen/arch/x86/setup.c                |  2 +-
>  xen/arch/x86/test-smc-lp-alt.c      | 23 +++++++++++++++++++++++
>  xen/arch/x86/test-smc-lp.c          | 23 +++++++++++++++++++++++
>  xen/arch/x86/test-smc.c             | 11 ++++++++++-
>  xen/include/public/sysctl.h         |  6 +++++-
>  8 files changed, 95 insertions(+), 3 deletions(-)
>  create mode 100644 xen/arch/x86/test-smc-lp-alt.c
>  create mode 100644 xen/arch/x86/test-smc-lp.c

Can these (and perhaps also the one file introduced earlier in the series)
perhaps become xen/arch/x86/test/smc*.c?

> --- /dev/null
> +++ b/xen/arch/x86/test-smc-lp-alt.c
> @@ -0,0 +1,23 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#include <asm/test-smc.h>
> +
> +/*
> + * Interesting case because `return false` can be encoded as an xor
> + * instruction, which is shorter than `return true` which is a mov instruction,
> + * and also shorter than a jmp instruction.
> + */

I'm a little wary of this comment: "mov $1, %al" is two bytes only, just like
"xor %eax, %eax" is.

> +bool cf_check test_lp_insn_replacement(void)

What's the purpose of the cf_check here?

Jan

Re: [PATCH v2 4/5] x86/livepatch: introduce a basic live patch test to gitlab CI

[Roger Pau Monné]

On Tue, Dec 05, 2023 at 12:49:38PM +0100, Jan Beulich wrote:
> On 28.11.2023 11:03, Roger Pau Monne wrote:
> > 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>
> > ---
> > Changes since v1:
> >  - New interface & test.
> > ---
> >  tools/misc/xen-livepatch.c          | 29 +++++++++++++++++++++++++++++
> >  xen/arch/x86/Makefile               |  2 ++
> >  xen/arch/x86/include/asm/test-smc.h |  2 ++
> >  xen/arch/x86/setup.c                |  2 +-
> >  xen/arch/x86/test-smc-lp-alt.c      | 23 +++++++++++++++++++++++
> >  xen/arch/x86/test-smc-lp.c          | 23 +++++++++++++++++++++++
> >  xen/arch/x86/test-smc.c             | 11 ++++++++++-
> >  xen/include/public/sysctl.h         |  6 +++++-
> >  8 files changed, 95 insertions(+), 3 deletions(-)
> >  create mode 100644 xen/arch/x86/test-smc-lp-alt.c
> >  create mode 100644 xen/arch/x86/test-smc-lp.c
> 
> Can these (and perhaps also the one file introduced earlier in the series)
> perhaps become xen/arch/x86/test/smc*.c?

Yes, sure, I don't see why not.

> > --- /dev/null
> > +++ b/xen/arch/x86/test-smc-lp-alt.c
> > @@ -0,0 +1,23 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +
> > +#include <asm/test-smc.h>
> > +
> > +/*
> > + * Interesting case because `return false` can be encoded as an xor
> > + * instruction, which is shorter than `return true` which is a mov instruction,
> > + * and also shorter than a jmp instruction.
> > + */
> 
> I'm a little wary of this comment: "mov $1, %al" is two bytes only, just like

Don't we need to zero the high part of the register also?  Or since
the return type is a bool the compiler could assume it's truncated by
the caller?

> "xor %eax, %eax" is.

GCC 13.2 (from godbolt) generates at -O2:

mov    $0x1,%eax
ret

Which is 5 bytes long mov insn.

The return false case is:

xor    %eax,%eax
ret

I can adjust to mention this specific behavior.

> > +bool cf_check test_lp_insn_replacement(void)
> 
> What's the purpose of the cf_check here?

Because it's added to the array of test functions to call in
test_smc().  Doesn't it need cf_check in that case?

Thanks, Roger.

Re: [PATCH v2 4/5] x86/livepatch: introduce a basic live patch test to gitlab CI

[Jan Beulich]

On 05.12.2023 14:08, Roger Pau Monné wrote:
> On Tue, Dec 05, 2023 at 12:49:38PM +0100, Jan Beulich wrote:
>> On 28.11.2023 11:03, Roger Pau Monne wrote:
>>> --- /dev/null
>>> +++ b/xen/arch/x86/test-smc-lp-alt.c
>>> @@ -0,0 +1,23 @@
>>> +/* SPDX-License-Identifier: GPL-2.0 */
>>> +
>>> +#include <asm/test-smc.h>
>>> +
>>> +/*
>>> + * Interesting case because `return false` can be encoded as an xor
>>> + * instruction, which is shorter than `return true` which is a mov instruction,
>>> + * and also shorter than a jmp instruction.
>>> + */
>>
>> I'm a little wary of this comment: "mov $1, %al" is two bytes only, just like
> 
> Don't we need to zero the high part of the register also?  Or since
> the return type is a bool the compiler could assume it's truncated by
> the caller?

I think so, yes. I.e. ...

>> "xor %eax, %eax" is.
> 
> GCC 13.2 (from godbolt) generates at -O2:
> 
> mov    $0x1,%eax
> ret
> 
> Which is 5 bytes long mov insn.

... at -Os I'd kind of expect the compiler to use the shorter (albeit
slower to execute) "mov $1,%al" (unless of course I'm overlooking a
specific rule in psABI).

> The return false case is:
> 
> xor    %eax,%eax
> ret
> 
> I can adjust to mention this specific behavior.
> 
>>> +bool cf_check test_lp_insn_replacement(void)
>>
>> What's the purpose of the cf_check here?
> 
> Because it's added to the array of test functions to call in
> test_smc().  Doesn't it need cf_check in that case?

Oh, of course it does. I managed to overlook that use (misguided by one
of the two files being built without being actually used).

Jan

Re: [PATCH v2 1/5] x86/livepatch: set function alignment to ensure minimal function size

[Andrew Cooper]

On 28/11/2023 10:03 am, Roger Pau Monne wrote:
> diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> index f3abdf9cd111..f629157086d0 100644
> --- a/xen/arch/x86/Makefile
> +++ b/xen/arch/x86/Makefile
> @@ -82,6 +82,8 @@ obj-$(CONFIG_COMPAT) += x86_64/platform_hypercall.o
>  obj-y += sysctl.o
>  endif
>  
> +CFLAGS-$(CONFIG_LIVEPATCH) += -falign-functions=$(CONFIG_CC_FUNCTION_ALIGNMENT)

I'd really prefer not to express it like this.  For one, a major reason
for using an alignment of 16b or more is simply performance.

Also, it isn't "CC" when we get the asm macros working.

Copy Linux more closely.  Then, you have LIVEPATCH select
FUNCTION_ALIGNMENT_{8,16}B as appropriate.  And PERFORMANCE selects
FUNCTION_ALIGNMENT_16B or perhaps 32B depending on uarch.

If we ever get around to having KCFI, then we need 16B irrespective of
anything else.



As for the subject, it's not really about size; the function size is
still going to be small irrespective of the alignment.  It's about
having space in the final binary to livepatch:

int foo(void)
{
    return 0;
}

into

int foo(void)
{
    return 1;
}

which is something you can't always do right now without overwriting the
head of whichever function is following foo() in the binary.

~Andrew

Re: [PATCH v2 1/5] x86/livepatch: set function alignment to ensure minimal function size

[Roger Pau Monné]

On Tue, Dec 05, 2023 at 01:42:42PM +0000, Andrew Cooper wrote:
> On 28/11/2023 10:03 am, Roger Pau Monne wrote:
> > diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> > index f3abdf9cd111..f629157086d0 100644
> > --- a/xen/arch/x86/Makefile
> > +++ b/xen/arch/x86/Makefile
> > @@ -82,6 +82,8 @@ obj-$(CONFIG_COMPAT) += x86_64/platform_hypercall.o
> >  obj-y += sysctl.o
> >  endif
> >  
> > +CFLAGS-$(CONFIG_LIVEPATCH) += -falign-functions=$(CONFIG_CC_FUNCTION_ALIGNMENT)
> 
> I'd really prefer not to express it like this.  For one, a major reason
> for using an alignment of 16b or more is simply performance.
> 
> Also, it isn't "CC" when we get the asm macros working.
> 
> Copy Linux more closely.  Then, you have LIVEPATCH select
> FUNCTION_ALIGNMENT_{8,16}B as appropriate.  And PERFORMANCE selects
> FUNCTION_ALIGNMENT_16B or perhaps 32B depending on uarch.

So just use CONFIG_FUNCTION_ALIGNMENT and drop the CC part of it?
That would indeed be fine.  We will also need to adjust
CC_SPLIT_SECTIONS to drop the CC_ prefix when we start using it in
assembly code.

> If we ever get around to having KCFI, then we need 16B irrespective of
> anything else.
> 
> 
> 
> As for the subject, it's not really about size; the function size is
> still going to be small irrespective of the alignment.

What about wording it like:

x86/livepatch: set function alignment to ensure minimal space between functions

Thanks, Roger.

Re: [PATCH v2 1/5] x86/livepatch: set function alignment to ensure minimal function size

[Jan Beulich]

On 05.12.2023 16:01, Roger Pau Monné wrote:
> On Tue, Dec 05, 2023 at 01:42:42PM +0000, Andrew Cooper wrote:
>> On 28/11/2023 10:03 am, Roger Pau Monne wrote:
>>> diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
>>> index f3abdf9cd111..f629157086d0 100644
>>> --- a/xen/arch/x86/Makefile
>>> +++ b/xen/arch/x86/Makefile
>>> @@ -82,6 +82,8 @@ obj-$(CONFIG_COMPAT) += x86_64/platform_hypercall.o
>>>  obj-y += sysctl.o
>>>  endif
>>>  
>>> +CFLAGS-$(CONFIG_LIVEPATCH) += -falign-functions=$(CONFIG_CC_FUNCTION_ALIGNMENT)
>>
>> I'd really prefer not to express it like this.  For one, a major reason
>> for using an alignment of 16b or more is simply performance.
>>
>> Also, it isn't "CC" when we get the asm macros working.
>>
>> Copy Linux more closely.  Then, you have LIVEPATCH select
>> FUNCTION_ALIGNMENT_{8,16}B as appropriate.  And PERFORMANCE selects
>> FUNCTION_ALIGNMENT_16B or perhaps 32B depending on uarch.
> 
> So just use CONFIG_FUNCTION_ALIGNMENT and drop the CC part of it?
> That would indeed be fine.  We will also need to adjust
> CC_SPLIT_SECTIONS to drop the CC_ prefix when we start using it in
> assembly code.

Could we prune the CC infixes once everything is settled asm-code-wise?

>> If we ever get around to having KCFI, then we need 16B irrespective of
>> anything else.
>>
>>
>>
>> As for the subject, it's not really about size; the function size is
>> still going to be small irrespective of the alignment.
> 
> What about wording it like:
> 
> x86/livepatch: set function alignment to ensure minimal space between functions

This still wouldn't be right, as there may be no padding at all between
functions (if they're just the right size). Maybe "minimal distance
between function entry points"? Getting long-ish, though ...

Jan

Re: [PATCH v2 1/5] x86/livepatch: set function alignment to ensure minimal function size

[Roger Pau Monné]

On Tue, Dec 05, 2023 at 04:14:57PM +0100, Jan Beulich wrote:
> On 05.12.2023 16:01, Roger Pau Monné wrote:
> > On Tue, Dec 05, 2023 at 01:42:42PM +0000, Andrew Cooper wrote:
> >> On 28/11/2023 10:03 am, Roger Pau Monne wrote:
> >>> diff --git a/xen/arch/x86/Makefile b/xen/arch/x86/Makefile
> >>> index f3abdf9cd111..f629157086d0 100644
> >>> --- a/xen/arch/x86/Makefile
> >>> +++ b/xen/arch/x86/Makefile
> >>> @@ -82,6 +82,8 @@ obj-$(CONFIG_COMPAT) += x86_64/platform_hypercall.o
> >>>  obj-y += sysctl.o
> >>>  endif
> >>>  
> >>> +CFLAGS-$(CONFIG_LIVEPATCH) += -falign-functions=$(CONFIG_CC_FUNCTION_ALIGNMENT)
> >>
> >> I'd really prefer not to express it like this.  For one, a major reason
> >> for using an alignment of 16b or more is simply performance.
> >>
> >> Also, it isn't "CC" when we get the asm macros working.
> >>
> >> Copy Linux more closely.  Then, you have LIVEPATCH select
> >> FUNCTION_ALIGNMENT_{8,16}B as appropriate.  And PERFORMANCE selects
> >> FUNCTION_ALIGNMENT_16B or perhaps 32B depending on uarch.
> > 
> > So just use CONFIG_FUNCTION_ALIGNMENT and drop the CC part of it?
> > That would indeed be fine.  We will also need to adjust
> > CC_SPLIT_SECTIONS to drop the CC_ prefix when we start using it in
> > assembly code.
> 
> Could we prune the CC infixes once everything is settled asm-code-wise?

That would also be fine by me.

> >> If we ever get around to having KCFI, then we need 16B irrespective of
> >> anything else.
> >>
> >>
> >>
> >> As for the subject, it's not really about size; the function size is
> >> still going to be small irrespective of the alignment.
> > 
> > What about wording it like:
> > 
> > x86/livepatch: set function alignment to ensure minimal space between functions
> 
> This still wouldn't be right, as there may be no padding at all between
> functions (if they're just the right size).

But no padding would still be fine given the text above, as then the
minimal space requirement is already meet?

> Maybe "minimal distance
> between function entry points"? Getting long-ish, though ...

Oh, I see.  You want to explicitly mention the distance is between
function entry points, as otherwise one way to read the subject would
be distance between function end and next function entry point?

It's indeed a bit long for my taste, but I don't mind adjusting if you
think the current wording could cause confusion.

Thanks, Roger.

Re: [PATCH v2 1/5] x86/livepatch: set function alignment to ensure minimal function size

[Jan Beulich]

On 05.12.2023 16:36, Roger Pau Monné wrote:
> On Tue, Dec 05, 2023 at 04:14:57PM +0100, Jan Beulich wrote:
>> On 05.12.2023 16:01, Roger Pau Monné wrote:
>>> On Tue, Dec 05, 2023 at 01:42:42PM +0000, Andrew Cooper wrote:
>>>> As for the subject, it's not really about size; the function size is
>>>> still going to be small irrespective of the alignment.
>>>
>>> What about wording it like:
>>>
>>> x86/livepatch: set function alignment to ensure minimal space between functions
>>
>> This still wouldn't be right, as there may be no padding at all between
>> functions (if they're just the right size).
> 
> But no padding would still be fine given the text above, as then the
> minimal space requirement is already meet?
> 
>> Maybe "minimal distance
>> between function entry points"? Getting long-ish, though ...
> 
> Oh, I see.  You want to explicitly mention the distance is between
> function entry points, as otherwise one way to read the subject would
> be distance between function end and next function entry point?

Yes, I saw no other way of reading it. IOW ...

> It's indeed a bit long for my taste, but I don't mind adjusting if you
> think the current wording could cause confusion.

... it already did cause confusion. But maybe we can still think of
shrinking the result some ...

Jan

Re: [PATCH v2 5/5] automation: add x86-64 livepatching test

[Roger Pau Monné]

On Wed, Nov 29, 2023 at 07:03:10PM -0800, Stefano Stabellini wrote:
> On Tue, 28 Nov 2023, Roger Pau Monne wrote:
> > 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>
> > ---
> >  automation/gitlab-ci/build.yaml               |  8 +++
> >  automation/gitlab-ci/test.yaml                |  8 +++
> >  automation/scripts/build                      | 21 ++++++
> >  .../scripts/qemu-alpine-x86_64-livepatch.sh   | 68 +++++++++++++++++++
> >  4 files changed, 105 insertions(+)
> >  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..22026df51b87 100644
> > --- a/automation/gitlab-ci/build.yaml
> > +++ b/automation/gitlab-ci/build.yaml
> > @@ -358,6 +358,14 @@ alpine-3.18-gcc-debug:
> >    variables:
> >      CONTAINER: alpine:3.18
> >  
> > +alpine-3.18-gcc-livepatch:
> > +  extends: .gcc-x86-64-build
> > +  variables:
> > +    CONTAINER: alpine:3.18
> > +    LIVEPATCH: y
> > +    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 b/automation/scripts/build
> > index b3c71fb6fb60..0a0a6dceb08c 100755
> > --- a/automation/scripts/build
> > +++ b/automation/scripts/build
> > @@ -103,3 +103,24 @@ else
> >      cp -r dist binaries/
> >      if [[ -f xen/xen ]] ; then cp xen/xen binaries/xen; fi
> >  fi
> > +
> > +if [[ "$LIVEPATCH" == "y" ]]; then
> > +    # Build a test livepatch using livepatch-build-tools.
> > +
> > +    if [[ "$XEN_TARGET_ARCH" != "x86_64" ]]; then
> > +        exit 1
> > +    fi
> > +
> > +    # git diff --no-index returns 0 if no differences, otherwise 1.
> > +    git diff --no-index --output=test.patch xen/arch/x86/test-smc-lp.c \
> > +                                            xen/arch/x86/test-smc-lp-alt.c && 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
> > +fi
> 
> I realize this is a matter of taste but if possible I would move this to
> qemu-alpine-x86_64-livepatch.sh not to make the build script too
> complex.

I've attempted that, but there are some issues.  First, the
elfutils-dev package would need to be added to the test container,
checkout livepatch-build-tools.git from the test script, and do the
differential build in the test script, so all the Xen hypervisor build
dependencies would also be needed in the test container.

> Otherwise, plase create automation/scripts/livepatch and move this code
> there. You can call automation/scripts/livepatch from
> automation/scripts/build.

Unless you have a strong desire to pursue building the livepatch in
the test step, I will go with the route proposed here, and split the
livepatch build into automation/scripts/build-livepatch.

> Other than that, this is great! I'll let other review the livepatch
> specific changes in this series

Thanks, will post a new version soon.

Roger.

Re: [PATCH v2 5/5] automation: add x86-64 livepatching test

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 4561 bytes --]

On Wed, 13 Dec 2023, Roger Pau Monné wrote:
> On Wed, Nov 29, 2023 at 07:03:10PM -0800, Stefano Stabellini wrote:
> > On Tue, 28 Nov 2023, Roger Pau Monne wrote:
> > > 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>
> > > ---
> > >  automation/gitlab-ci/build.yaml               |  8 +++
> > >  automation/gitlab-ci/test.yaml                |  8 +++
> > >  automation/scripts/build                      | 21 ++++++
> > >  .../scripts/qemu-alpine-x86_64-livepatch.sh   | 68 +++++++++++++++++++
> > >  4 files changed, 105 insertions(+)
> > >  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..22026df51b87 100644
> > > --- a/automation/gitlab-ci/build.yaml
> > > +++ b/automation/gitlab-ci/build.yaml
> > > @@ -358,6 +358,14 @@ alpine-3.18-gcc-debug:
> > >    variables:
> > >      CONTAINER: alpine:3.18
> > >  
> > > +alpine-3.18-gcc-livepatch:
> > > +  extends: .gcc-x86-64-build
> > > +  variables:
> > > +    CONTAINER: alpine:3.18
> > > +    LIVEPATCH: y
> > > +    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 b/automation/scripts/build
> > > index b3c71fb6fb60..0a0a6dceb08c 100755
> > > --- a/automation/scripts/build
> > > +++ b/automation/scripts/build
> > > @@ -103,3 +103,24 @@ else
> > >      cp -r dist binaries/
> > >      if [[ -f xen/xen ]] ; then cp xen/xen binaries/xen; fi
> > >  fi
> > > +
> > > +if [[ "$LIVEPATCH" == "y" ]]; then
> > > +    # Build a test livepatch using livepatch-build-tools.
> > > +
> > > +    if [[ "$XEN_TARGET_ARCH" != "x86_64" ]]; then
> > > +        exit 1
> > > +    fi
> > > +
> > > +    # git diff --no-index returns 0 if no differences, otherwise 1.
> > > +    git diff --no-index --output=test.patch xen/arch/x86/test-smc-lp.c \
> > > +                                            xen/arch/x86/test-smc-lp-alt.c && 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
> > > +fi
> > 
> > I realize this is a matter of taste but if possible I would move this to
> > qemu-alpine-x86_64-livepatch.sh not to make the build script too
> > complex.
> 
> I've attempted that, but there are some issues.  First, the
> elfutils-dev package would need to be added to the test container,
> checkout livepatch-build-tools.git from the test script, and do the
> differential build in the test script, so all the Xen hypervisor build
> dependencies would also be needed in the test container.
> 
> > Otherwise, plase create automation/scripts/livepatch and move this code
> > there. You can call automation/scripts/livepatch from
> > automation/scripts/build.
> 
> Unless you have a strong desire to pursue building the livepatch in
> the test step, I will go with the route proposed here, and split the
> livepatch build into automation/scripts/build-livepatch.

I am OK with this.


> > Other than that, this is great! I'll let other review the livepatch
> > specific changes in this series
> 
> Thanks, will post a new version soon.
> 
> Roger.
>