[PATCH v6 0/4] x86/xen-ucode: Introduce --force option

[PATCH v6 0/4] x86/xen-ucode: Introduce --force option

[Fouad Hilly]

Refactor and introduce --force option to xen-ucode, which skips microcode
version check when updating x86 CPU micocode. A new hypercall introduced
with flags field to facilitate the new option and allow for future flags
as needed.
This change is required to enable developers to load ucode that is the same version as the
one already loaded or downgrade for testing.

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

Fouad Hilly (4):
  x86/ucode: Introduce XENPF_microcode_update2 with flags parameter
  x86/ucode: refactor xen-ucode to utilize getopt
  x86/ucode: Introduce --force option to xen-ucode
  x86/ucode: Utilize ucode_force and remove opt_ucode_allow_same

 docs/misc/xen-command-line.pandoc    |  7 +-
 tools/include/xenctrl.h              |  3 +-
 tools/libs/ctrl/xc_misc.c            | 12 ++--
 tools/misc/xen-ucode.c               | 63 +++++++++++++++---
 xen/arch/x86/cpu/microcode/amd.c     |  8 ++-
 xen/arch/x86/cpu/microcode/core.c    | 99 +++++++++++++++++-----------
 xen/arch/x86/cpu/microcode/intel.c   |  9 ++-
 xen/arch/x86/cpu/microcode/private.h |  5 +-
 xen/arch/x86/include/asm/microcode.h |  3 +-
 xen/arch/x86/platform_hypercall.c    | 13 +++-
 xen/include/public/platform.h        | 14 ++++
 11 files changed, 167 insertions(+), 69 deletions(-)

-- 
2.42.0

[PATCH v6 1/4] x86/ucode: Introduce XENPF_microcode_update2 with flags parameter

[Fouad Hilly]

Refactor microcode_update() by adding flags field.
struct xenpf_microcode_update2 added with uint32_t flags field.
Introduce XENPF_microcode_update2 hypercall with flags field.

Signed-off-by: Fouad Hilly <fouad.hilly@cloud.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
[v6]
No change
[v5]
1- Update commit message to include the full name of XENPF_microcode_update2
[v4]
1- Commit message and description updated.
2- Changing the order of the patches.
[v3]
1- Updated Commit message description.
2- Revereted changes to a stable ABI and introduced a new struct.
3- ucode_force_flag updated from static to a local variable.
4- microcode_update() updated to reject unsupported flags yet.
[v2]
1- Update message description to highlight interface change.
2- Removed extra empty lines.
3- removed unnecessary define.
4- Corrected long lines.
5- Removed ternary operator.
6- Introduced static ucode_update_flags, which will be used later to determine local ucode_force_flag.
---
 xen/arch/x86/cpu/microcode/core.c    | 11 ++++++++---
 xen/arch/x86/include/asm/microcode.h |  3 ++-
 xen/arch/x86/platform_hypercall.c    | 13 ++++++++++++-
 xen/include/public/platform.h        | 14 ++++++++++++++
 4 files changed, 36 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
index e90055772acf..8a9e744489b9 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -40,6 +40,8 @@
 #include <asm/processor.h>
 #include <asm/setup.h>
 
+#include <public/platform.h>
+
 #include "private.h"
 
 /*
@@ -570,6 +572,7 @@ static int cf_check do_microcode_update(void *patch)
 }
 
 struct ucode_buf {
+    unsigned int flags;
     unsigned int len;
     char buffer[];
 };
@@ -708,13 +711,14 @@ static long cf_check microcode_update_helper(void *data)
     return ret;
 }
 
-int microcode_update(XEN_GUEST_HANDLE(const_void) buf, unsigned long len)
+int microcode_update(XEN_GUEST_HANDLE(const_void) buf,
+                     unsigned long len, unsigned int flags)
 {
     int ret;
     struct ucode_buf *buffer;
 
-    if ( len != (uint32_t)len )
-        return -E2BIG;
+    if ( flags & ~XENPF_UCODE_FORCE )
+        return -EINVAL;
 
     if ( !ucode_ops.apply_microcode )
         return -EINVAL;
@@ -730,6 +734,7 @@ int microcode_update(XEN_GUEST_HANDLE(const_void) buf, unsigned long len)
         return -EFAULT;
     }
     buffer->len = len;
+    buffer->flags = flags;
 
     /*
      * Always queue microcode_update_helper() on CPU0.  Most of the logic
diff --git a/xen/arch/x86/include/asm/microcode.h b/xen/arch/x86/include/asm/microcode.h
index 8f59b20b0289..57c08205d475 100644
--- a/xen/arch/x86/include/asm/microcode.h
+++ b/xen/arch/x86/include/asm/microcode.h
@@ -22,7 +22,8 @@ struct cpu_signature {
 DECLARE_PER_CPU(struct cpu_signature, cpu_sig);
 
 void microcode_set_module(unsigned int idx);
-int microcode_update(XEN_GUEST_HANDLE(const_void) buf, unsigned long len);
+int microcode_update(XEN_GUEST_HANDLE(const_void) buf,
+                     unsigned long len, unsigned int flags);
 int early_microcode_init(unsigned long *module_map,
                          const struct multiboot_info *mbi);
 int microcode_init_cache(unsigned long *module_map,
diff --git a/xen/arch/x86/platform_hypercall.c b/xen/arch/x86/platform_hypercall.c
index 95467b88ab64..7e3278109300 100644
--- a/xen/arch/x86/platform_hypercall.c
+++ b/xen/arch/x86/platform_hypercall.c
@@ -311,7 +311,18 @@ ret_t do_platform_op(
 
         guest_from_compat_handle(data, op->u.microcode.data);
 
-        ret = microcode_update(data, op->u.microcode.length);
+        ret = microcode_update(data, op->u.microcode.length, 0);
+        break;
+    }
+
+    case XENPF_microcode_update2:
+    {
+        XEN_GUEST_HANDLE(const_void) data;
+
+        guest_from_compat_handle(data, op->u.microcode2.data);
+
+        ret = microcode_update(data, op->u.microcode2.length,
+                               op->u.microcode2.flags);
         break;
     }
 
diff --git a/xen/include/public/platform.h b/xen/include/public/platform.h
index 15777b541690..2725b8d1044f 100644
--- a/xen/include/public/platform.h
+++ b/xen/include/public/platform.h
@@ -624,6 +624,19 @@ struct xenpf_ucode_revision {
 typedef struct xenpf_ucode_revision xenpf_ucode_revision_t;
 DEFINE_XEN_GUEST_HANDLE(xenpf_ucode_revision_t);
 
+/* Hypercall to microcode_update with flags */
+#define XENPF_microcode_update2    66
+struct xenpf_microcode_update2 {
+    /* IN variables. */
+    uint32_t flags;                   /* Flags to be passed with ucode. */
+/* Force to skip microcode version check */
+#define XENPF_UCODE_FORCE           1
+    uint32_t length;                  /* Length of microcode data. */
+    XEN_GUEST_HANDLE(const_void) data;/* Pointer to microcode data */
+};
+typedef struct xenpf_microcode_update2 xenpf_microcode_update2_t;
+DEFINE_XEN_GUEST_HANDLE(xenpf_microcode_update2_t);
+
 /*
  * ` enum neg_errnoval
  * ` HYPERVISOR_platform_op(const struct xen_platform_op*);
@@ -656,6 +669,7 @@ struct xen_platform_op {
         xenpf_symdata_t               symdata;
         xenpf_dom0_console_t          dom0_console;
         xenpf_ucode_revision_t        ucode_revision;
+        xenpf_microcode_update2_t     microcode2;
         uint8_t                       pad[128];
     } u;
 };
-- 
2.42.0

[PATCH v6 2/4] x86/ucode: refactor xen-ucode to utilize getopt

[Fouad Hilly]

Use getopt_long() to handle command line arguments.
Introduce ext_err for common exit with errors.
Introducing usage() to handle usage\help messages in a common block.
show_curr_cpu is printed to stdout only.

Signed-off-by: Fouad Hilly <fouad.hilly@cloud.com>
---
[v6]
1- Update usage() printed message format: [microcode file] [options] -> [microcode file | options]
2- Add missing blanks in switch ( opt )
[v5]
1- Update message description.
2- re-arrange static and automatic variables.
3- Fix indentations.
4- reverted the deletion of show-cpu-info for backwards compatibility.
[v4]
1- Merge three patches into one.
2- usage() to print messages to the correct stream.
3- Update commit message and description.
---
 tools/misc/xen-ucode.c | 52 +++++++++++++++++++++++++++++++++++-------
 1 file changed, 44 insertions(+), 8 deletions(-)

diff --git a/tools/misc/xen-ucode.c b/tools/misc/xen-ucode.c
index 390969db3d1c..2c9f337b86cb 100644
--- a/tools/misc/xen-ucode.c
+++ b/tools/misc/xen-ucode.c
@@ -11,6 +11,7 @@
 #include <sys/stat.h>
 #include <fcntl.h>
 #include <xenctrl.h>
+#include <getopt.h>
 
 static xc_interface *xch;
 
@@ -71,12 +72,29 @@ static void show_curr_cpu(FILE *f)
     }
 }
 
+static void usage(FILE *stream, const char *name)
+{
+    fprintf(stream,
+            "%s: Xen microcode updating tool\n"
+            "options:\n"
+            "  -h, --help            display this help\n"
+            "  -s, --show-cpu-info   show CPU information\n"
+            "Usage: %s [microcode file | options]\n", name, name);
+    show_curr_cpu(stream);
+}
+
 int main(int argc, char *argv[])
 {
+    static const struct option options[] = {
+        {"help", no_argument, NULL, 'h'},
+        {"show-cpu-info", no_argument, NULL, 's'},
+        {NULL, no_argument, NULL, 0}
+    };
     int fd, ret;
     char *filename, *buf;
     size_t len;
     struct stat st;
+    int opt;
 
     xch = xc_interface_open(NULL, NULL, 0);
     if ( xch == NULL )
@@ -86,22 +104,34 @@ int main(int argc, char *argv[])
         exit(1);
     }
 
-    if ( argc < 2 )
+    while ( (opt = getopt_long(argc, argv, "hs", options, NULL)) != -1 )
     {
-        fprintf(stderr,
-                "xen-ucode: Xen microcode updating tool\n"
-                "Usage: %s [<microcode file> | show-cpu-info]\n", argv[0]);
-        show_curr_cpu(stderr);
-        exit(2);
+        switch ( opt )
+        {
+        case 'h':
+            usage(stdout, argv[0]);
+            exit(EXIT_SUCCESS);
+
+        case 's':
+            show_curr_cpu(stdout);
+            exit(EXIT_SUCCESS);
+
+        default:
+            goto ext_err;
+        }
     }
 
-    if ( !strcmp(argv[1], "show-cpu-info") )
+    if ( optind == argc )
+        goto ext_err;
+
+    /* For backwards compatibility to the pre-getopt() cmdline handling */
+    if ( !strcmp(argv[optind], "show-cpu-info") )
     {
         show_curr_cpu(stdout);
         return 0;
     }
 
-    filename = argv[1];
+    filename = argv[optind];
     fd = open(filename, O_RDONLY);
     if ( fd < 0 )
     {
@@ -146,4 +176,10 @@ int main(int argc, char *argv[])
     close(fd);
 
     return 0;
+
+ ext_err:
+    fprintf(stderr,
+            "%s: unable to process command line arguments\n", argv[0]);
+    usage(stderr, argv[0]);
+    exit(EXIT_FAILURE);
 }
-- 
2.42.0

[PATCH v6 3/4] x86/ucode: Introduce --force option to xen-ucode

[Fouad Hilly]

Introduce --force option to xen-ucode to force skipping microcode version check, which
allows the user to update x86 microcode even if both versions are the same or downgrade.
xc_microcode_update() refactored to accept flags and utilize xenpf_microcode_update2.

Signed-off-by: Fouad Hilly <fouad.hilly@cloud.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
[v6]
1- Fix usage() output for -f option to be explicitly wrapped for 80 character width
[v5]
1- Update commit message.
2- Re-phrase --force option description.
[v4]
1- Add --force to xen-ucode options.
2- Update xc_microcode_update() to accept and handle flags.
---
 tools/include/xenctrl.h   |  3 ++-
 tools/libs/ctrl/xc_misc.c | 12 +++++++-----
 tools/misc/xen-ucode.c    | 15 ++++++++++++---
 3 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 9ceca0cffc2f..2c4608c09ab0 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -1171,7 +1171,8 @@ typedef uint32_t xc_node_to_node_dist_t;
 int xc_physinfo(xc_interface *xch, xc_physinfo_t *info);
 int xc_cputopoinfo(xc_interface *xch, unsigned *max_cpus,
                    xc_cputopo_t *cputopo);
-int xc_microcode_update(xc_interface *xch, const void *buf, size_t len);
+int xc_microcode_update(xc_interface *xch, const void *buf,
+                        size_t len, unsigned int flags);
 int xc_get_cpu_version(xc_interface *xch, struct xenpf_pcpu_version *cpu_ver);
 int xc_get_ucode_revision(xc_interface *xch,
                           struct xenpf_ucode_revision *ucode_rev);
diff --git a/tools/libs/ctrl/xc_misc.c b/tools/libs/ctrl/xc_misc.c
index 50282fd60dcc..6a60216bda03 100644
--- a/tools/libs/ctrl/xc_misc.c
+++ b/tools/libs/ctrl/xc_misc.c
@@ -203,11 +203,12 @@ int xc_physinfo(xc_interface *xch,
     return 0;
 }
 
-int xc_microcode_update(xc_interface *xch, const void *buf, size_t len)
+int xc_microcode_update(xc_interface *xch, const void *buf,
+                        size_t len, unsigned int flags)
 {
     int ret;
     struct xen_platform_op platform_op = {};
-    DECLARE_HYPERCALL_BUFFER(struct xenpf_microcode_update, uc);
+    DECLARE_HYPERCALL_BUFFER(struct xenpf_microcode_update2, uc);
 
     uc = xc_hypercall_buffer_alloc(xch, uc, len);
     if ( uc == NULL )
@@ -215,9 +216,10 @@ int xc_microcode_update(xc_interface *xch, const void *buf, size_t len)
 
     memcpy(uc, buf, len);
 
-    platform_op.cmd = XENPF_microcode_update;
-    platform_op.u.microcode.length = len;
-    set_xen_guest_handle(platform_op.u.microcode.data, uc);
+    platform_op.cmd = XENPF_microcode_update2;
+    platform_op.u.microcode2.length = len;
+    platform_op.u.microcode2.flags = flags;
+    set_xen_guest_handle(platform_op.u.microcode2.data, uc);
 
     ret = do_platform_op(xch, &platform_op);
 
diff --git a/tools/misc/xen-ucode.c b/tools/misc/xen-ucode.c
index 2c9f337b86cb..688e540943b1 100644
--- a/tools/misc/xen-ucode.c
+++ b/tools/misc/xen-ucode.c
@@ -13,6 +13,8 @@
 #include <xenctrl.h>
 #include <getopt.h>
 
+#include <xen/platform.h>
+
 static xc_interface *xch;
 
 static const char intel_id[] = "GenuineIntel";
@@ -79,7 +81,9 @@ static void usage(FILE *stream, const char *name)
             "options:\n"
             "  -h, --help            display this help\n"
             "  -s, --show-cpu-info   show CPU information\n"
-            "Usage: %s [microcode file | options]\n", name, name);
+            "  -f, --force           skip certain checks; do not use unless\n"
+            "you know exactly what you are doing\n"
+            "Usage: %s [microcode file [-f,--force] | options]\n", name, name);
     show_curr_cpu(stream);
 }
 
@@ -88,6 +92,7 @@ int main(int argc, char *argv[])
     static const struct option options[] = {
         {"help", no_argument, NULL, 'h'},
         {"show-cpu-info", no_argument, NULL, 's'},
+        {"force", no_argument, NULL, 'f'},
         {NULL, no_argument, NULL, 0}
     };
     int fd, ret;
@@ -95,6 +100,7 @@ int main(int argc, char *argv[])
     size_t len;
     struct stat st;
     int opt;
+    uint32_t ucode_flags = 0;
 
     xch = xc_interface_open(NULL, NULL, 0);
     if ( xch == NULL )
@@ -104,7 +110,7 @@ int main(int argc, char *argv[])
         exit(1);
     }
 
-    while ( (opt = getopt_long(argc, argv, "hs", options, NULL)) != -1 )
+    while ( (opt = getopt_long(argc, argv, "hsf", options, NULL)) != -1 )
     {
         switch ( opt )
         {
@@ -116,6 +122,9 @@ int main(int argc, char *argv[])
             show_curr_cpu(stdout);
             exit(EXIT_SUCCESS);
 
+        case 'f':
+            ucode_flags = XENPF_UCODE_FORCE;
+            break;
         default:
             goto ext_err;
         }
@@ -156,7 +165,7 @@ int main(int argc, char *argv[])
     }
 
     errno = 0;
-    ret = xc_microcode_update(xch, buf, len);
+    ret = xc_microcode_update(xch, buf, len, ucode_flags);
     if ( ret == -1 && errno == EEXIST )
         printf("Microcode already up to date\n");
     else if ( ret )
-- 
2.42.0

[PATCH v6 4/4] x86/ucode: Utilize ucode_force and remove opt_ucode_allow_same

[Fouad Hilly]

Pass xen-ucode flags to do low level checks on micorocde version and
utilize it to allow for microcode downgrade or reapply the same version of the
microcode.
ucode_force is required to be passed to a low level Intel and AMD for version
checks to be done.
While adding ucode_force, opt_ucode_allow_same was removed.
Remove opt_ucode_allow_same from documentation.

Signed-off-by: Fouad Hilly <fouad.hilly@cloud.com>
---
[v6]
1- Removed struct microcode_nmi_patch_with_flags.
2- removed const from control_thread_fn() and primary_thread_fn().
3- Fixed "flags" checks.
[v5]
1- Update commit message.
2- Introduce structs microcode_patch_with_flags and microcode_nmi_patch_with_flags.
3- pass flags to Intel and AMD low level through apply_microcode().
[4]
1- As opt_ucode_allow_same is not required anymore, it has been removed while introducing ucode_force.
2- Apply the changes for both AMD and Intel.
3- Remove the mention of opt_ucode_allow_same from documentation.
---
 docs/misc/xen-command-line.pandoc    |  7 +--
 xen/arch/x86/cpu/microcode/amd.c     |  8 ++-
 xen/arch/x86/cpu/microcode/core.c    | 88 +++++++++++++++++-----------
 xen/arch/x86/cpu/microcode/intel.c   |  9 ++-
 xen/arch/x86/cpu/microcode/private.h |  5 +-
 5 files changed, 68 insertions(+), 49 deletions(-)

diff --git a/docs/misc/xen-command-line.pandoc b/docs/misc/xen-command-line.pandoc
index 98a45211556b..2a8d47bbc664 100644
--- a/docs/misc/xen-command-line.pandoc
+++ b/docs/misc/xen-command-line.pandoc
@@ -2650,7 +2650,7 @@ performance.
    Alternatively, selecting `tsx=1` will re-enable TSX at the users own risk.
 
 ### ucode
-> `= List of [ <integer> | scan=<bool>, nmi=<bool>, allow-same=<bool> ]`
+> `= List of [ <integer> | scan=<bool>, nmi=<bool> ]`
 
     Applicability: x86
     Default: `nmi`
@@ -2682,11 +2682,6 @@ precedence over `scan`.
 stop_machine context. In NMI handler, even NMIs are blocked, which is
 considered safer. The default value is `true`.
 
-'allow-same' alters the default acceptance policy for new microcode to permit
-trying to reload the same version.  Many CPUs will actually reload microcode
-of the same version, and this allows for easy testing of the late microcode
-loading path.
-
 ### unrestricted_guest (Intel)
 > `= <boolean>`
 
diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c
index f76a563c8b84..80ff6335d64a 100644
--- a/xen/arch/x86/cpu/microcode/amd.c
+++ b/xen/arch/x86/cpu/microcode/amd.c
@@ -22,6 +22,8 @@
 
 #include "private.h"
 
+#include "public/platform.h"
+
 #define pr_debug(x...) ((void)0)
 
 struct equiv_cpu_entry {
@@ -214,13 +216,15 @@ static enum microcode_match_result cf_check compare_patch(
     return compare_header(new, old);
 }
 
-static int cf_check apply_microcode(const struct microcode_patch *patch)
+static int cf_check apply_microcode(const struct microcode_patch *patch,
+                                    unsigned int flags)
 {
     int hw_err;
     unsigned int cpu = smp_processor_id();
     struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
     uint32_t rev, old_rev = sig->rev;
     enum microcode_match_result result = microcode_fits(patch);
+    bool ucode_force = flags & XENPF_UCODE_FORCE;
 
     if ( result == MIS_UCODE )
         return -EINVAL;
@@ -229,7 +233,7 @@ static int cf_check apply_microcode(const struct microcode_patch *patch)
      * Allow application of the same revision to pick up SMT-specific changes
      * even if the revision of the other SMT thread is already up-to-date.
      */
-    if ( result == OLD_UCODE )
+    if ( !ucode_force && (result == SAME_UCODE || result == OLD_UCODE) )
         return -EEXIST;
 
     if ( check_final_patch_levels(sig) )
diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
index 8a9e744489b9..a0106a987e33 100644
--- a/xen/arch/x86/cpu/microcode/core.c
+++ b/xen/arch/x86/cpu/microcode/core.c
@@ -90,6 +90,11 @@ struct ucode_mod_blob {
     size_t size;
 };
 
+struct patch_with_flags {
+    unsigned int flags;
+    struct microcode_patch *patch;
+};
+
 static struct ucode_mod_blob __initdata ucode_blob;
 /*
  * By default we will NOT parse the multiboot modules to see if there is
@@ -100,8 +105,6 @@ static bool __initdata ucode_scan;
 /* By default, ucode loading is done in NMI handler */
 static bool ucode_in_nmi = true;
 
-bool __read_mostly opt_ucode_allow_same;
-
 /* Protected by microcode_mutex */
 static struct microcode_patch *microcode_cache;
 
@@ -128,8 +131,6 @@ static int __init cf_check parse_ucode(const char *s)
 
         if ( (val = parse_boolean("nmi", s, ss)) >= 0 )
             ucode_in_nmi = val;
-        else if ( (val = parse_boolean("allow-same", s, ss)) >= 0 )
-            opt_ucode_allow_same = val;
         else if ( !ucode_mod_forced ) /* Not forced by EFI */
         {
             if ( (val = parse_boolean("scan", s, ss)) >= 0 )
@@ -237,7 +238,11 @@ static DEFINE_PER_CPU(int, loading_err);
  */
 static cpumask_t cpu_callin_map;
 static atomic_t cpu_out, cpu_updated;
-static const struct microcode_patch *nmi_patch = ZERO_BLOCK_PTR;
+static struct patch_with_flags nmi_patch_with_flags =
+{
+    .flags  = 0,
+    .patch  = ZERO_BLOCK_PTR,
+};
 
 /*
  * Return a patch that covers current CPU. If there are multiple patches,
@@ -327,7 +332,8 @@ static bool cf_check wait_cpu_callout(unsigned int nr)
  * If no patch is provided, the cached patch will be loaded. Microcode update
  * during APs bringup and CPU resuming falls into this case.
  */
-static int microcode_update_cpu(const struct microcode_patch *patch)
+static int microcode_update_cpu(const struct microcode_patch *patch,
+                                unsigned int flags)
 {
     int err;
 
@@ -335,10 +341,11 @@ static int microcode_update_cpu(const struct microcode_patch *patch)
 
     spin_lock(&microcode_mutex);
     if ( patch )
-        err = alternative_call(ucode_ops.apply_microcode, patch);
+        err = alternative_call(ucode_ops.apply_microcode, patch, flags);
     else if ( microcode_cache )
     {
-        err = alternative_call(ucode_ops.apply_microcode, microcode_cache);
+        err = alternative_call(ucode_ops.apply_microcode, microcode_cache,
+                               flags);
         if ( err == -EIO )
         {
             microcode_free_patch(microcode_cache);
@@ -379,7 +386,8 @@ static int secondary_nmi_work(void)
     return wait_for_state(LOADING_EXIT) ? 0 : -EBUSY;
 }
 
-static int primary_thread_work(const struct microcode_patch *patch)
+static int primary_thread_work(struct microcode_patch *patch,
+                               unsigned int flags)
 {
     int ret;
 
@@ -388,7 +396,7 @@ static int primary_thread_work(const struct microcode_patch *patch)
     if ( !wait_for_state(LOADING_ENTER) )
         return -EBUSY;
 
-    ret = alternative_call(ucode_ops.apply_microcode, patch);
+    ret = alternative_call(ucode_ops.apply_microcode, patch, flags);
     if ( !ret )
         atomic_inc(&cpu_updated);
     atomic_inc(&cpu_out);
@@ -416,7 +424,8 @@ static int cf_check microcode_nmi_callback(
         return 0;
 
     if ( primary_cpu )
-        ret = primary_thread_work(nmi_patch);
+        ret = primary_thread_work(nmi_patch_with_flags.patch,
+                                  nmi_patch_with_flags.flags);
     else
         ret = secondary_nmi_work();
     this_cpu(loading_err) = ret;
@@ -446,7 +455,8 @@ static int secondary_thread_fn(void)
     return this_cpu(loading_err);
 }
 
-static int primary_thread_fn(const struct microcode_patch *patch)
+static int primary_thread_fn(struct microcode_patch *patch,
+                             unsigned int flags)
 {
     if ( !wait_for_state(LOADING_CALLIN) )
         return -EBUSY;
@@ -466,10 +476,11 @@ static int primary_thread_fn(const struct microcode_patch *patch)
         return this_cpu(loading_err);
     }
 
-    return primary_thread_work(patch);
+    return primary_thread_work(patch, flags);
 }
 
-static int control_thread_fn(const struct microcode_patch *patch)
+static int control_thread_fn(struct microcode_patch *patch,
+                             unsigned int flags)
 {
     unsigned int cpu = smp_processor_id(), done;
     unsigned long tick;
@@ -482,7 +493,8 @@ static int control_thread_fn(const struct microcode_patch *patch)
      */
     watchdog_disable();
 
-    nmi_patch = patch;
+    nmi_patch_with_flags.patch = patch;
+    nmi_patch_with_flags.flags = flags;
     smp_wmb();
     saved_nmi_callback = set_nmi_callback(microcode_nmi_callback);
 
@@ -498,7 +510,7 @@ static int control_thread_fn(const struct microcode_patch *patch)
         goto out;
 
     /* Control thread loads ucode first while others are in NMI handler. */
-    ret = alternative_call(ucode_ops.apply_microcode, patch);
+    ret = alternative_call(ucode_ops.apply_microcode, patch, flags);
     if ( !ret )
         atomic_inc(&cpu_updated);
     atomic_inc(&cpu_out);
@@ -544,17 +556,19 @@ static int control_thread_fn(const struct microcode_patch *patch)
 
     set_nmi_callback(saved_nmi_callback);
     smp_wmb();
-    nmi_patch = ZERO_BLOCK_PTR;
+    nmi_patch_with_flags.patch = ZERO_BLOCK_PTR;
+    nmi_patch_with_flags.flags = 0;
 
     watchdog_enable();
 
     return ret;
 }
 
-static int cf_check do_microcode_update(void *patch)
+static int cf_check do_microcode_update(void *_patch_with_flags)
 {
     unsigned int cpu = smp_processor_id();
     int ret;
+    struct patch_with_flags *patch_with_flags = _patch_with_flags;
 
     /*
      * The control thread set state to coordinate ucode loading. Primary
@@ -562,9 +576,11 @@ static int cf_check do_microcode_update(void *patch)
      * the completion of the ucode loading process.
      */
     if ( cpu == cpumask_first(&cpu_online_map) )
-        ret = control_thread_fn(patch);
+        ret = control_thread_fn(patch_with_flags->patch,
+                                patch_with_flags->flags);
     else if ( is_cpu_primary(cpu) )
-        ret = primary_thread_fn(patch);
+        ret = primary_thread_fn(patch_with_flags->patch,
+                                patch_with_flags->flags);
     else
         ret = secondary_thread_fn();
 
@@ -582,7 +598,8 @@ static long cf_check microcode_update_helper(void *data)
     int ret;
     struct ucode_buf *buffer = data;
     unsigned int cpu, updated;
-    struct microcode_patch *patch;
+    struct patch_with_flags patch_with_flags;
+    bool ucode_force = buffer->flags & XENPF_UCODE_FORCE;
 
     /* cpu_online_map must not change during update */
     if ( !get_cpu_maps() )
@@ -606,16 +623,17 @@ static long cf_check microcode_update_helper(void *data)
         goto put;
     }
 
-    patch = parse_blob(buffer->buffer, buffer->len);
+    patch_with_flags.patch = parse_blob(buffer->buffer, buffer->len);
+    patch_with_flags.flags = buffer->flags;
     xfree(buffer);
-    if ( IS_ERR(patch) )
+    if ( IS_ERR(patch_with_flags.patch) )
     {
-        ret = PTR_ERR(patch);
+        ret = PTR_ERR(patch_with_flags.patch);
         printk(XENLOG_WARNING "Parsing microcode blob error %d\n", ret);
         goto put;
     }
 
-    if ( !patch )
+    if ( !patch_with_flags.patch )
     {
         printk(XENLOG_WARNING "microcode: couldn't find any matching ucode in "
                               "the provided blob!\n");
@@ -632,17 +650,17 @@ static long cf_check microcode_update_helper(void *data)
     {
         enum microcode_match_result result;
 
-        result = alternative_call(ucode_ops.compare_patch, patch,
-                                  microcode_cache);
+        result = alternative_call(ucode_ops.compare_patch,
+                                  patch_with_flags.patch, microcode_cache);
 
         if ( result != NEW_UCODE &&
-             !(opt_ucode_allow_same && result == SAME_UCODE) )
+             !(ucode_force && (result == OLD_UCODE || result == SAME_UCODE)) )
         {
             spin_unlock(&microcode_mutex);
             printk(XENLOG_WARNING
                    "microcode: couldn't find any newer%s revision in the provided blob!\n",
-                   opt_ucode_allow_same ? " (or the same)" : "");
-            microcode_free_patch(patch);
+                   ucode_force? " (or a valid)" : "");
+            microcode_free_patch(patch_with_flags.patch);
             ret = -EEXIST;
 
             goto put;
@@ -674,13 +692,13 @@ static long cf_check microcode_update_helper(void *data)
      *   this requirement can be relaxed in the future. Right now, this is
      *   conservative and good.
      */
-    ret = stop_machine_run(do_microcode_update, patch, NR_CPUS);
+    ret = stop_machine_run(do_microcode_update, &patch_with_flags, NR_CPUS);
 
     updated = atomic_read(&cpu_updated);
     if ( updated > 0 )
     {
         spin_lock(&microcode_mutex);
-        microcode_update_cache(patch);
+        microcode_update_cache(patch_with_flags.patch);
         spin_unlock(&microcode_mutex);
 
         /*
@@ -697,7 +715,7 @@ static long cf_check microcode_update_helper(void *data)
             alternative_vcall(ctxt_switch_masking, current);
     }
     else
-        microcode_free_patch(patch);
+        microcode_free_patch(patch_with_flags.patch);
 
     if ( updated && updated != nr_cores )
         printk(XENLOG_ERR "ERROR: Updating microcode succeeded on %u cores and failed\n"
@@ -775,7 +793,7 @@ int microcode_update_one(void)
     if ( !ucode_ops.apply_microcode )
         return -EOPNOTSUPP;
 
-    return microcode_update_cpu(NULL);
+    return microcode_update_cpu(NULL, 0);
 }
 
 static int __init early_update_cache(const void *data, size_t len)
@@ -858,7 +876,7 @@ static int __init early_microcode_update_cpu(void)
     if ( !patch )
         return -ENOENT;
 
-    return microcode_update_cpu(patch);
+    return microcode_update_cpu(patch, 0);
 }
 
 int __init early_microcode_init(unsigned long *module_map,
diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c
index f505aa1b7888..fc80f17d2376 100644
--- a/xen/arch/x86/cpu/microcode/intel.c
+++ b/xen/arch/x86/cpu/microcode/intel.c
@@ -29,6 +29,8 @@
 
 #include "private.h"
 
+#include "public/platform.h"
+
 #define pr_debug(x...) ((void)0)
 
 struct microcode_patch {
@@ -284,21 +286,22 @@ static enum microcode_match_result cf_check compare_patch(
     return compare_revisions(old->rev, new->rev);
 }
 
-static int cf_check apply_microcode(const struct microcode_patch *patch)
+static int cf_check apply_microcode(const struct microcode_patch *patch,
+                                    unsigned int flags)
 {
     uint64_t msr_content;
     unsigned int cpu = smp_processor_id();
     struct cpu_signature *sig = &this_cpu(cpu_sig);
     uint32_t rev, old_rev = sig->rev;
     enum microcode_match_result result;
+    bool ucode_force = flags & XENPF_UCODE_FORCE;
 
     result = microcode_update_match(patch);
 
     if ( result == MIS_UCODE )
         return -EINVAL;
 
-    if ( result == OLD_UCODE ||
-         (result == SAME_UCODE && !opt_ucode_allow_same) )
+    if ( !ucode_force && (result == SAME_UCODE || result == OLD_UCODE) )
         return -EEXIST;
 
     wbinvd();
diff --git a/xen/arch/x86/cpu/microcode/private.h b/xen/arch/x86/cpu/microcode/private.h
index da556fe5060a..017889e1b58d 100644
--- a/xen/arch/x86/cpu/microcode/private.h
+++ b/xen/arch/x86/cpu/microcode/private.h
@@ -3,8 +3,6 @@
 
 #include <asm/microcode.h>
 
-extern bool opt_ucode_allow_same;
-
 enum microcode_match_result {
     OLD_UCODE, /* signature matched, but revision id is older */
     SAME_UCODE, /* signature matched, but revision id is the same */
@@ -50,7 +48,8 @@ struct microcode_ops {
      * Attempt to load the provided patch into the CPU.  Returns an error if
      * anything didn't go as expected.
      */
-    int (*apply_microcode)(const struct microcode_patch *patch);
+    int (*apply_microcode)(const struct microcode_patch *patch,
+                           unsigned int flags);
 
     /*
      * Given two patches, are they both applicable to the current CPU, and is
-- 
2.42.0

Re: [PATCH v6 2/4] x86/ucode: refactor xen-ucode to utilize getopt

[Jan Beulich]

On 25.07.2024 10:27, Fouad Hilly wrote:
> @@ -71,12 +72,29 @@ static void show_curr_cpu(FILE *f)
>      }
>  }
>  
> +static void usage(FILE *stream, const char *name)
> +{
> +    fprintf(stream,
> +            "%s: Xen microcode updating tool\n"
> +            "options:\n"
> +            "  -h, --help            display this help\n"
> +            "  -s, --show-cpu-info   show CPU information\n"
> +            "Usage: %s [microcode file | options]\n", name, name);

You did see Anthony's comments on this before sending the new version,
didn't you? I agree with him (and I'm somewhat embarrassed that I didn't
notice this myself earlier on).

> @@ -146,4 +176,10 @@ int main(int argc, char *argv[])
>      close(fd);
>  
>      return 0;
> +
> + ext_err:
> +    fprintf(stderr,
> +            "%s: unable to process command line arguments\n", argv[0]);
> +    usage(stderr, argv[0]);
> +    exit(EXIT_FAILURE);
>  }

And there was a comment on this, too.

Jan

Re: [PATCH v6 2/4] x86/ucode: refactor xen-ucode to utilize getopt

[Jan Beulich]

On 25.07.2024 10:27, Fouad Hilly wrote:
> @@ -71,12 +72,29 @@ static void show_curr_cpu(FILE *f)
>      }
>  }
>  
> +static void usage(FILE *stream, const char *name)
> +{
> +    fprintf(stream,
> +            "%s: Xen microcode updating tool\n"
> +            "options:\n"
> +            "  -h, --help            display this help\n"
> +            "  -s, --show-cpu-info   show CPU information\n"
> +            "Usage: %s [microcode file | options]\n", name, name);

Oh, and: While I gave this precise layout as an outline, it wasn't really
meant to be used literally. Note how "microcode" and "file" now suggest
there need to be two separate command line elements. Perhaps using
"microcode-file" instead may already make this less ambiguous.

Jan

Re: [PATCH v6 3/4] x86/ucode: Introduce --force option to xen-ucode

[Jan Beulich]

On 25.07.2024 10:27, Fouad Hilly wrote:
> Introduce --force option to xen-ucode to force skipping microcode version check, which
> allows the user to update x86 microcode even if both versions are the same or downgrade.
> xc_microcode_update() refactored to accept flags and utilize xenpf_microcode_update2.
> 
> Signed-off-by: Fouad Hilly <fouad.hilly@cloud.com>
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> [v6]
> 1- Fix usage() output for -f option to be explicitly wrapped for 80 character width
> [v5]
> 1- Update commit message.
> 2- Re-phrase --force option description.
> [v4]
> 1- Add --force to xen-ucode options.
> 2- Update xc_microcode_update() to accept and handle flags.
> ---
>  tools/include/xenctrl.h   |  3 ++-
>  tools/libs/ctrl/xc_misc.c | 12 +++++++-----
>  tools/misc/xen-ucode.c    | 15 ++++++++++++---
>  3 files changed, 21 insertions(+), 9 deletions(-)
> 
> diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
> index 9ceca0cffc2f..2c4608c09ab0 100644
> --- a/tools/include/xenctrl.h
> +++ b/tools/include/xenctrl.h
> @@ -1171,7 +1171,8 @@ typedef uint32_t xc_node_to_node_dist_t;
>  int xc_physinfo(xc_interface *xch, xc_physinfo_t *info);
>  int xc_cputopoinfo(xc_interface *xch, unsigned *max_cpus,
>                     xc_cputopo_t *cputopo);
> -int xc_microcode_update(xc_interface *xch, const void *buf, size_t len);
> +int xc_microcode_update(xc_interface *xch, const void *buf,
> +                        size_t len, unsigned int flags);
>  int xc_get_cpu_version(xc_interface *xch, struct xenpf_pcpu_version *cpu_ver);
>  int xc_get_ucode_revision(xc_interface *xch,
>                            struct xenpf_ucode_revision *ucode_rev);
> diff --git a/tools/libs/ctrl/xc_misc.c b/tools/libs/ctrl/xc_misc.c
> index 50282fd60dcc..6a60216bda03 100644
> --- a/tools/libs/ctrl/xc_misc.c
> +++ b/tools/libs/ctrl/xc_misc.c
> @@ -203,11 +203,12 @@ int xc_physinfo(xc_interface *xch,
>      return 0;
>  }
>  
> -int xc_microcode_update(xc_interface *xch, const void *buf, size_t len)
> +int xc_microcode_update(xc_interface *xch, const void *buf,
> +                        size_t len, unsigned int flags)
>  {
>      int ret;
>      struct xen_platform_op platform_op = {};
> -    DECLARE_HYPERCALL_BUFFER(struct xenpf_microcode_update, uc);
> +    DECLARE_HYPERCALL_BUFFER(struct xenpf_microcode_update2, uc);
>  
>      uc = xc_hypercall_buffer_alloc(xch, uc, len);
>      if ( uc == NULL )
> @@ -215,9 +216,10 @@ int xc_microcode_update(xc_interface *xch, const void *buf, size_t len)
>  
>      memcpy(uc, buf, len);
>  
> -    platform_op.cmd = XENPF_microcode_update;
> -    platform_op.u.microcode.length = len;
> -    set_xen_guest_handle(platform_op.u.microcode.data, uc);
> +    platform_op.cmd = XENPF_microcode_update2;
> +    platform_op.u.microcode2.length = len;
> +    platform_op.u.microcode2.flags = flags;
> +    set_xen_guest_handle(platform_op.u.microcode2.data, uc);
>  
>      ret = do_platform_op(xch, &platform_op);
>  
> diff --git a/tools/misc/xen-ucode.c b/tools/misc/xen-ucode.c
> index 2c9f337b86cb..688e540943b1 100644
> --- a/tools/misc/xen-ucode.c
> +++ b/tools/misc/xen-ucode.c
> @@ -13,6 +13,8 @@
>  #include <xenctrl.h>
>  #include <getopt.h>
>  
> +#include <xen/platform.h>
> +
>  static xc_interface *xch;
>  
>  static const char intel_id[] = "GenuineIntel";
> @@ -79,7 +81,9 @@ static void usage(FILE *stream, const char *name)
>              "options:\n"
>              "  -h, --help            display this help\n"
>              "  -s, --show-cpu-info   show CPU information\n"
> -            "Usage: %s [microcode file | options]\n", name, name);
> +            "  -f, --force           skip certain checks; do not use unless\n"
> +            "you know exactly what you are doing\n"

Did you look at the produced output? Imo you want to have

            "  -f, --force           skip certain checks; do not use unless\n"
            "                        you know exactly what you are doing\n"

> +            "Usage: %s [microcode file [-f,--force] | options]\n", name, name);

At least

            "Usage: %s [microcode file [-f|--force] | options]\n", name, name);

But: "options" now includes -f / --force, yet that on its own makes no sense.
I think this needs further textual clarification to properly indicate what is
valid to use and what is not.

Jan

Re: [PATCH v6 4/4] x86/ucode: Utilize ucode_force and remove opt_ucode_allow_same

[Jan Beulich]

On 25.07.2024 10:27, Fouad Hilly wrote:
> --- a/docs/misc/xen-command-line.pandoc
> +++ b/docs/misc/xen-command-line.pandoc
> @@ -2650,7 +2650,7 @@ performance.
>     Alternatively, selecting `tsx=1` will re-enable TSX at the users own risk.
>  
>  ### ucode
> -> `= List of [ <integer> | scan=<bool>, nmi=<bool>, allow-same=<bool> ]`
> +> `= List of [ <integer> | scan=<bool>, nmi=<bool> ]`
>  
>      Applicability: x86
>      Default: `nmi`
> @@ -2682,11 +2682,6 @@ precedence over `scan`.
>  stop_machine context. In NMI handler, even NMIs are blocked, which is
>  considered safer. The default value is `true`.
>  
> -'allow-same' alters the default acceptance policy for new microcode to permit
> -trying to reload the same version.  Many CPUs will actually reload microcode
> -of the same version, and this allows for easy testing of the late microcode
> -loading path.

The removal of a command line (sub)option should come with a CHANGELOG.md
entry.

> --- a/xen/arch/x86/cpu/microcode/core.c
> +++ b/xen/arch/x86/cpu/microcode/core.c
> @@ -90,6 +90,11 @@ struct ucode_mod_blob {
>      size_t size;
>  };
>  
> +struct patch_with_flags {
> +    unsigned int flags;
> +    struct microcode_patch *patch;

Pointer-to-const? If the const was omitted here just because of
microcode_free_patch(), then I think the issue should be taken care
of there.

> @@ -237,7 +238,11 @@ static DEFINE_PER_CPU(int, loading_err);
>   */
>  static cpumask_t cpu_callin_map;
>  static atomic_t cpu_out, cpu_updated;
> -static const struct microcode_patch *nmi_patch = ZERO_BLOCK_PTR;
> +static struct patch_with_flags nmi_patch_with_flags =

Could the variable name perhaps continue to be "nmi_patch"? Or be
simply "nmi_arg" or some such?

> +{
> +    .flags  = 0,

Nit: This isn't really needed.

> @@ -379,7 +386,8 @@ static int secondary_nmi_work(void)
>      return wait_for_state(LOADING_EXIT) ? 0 : -EBUSY;
>  }
>  
> -static int primary_thread_work(const struct microcode_patch *patch)
> +static int primary_thread_work(struct microcode_patch *patch,
> +                               unsigned int flags)
>  {

Why is this change needed?

> @@ -446,7 +455,8 @@ static int secondary_thread_fn(void)
>      return this_cpu(loading_err);
>  }
>  
> -static int primary_thread_fn(const struct microcode_patch *patch)
> +static int primary_thread_fn(struct microcode_patch *patch,
> +                             unsigned int flags)

Same here.

Jan

Re: [PATCH v6 2/4] x86/ucode: refactor xen-ucode to utilize getopt

[Fouad Hilly]

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

On Thu, Jul 25, 2024 at 9:41 AM Jan Beulich <jbeulich@suse.com> wrote:

> On 25.07.2024 10:27, Fouad Hilly wrote:
> > @@ -71,12 +72,29 @@ static void show_curr_cpu(FILE *f)
> >      }
> >  }
> >
> > +static void usage(FILE *stream, const char *name)
> > +{
> > +    fprintf(stream,
> > +            "%s: Xen microcode updating tool\n"
> > +            "options:\n"
> > +            "  -h, --help            display this help\n"
> > +            "  -s, --show-cpu-info   show CPU information\n"
> > +            "Usage: %s [microcode file | options]\n", name, name);
>
> Oh, and: While I gave this precise layout as an outline, it wasn't really
> meant to be used literally. Note how "microcode" and "file" now suggest
> there need to be two separate command line elements. Perhaps using
> "microcode-file" instead may already make this less ambiguous.
>

Yes indeed, I will fix in v7:
  static void usage(FILE *stream, const char *name)
{
    fprintf(stream,
            "%s: Xen microcode updating tool\n"
            "Usage: %s [options | microcode-file]\n"
            "options:\n"
            "  -h, --help                       display this help\n"
            "  -s, --show-cpu-info              show CPU information\n"
            "  -f, --force <microcode-file>     skip certain checks; do not
\n"
            "                                   use unless you know exactly
\n"
            "                                   what you are doing\n",
            name, name);
    show_curr_cpu(stream);
}

>
> Jan
>

Thanks,

Fouad

[-- Attachment #2: Type: text/html, Size: 6790 bytes --]

Re: [PATCH v6 3/4] x86/ucode: Introduce --force option to xen-ucode

[Fouad Hilly]

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

On Thu, Jul 25, 2024 at 9:44 AM Jan Beulich <jbeulich@suse.com> wrote:

> On 25.07.2024 10:27, Fouad Hilly wrote:
> > Introduce --force option to xen-ucode to force skipping microcode
> version check, which
> > allows the user to update x86 microcode even if both versions are the
> same or downgrade.
> > xc_microcode_update() refactored to accept flags and utilize
> xenpf_microcode_update2.
> >
> > Signed-off-by: Fouad Hilly <fouad.hilly@cloud.com>
> > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > ---
> > [v6]
> > 1- Fix usage() output for -f option to be explicitly wrapped for 80
> character width
> > [v5]
> > 1- Update commit message.
> > 2- Re-phrase --force option description.
> > [v4]
> > 1- Add --force to xen-ucode options.
> > 2- Update xc_microcode_update() to accept and handle flags.
> > ---
> >  tools/include/xenctrl.h   |  3 ++-
> >  tools/libs/ctrl/xc_misc.c | 12 +++++++-----
> >  tools/misc/xen-ucode.c    | 15 ++++++++++++---
> >  3 files changed, 21 insertions(+), 9 deletions(-)
> >
> > diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
> > index 9ceca0cffc2f..2c4608c09ab0 100644
> > --- a/tools/include/xenctrl.h
> > +++ b/tools/include/xenctrl.h
> > @@ -1171,7 +1171,8 @@ typedef uint32_t xc_node_to_node_dist_t;
> >  int xc_physinfo(xc_interface *xch, xc_physinfo_t *info);
> >  int xc_cputopoinfo(xc_interface *xch, unsigned *max_cpus,
> >                     xc_cputopo_t *cputopo);
> > -int xc_microcode_update(xc_interface *xch, const void *buf, size_t len);
> > +int xc_microcode_update(xc_interface *xch, const void *buf,
> > +                        size_t len, unsigned int flags);
> >  int xc_get_cpu_version(xc_interface *xch, struct xenpf_pcpu_version
> *cpu_ver);
> >  int xc_get_ucode_revision(xc_interface *xch,
> >                            struct xenpf_ucode_revision *ucode_rev);
> > diff --git a/tools/libs/ctrl/xc_misc.c b/tools/libs/ctrl/xc_misc.c
> > index 50282fd60dcc..6a60216bda03 100644
> > --- a/tools/libs/ctrl/xc_misc.c
> > +++ b/tools/libs/ctrl/xc_misc.c
> > @@ -203,11 +203,12 @@ int xc_physinfo(xc_interface *xch,
> >      return 0;
> >  }
> >
> > -int xc_microcode_update(xc_interface *xch, const void *buf, size_t len)
> > +int xc_microcode_update(xc_interface *xch, const void *buf,
> > +                        size_t len, unsigned int flags)
> >  {
> >      int ret;
> >      struct xen_platform_op platform_op = {};
> > -    DECLARE_HYPERCALL_BUFFER(struct xenpf_microcode_update, uc);
> > +    DECLARE_HYPERCALL_BUFFER(struct xenpf_microcode_update2, uc);
> >
> >      uc = xc_hypercall_buffer_alloc(xch, uc, len);
> >      if ( uc == NULL )
> > @@ -215,9 +216,10 @@ int xc_microcode_update(xc_interface *xch, const
> void *buf, size_t len)
> >
> >      memcpy(uc, buf, len);
> >
> > -    platform_op.cmd = XENPF_microcode_update;
> > -    platform_op.u.microcode.length = len;
> > -    set_xen_guest_handle(platform_op.u.microcode.data, uc);
> > +    platform_op.cmd = XENPF_microcode_update2;
> > +    platform_op.u.microcode2.length = len;
> > +    platform_op.u.microcode2.flags = flags;
> > +    set_xen_guest_handle(platform_op.u.microcode2.data, uc);
> >
> >      ret = do_platform_op(xch, &platform_op);
> >
> > diff --git a/tools/misc/xen-ucode.c b/tools/misc/xen-ucode.c
> > index 2c9f337b86cb..688e540943b1 100644
> > --- a/tools/misc/xen-ucode.c
> > +++ b/tools/misc/xen-ucode.c
> > @@ -13,6 +13,8 @@
> >  #include <xenctrl.h>
> >  #include <getopt.h>
> >
> > +#include <xen/platform.h>
> > +
> >  static xc_interface *xch;
> >
> >  static const char intel_id[] = "GenuineIntel";
> > @@ -79,7 +81,9 @@ static void usage(FILE *stream, const char *name)
> >              "options:\n"
> >              "  -h, --help            display this help\n"
> >              "  -s, --show-cpu-info   show CPU information\n"
> > -            "Usage: %s [microcode file | options]\n", name, name);
> > +            "  -f, --force           skip certain checks; do not use
> unless\n"
> > +            "you know exactly what you are doing\n"
>
> Did you look at the produced output? Imo you want to have
>
>             "  -f, --force           skip certain checks; do not use
> unless\n"
>             "                        you know exactly what you are doing\n"
>
> > +            "Usage: %s [microcode file [-f,--force] | options]\n",
> name, name);
>
> At least
>
>             "Usage: %s [microcode file [-f|--force] | options]\n", name,
> name);
>
> But: "options" now includes -f / --force, yet that on its own makes no
> sense.
> I think this needs further textual clarification to properly indicate what
> is
> valid to use and what is not.
>

Will be fixed in v7:
static void usage(FILE *stream, const char *name)
{
    fprintf(stream,
            "%s: Xen microcode updating tool\n"
            "Usage: %s [options | microcode-file]\n"
            "options:\n"
            "  -h, --help                       display this help\n"
            "  -s, --show-cpu-info              show CPU information\n"
            "  -f, --force <microcode-file>     skip certain checks; do not
\n"
            "                                   use unless you know exactly
\n"
            "                                   what you are doing\n",
            name, name);
    show_curr_cpu(stream);
}

>
> Jan
>

Thanks,

Fouad

[-- Attachment #2: Type: text/html, Size: 9321 bytes --]

Re: [PATCH v6 4/4] x86/ucode: Utilize ucode_force and remove opt_ucode_allow_same

[Fouad Hilly]

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

On Mon, Jul 29, 2024 at 12:30 PM Jan Beulich <jbeulich@suse.com> wrote:

> On 25.07.2024 10:27, Fouad Hilly wrote:
> > --- a/docs/misc/xen-command-line.pandoc
> > +++ b/docs/misc/xen-command-line.pandoc
> > @@ -2650,7 +2650,7 @@ performance.
> >     Alternatively, selecting `tsx=1` will re-enable TSX at the users own
> risk.
> >
> >  ### ucode
> > -> `= List of [ <integer> | scan=<bool>, nmi=<bool>, allow-same=<bool> ]`
> > +> `= List of [ <integer> | scan=<bool>, nmi=<bool> ]`
> >
> >      Applicability: x86
> >      Default: `nmi`
> > @@ -2682,11 +2682,6 @@ precedence over `scan`.
> >  stop_machine context. In NMI handler, even NMIs are blocked, which is
> >  considered safer. The default value is `true`.
> >
> > -'allow-same' alters the default acceptance policy for new microcode to
> permit
> > -trying to reload the same version.  Many CPUs will actually reload
> microcode
> > -of the same version, and this allows for easy testing of the late
> microcode
> > -loading path.
>
> The removal of a command line (sub)option should come with a CHANGELOG.md
> entry.
>

Will be fixed in v7

>
> > --- a/xen/arch/x86/cpu/microcode/core.c
> > +++ b/xen/arch/x86/cpu/microcode/core.c
> > @@ -90,6 +90,11 @@ struct ucode_mod_blob {
> >      size_t size;
> >  };
> >
> > +struct patch_with_flags {
> > +    unsigned int flags;
> > +    struct microcode_patch *patch;
>
> Pointer-to-const? If the const was omitted here just because of
> microcode_free_patch(), then I think the issue should be taken care
> of there.
>

This struct is required as is, I initially added a similar struct with
const (which was removed in v6).
updated control_thread_fn()
-static int control_thread_fn(const struct microcode_patch *patch)
+static int control_thread_fn(struct microcode_patch *patch,
+                             unsigned int flags)

> @@ -237,7 +238,11 @@ static DEFINE_PER_CPU(int, loading_err);
> >   */
> >  static cpumask_t cpu_callin_map;
> >  static atomic_t cpu_out, cpu_updated;
> > -static const struct microcode_patch *nmi_patch = ZERO_BLOCK_PTR;
> > +static struct patch_with_flags nmi_patch_with_flags =
>
> Could the variable name perhaps continue to be "nmi_patch"? Or be
> simply "nmi_arg" or some such?
>

Sure, will keep it as nmi_patch

>
> > +{
> > +    .flags  = 0,
>
> Nit: This isn't really needed.
>
> > @@ -379,7 +386,8 @@ static int secondary_nmi_work(void)
> >      return wait_for_state(LOADING_EXIT) ? 0 : -EBUSY;
> >  }
> >
> > -static int primary_thread_work(const struct microcode_patch *patch)
> > +static int primary_thread_work(struct microcode_patch *patch,
> > +                               unsigned int flags)
> >  {
>
> Why is this change needed?
>
This will be reverted in v7

>
> > @@ -446,7 +455,8 @@ static int secondary_thread_fn(void)
> >      return this_cpu(loading_err);
> >  }
> >
> > -static int primary_thread_fn(const struct microcode_patch *patch)
> > +static int primary_thread_fn(struct microcode_patch *patch,
> > +                             unsigned int flags)
>
> Same here.
>

 This will be reverted in v7

>
> Jan
>

Thanks,

Fouad

[-- Attachment #2: Type: text/html, Size: 4841 bytes --]

Re: [PATCH v6 4/4] x86/ucode: Utilize ucode_force and remove opt_ucode_allow_same

[Jan Beulich]

On 19.08.2024 10:57, Fouad Hilly wrote:
> On Mon, Jul 29, 2024 at 12:30 PM Jan Beulich <jbeulich@suse.com> wrote:
>> On 25.07.2024 10:27, Fouad Hilly wrote:
>>> --- a/xen/arch/x86/cpu/microcode/core.c
>>> +++ b/xen/arch/x86/cpu/microcode/core.c
>>> @@ -90,6 +90,11 @@ struct ucode_mod_blob {
>>>      size_t size;
>>>  };
>>>
>>> +struct patch_with_flags {
>>> +    unsigned int flags;
>>> +    struct microcode_patch *patch;
>>
>> Pointer-to-const? If the const was omitted here just because of
>> microcode_free_patch(), then I think the issue should be taken care
>> of there.
> 
> This struct is required as is, I initially added a similar struct with
> const (which was removed in v6).
> updated control_thread_fn()
> -static int control_thread_fn(const struct microcode_patch *patch)
> +static int control_thread_fn(struct microcode_patch *patch,
> +                             unsigned int flags)

And why's that change necessary, other than to cater for the omitted const
in the struct?

Jan

Re: [PATCH v6 3/4] x86/ucode: Introduce --force option to xen-ucode

[Anthony PERARD]

On Mon, Aug 19, 2024 at 09:56:57AM +0100, Fouad Hilly wrote:
> On Thu, Jul 25, 2024 at 9:44 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> > On 25.07.2024 10:27, Fouad Hilly wrote:
> > > @@ -79,7 +81,9 @@ static void usage(FILE *stream, const char *name)
> > >              "options:\n"
> > >              "  -h, --help            display this help\n"
> > >              "  -s, --show-cpu-info   show CPU information\n"
> > > -            "Usage: %s [microcode file | options]\n", name, name);
> > > +            "  -f, --force           skip certain checks; do not use unless\n"
> > > +            "you know exactly what you are doing\n"
> >
> > Did you look at the produced output? Imo you want to have
> >
> >             "  -f, --force           skip certain checks; do not use unless\n"
> >             "                        you know exactly what you are doing\n"
> >
> > > +            "Usage: %s [microcode file [-f,--force] | options]\n", name, name);
> >
> > At least
> >
> >             "Usage: %s [microcode file [-f|--force] | options]\n", name, name);
> >
> > But: "options" now includes -f / --force, yet that on its own makes no sense.
> > I think this needs further textual clarification to properly indicate what is
> > valid to use and what is not.
> >
>
> Will be fixed in v7:
> static void usage(FILE *stream, const char *name)
> {
>     fprintf(stream,
>             "%s: Xen microcode updating tool\n"
>             "Usage: %s [options | microcode-file]\n"
>             "options:\n"
>             "  -h, --help                       display this help\n"
>             "  -s, --show-cpu-info              show CPU information\n"
>             "  -f, --force <microcode-file>     skip certain checks; do not
> \n"

If I recall correctly, "--force" doesn't take any argument, so this
usage is misleading. One could be tempted to execute `./xen-ucode
-fmicrocode` or event `./xen-ucode --force -microcode` and expect it to
work with files "microcode" or "-microcode" but instead I think getopt()
is just going to return an error.

Instead of writing "--force <microcode-file>", could you change the help
text, with something like "skip certain checks when applying microcode"?

>             "                                   use unless you know exactly
> \n"
>             "                                   what you are doing\n",
>             name, name);
>     show_curr_cpu(stream);

Cheers,

--

Anthony Perard | Vates XCP-ng Developer

XCP-ng & Xen Orchestra - Vates solutions

web: https://vates.tech

Re: [PATCH v6 3/4] x86/ucode: Introduce --force option to xen-ucode

[Fouad Hilly]

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

On Mon, Aug 19, 2024 at 10:47 AM Anthony PERARD <anthony.perard@vates.tech>
wrote:

> On Mon, Aug 19, 2024 at 09:56:57AM +0100, Fouad Hilly wrote:
> > On Thu, Jul 25, 2024 at 9:44 AM Jan Beulich <jbeulich@suse.com> wrote:
> >
> > > On 25.07.2024 10:27, Fouad Hilly wrote:
> > > > @@ -79,7 +81,9 @@ static void usage(FILE *stream, const char *name)
> > > >              "options:\n"
> > > >              "  -h, --help            display this help\n"
> > > >              "  -s, --show-cpu-info   show CPU information\n"
> > > > -            "Usage: %s [microcode file | options]\n", name, name);
> > > > +            "  -f, --force           skip certain checks; do not
> use unless\n"
> > > > +            "you know exactly what you are doing\n"
> > >
> > > Did you look at the produced output? Imo you want to have
> > >
> > >             "  -f, --force           skip certain checks; do not use
> unless\n"
> > >             "                        you know exactly what you are
> doing\n"
> > >
> > > > +            "Usage: %s [microcode file [-f,--force] | options]\n",
> name, name);
> > >
> > > At least
> > >
> > >             "Usage: %s [microcode file [-f|--force] | options]\n",
> name, name);
> > >
> > > But: "options" now includes -f / --force, yet that on its own makes no
> sense.
> > > I think this needs further textual clarification to properly indicate
> what is
> > > valid to use and what is not.
> > >
> >
> > Will be fixed in v7:
> > static void usage(FILE *stream, const char *name)
> > {
> >     fprintf(stream,
> >             "%s: Xen microcode updating tool\n"
> >             "Usage: %s [options | microcode-file]\n"
> >             "options:\n"
> >             "  -h, --help                       display this help\n"
> >             "  -s, --show-cpu-info              show CPU information\n"
> >             "  -f, --force <microcode-file>     skip certain checks; do
> not
> > \n"
>
> If I recall correctly, "--force" doesn't take any argument, so this
> usage is misleading. One could be tempted to execute `./xen-ucode
> -fmicrocode` or event `./xen-ucode --force -microcode` and expect it to
> work with files "microcode" or "-microcode" but instead I think getopt()
> is just going to return an error.
>
> Instead of writing "--force <microcode-file>", could you change the help
> text, with something like "skip certain checks when applying microcode"?
>

Sure, can be done in v7:

static void usage(FILE *stream, const char *name)
{
    fprintf(stream,
            "%s: Xen microcode updating tool\n"
            "Usage: %s [options | microcode-file]\n"
            "options:\n"
            "  -h, --help               display this help\n"
            "  -s, --show-cpu-info      show CPU information\n",
            "  -f, --force              skip certain checks when applying\n"
            "                           microcode; do not use unless you
know\n"
            "                           exactly what you are doing\n",
            name, name);
    show_curr_cpu(stream);
}

>
> >             "                                   use unless you know
> exactly
> > \n"
> >             "                                   what you are doing\n",
> >             name, name);
> >     show_curr_cpu(stream);
>
> Cheers,
>
> --
>
> Anthony Perard | Vates XCP-ng Developer
>
> XCP-ng & Xen Orchestra - Vates solutions
>
> web: https://vates.tech


Thanks,

Fouad

[-- Attachment #2: Type: text/html, Size: 7294 bytes --]

Re: [PATCH v6 4/4] x86/ucode: Utilize ucode_force and remove opt_ucode_allow_same

[Fouad Hilly]

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

On Mon, Aug 19, 2024 at 10:22 AM Jan Beulich <jbeulich@suse.com> wrote:

> On 19.08.2024 10:57, Fouad Hilly wrote:
> > On Mon, Jul 29, 2024 at 12:30 PM Jan Beulich <jbeulich@suse.com> wrote:
> >> On 25.07.2024 10:27, Fouad Hilly wrote:
> >>> --- a/xen/arch/x86/cpu/microcode/core.c
> >>> +++ b/xen/arch/x86/cpu/microcode/core.c
> >>> @@ -90,6 +90,11 @@ struct ucode_mod_blob {
> >>>      size_t size;
> >>>  };
> >>>
> >>> +struct patch_with_flags {
> >>> +    unsigned int flags;
> >>> +    struct microcode_patch *patch;
> >>
> >> Pointer-to-const? If the const was omitted here just because of
> >> microcode_free_patch(), then I think the issue should be taken care
> >> of there.
> >
> > This struct is required as is, I initially added a similar struct with
> > const (which was removed in v6).
> > updated control_thread_fn()
> > -static int control_thread_fn(const struct microcode_patch *patch)
> > +static int control_thread_fn(struct microcode_patch *patch,
> > +                             unsigned int flags)
>
> And why's that change necessary, other than to cater for the omitted const
> in the struct?
>

I see your point, I will keep const and update microcode_free_patch() as
needed in v7.

>
> Jan
>


Thanks,

Fouad

[-- Attachment #2: Type: text/html, Size: 2086 bytes --]