[PATCH v2 0/5] x86/xen-ucode: Introduce --force option

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

Fouad Hilly (5):
  x86: Update x86 low level version check of microcode
  x86: Refactor microcode_update() hypercall with flags
  x86: Add usage() to print out usage message
  x86: Use getopt to handle command line args
  x86: Add --force option to xen-ucode to override microcode version
    check

 tools/include/xenctrl.h              |  3 +-
 tools/libs/ctrl/xc_misc.c            | 13 +++++-
 tools/misc/xen-ucode.c               | 62 ++++++++++++++++++++++------
 xen/arch/x86/cpu/microcode/amd.c     |  8 +---
 xen/arch/x86/cpu/microcode/core.c    | 14 +++++--
 xen/arch/x86/cpu/microcode/intel.c   | 11 ++---
 xen/arch/x86/include/asm/microcode.h |  3 +-
 xen/arch/x86/platform_hypercall.c    | 12 +++++-
 xen/include/public/platform.h        |  6 +++
 9 files changed, 97 insertions(+), 35 deletions(-)

-- 
2.42.0

[PATCH v2 1/5] x86: Update x86 low level version check of microcode

[Fouad Hilly]

Update microcode version check at Intel and AMD Level by:
Preventing the low level code from sending errors if the microcode
version provided is not a newer version. Other errors will be sent like before.
When the provided microcode version is the same as the current one, code
to point to microcode provided.
Microcode version check happens at higher and common level in core.c.
Keep all the required code at low level that checks for signature and CPU compatibility

[v2]
Update message description to better describe the changes

Signed-off-by: Fouad Hilly <fouad.hilly@cloud.com>
---
 xen/arch/x86/cpu/microcode/amd.c   |  8 ++------
 xen/arch/x86/cpu/microcode/intel.c | 11 +++--------
 2 files changed, 5 insertions(+), 14 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/amd.c b/xen/arch/x86/cpu/microcode/amd.c
index 75fc84e445ce..4f805f662701 100644
--- a/xen/arch/x86/cpu/microcode/amd.c
+++ b/xen/arch/x86/cpu/microcode/amd.c
@@ -383,12 +383,8 @@ static struct microcode_patch *cf_check cpu_request_microcode(
                 goto skip;
             }
 
-            /*
-             * If the new ucode covers current CPU, compare ucodes and store the
-             * one with higher revision.
-             */
-            if ( (microcode_fits(mc->patch) != MIS_UCODE) &&
-                 (!saved || (compare_header(mc->patch, saved) == NEW_UCODE)) )
+            /* If the provided ucode covers current CPU, then store its revision. */
+            if ( (microcode_fits(mc->patch) != MIS_UCODE) && !saved )
             {
                 saved = mc->patch;
                 saved_size = mc->len;
diff --git a/xen/arch/x86/cpu/microcode/intel.c b/xen/arch/x86/cpu/microcode/intel.c
index 060c529a6e5d..e65c02a57987 100644
--- a/xen/arch/x86/cpu/microcode/intel.c
+++ b/xen/arch/x86/cpu/microcode/intel.c
@@ -294,8 +294,7 @@ static int cf_check apply_microcode(const struct microcode_patch *patch)
 
     result = microcode_update_match(patch);
 
-    if ( result != NEW_UCODE &&
-         !(opt_ucode_allow_same && result == SAME_UCODE) )
+    if ( result != NEW_UCODE && result != SAME_UCODE )
         return -EINVAL;
 
     wbinvd();
@@ -354,12 +353,8 @@ static struct microcode_patch *cf_check cpu_request_microcode(
         if ( error )
             break;
 
-        /*
-         * If the new update covers current CPU, compare updates and store the
-         * one with higher revision.
-         */
-        if ( (microcode_update_match(mc) != MIS_UCODE) &&
-             (!saved || compare_revisions(saved->rev, mc->rev) == NEW_UCODE) )
+        /* If the provided ucode covers current CPU, then store its revision. */
+        if ( (microcode_update_match(mc) != MIS_UCODE) && !saved )
             saved = mc;
 
         buf  += blob_size;
-- 
2.42.0

[PATCH v2 2/5] x86: Refactor microcode_update() hypercall with flags

[Fouad Hilly]

Refactor microcode_update() hypercall by adding flags field.
Introduce XENPF_microcode_update2 hypercall to handle flags field.
struct xenpf_microcode_update updated to have uint32_t flags at
the end of the sturcture.

[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.

Signed-off-by: Fouad Hilly <fouad.hilly@cloud.com>
---
 xen/arch/x86/cpu/microcode/core.c    | 14 +++++++++++---
 xen/arch/x86/include/asm/microcode.h |  3 ++-
 xen/arch/x86/platform_hypercall.c    | 12 +++++++++++-
 xen/include/public/platform.h        |  6 ++++++
 4 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/cpu/microcode/core.c b/xen/arch/x86/cpu/microcode/core.c
index 1c9f66ea8a0f..99b651d8c3a1 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"
 
 /*
@@ -100,6 +102,8 @@ static bool ucode_in_nmi = true;
 
 bool __read_mostly opt_ucode_allow_same;
 
+static unsigned int ucode_update_flags = 0;
+
 /* Protected by microcode_mutex */
 static struct microcode_patch *microcode_cache;
 
@@ -580,6 +584,7 @@ static long cf_check microcode_update_helper(void *data)
     struct ucode_buf *buffer = data;
     unsigned int cpu, updated;
     struct microcode_patch *patch;
+    bool ucode_force_flag = ucode_update_flags == XENPF_UCODE_FLAG_FORCE_SET;
 
     /* cpu_online_map must not change during update */
     if ( !get_cpu_maps() )
@@ -633,12 +638,12 @@ static long cf_check microcode_update_helper(void *data)
                                   microcode_cache);
 
         if ( result != NEW_UCODE &&
-             !(opt_ucode_allow_same && result == SAME_UCODE) )
+             !((opt_ucode_allow_same || ucode_force_flag) && 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)" : "");
+                   (opt_ucode_allow_same || ucode_force_flag) ? " (or the same)" : "");
             microcode_free_patch(patch);
             ret = -ENOENT;
 
@@ -708,7 +713,8 @@ 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;
@@ -731,6 +737,8 @@ int microcode_update(XEN_GUEST_HANDLE(const_void) buf, unsigned long len)
     }
     buffer->len = len;
 
+    ucode_update_flags = flags;
+
     /*
      * Always queue microcode_update_helper() on CPU0.  Most of the logic
      * won't care, but the update of the Raw CPU policy wants to (re)run on
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..3b29ede8b316 100644
--- a/xen/arch/x86/platform_hypercall.c
+++ b/xen/arch/x86/platform_hypercall.c
@@ -311,7 +311,17 @@ 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.microcode.data);
+
+        ret = microcode_update(data, op->u.microcode.length, op->u.microcode.flags);
         break;
     }
 
diff --git a/xen/include/public/platform.h b/xen/include/public/platform.h
index 15777b541690..cc19b2956b46 100644
--- a/xen/include/public/platform.h
+++ b/xen/include/public/platform.h
@@ -99,6 +99,9 @@ struct xenpf_microcode_update {
     /* IN variables. */
     XEN_GUEST_HANDLE(const_void) data;/* Pointer to microcode data */
     uint32_t length;                  /* Length of microcode data. */
+    uint32_t flags;                   /* Flags to be passed with ucode. */
+/* Force to skip microcode version check when set */
+#define XENPF_UCODE_FLAG_FORCE_SET     1
 };
 typedef struct xenpf_microcode_update xenpf_microcode_update_t;
 DEFINE_XEN_GUEST_HANDLE(xenpf_microcode_update_t);
@@ -624,6 +627,9 @@ 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
+
 /*
  * ` enum neg_errnoval
  * ` HYPERVISOR_platform_op(const struct xen_platform_op*);
-- 
2.42.0

[PATCH v2 3/5] x86: Add usage() to print out usage message

[Fouad Hilly]

Refactor xen-ucode tool by adding usage() to handle usage\help messages
As we add more command options this will keep help\usage messages in common block

[v2]
1- Improved message description.
2- Fixed formatting and indentation.
3- Error message to print to stderr.

Signed-off-by: Fouad Hilly <fouad.hilly@cloud.com>
---
 tools/misc/xen-ucode.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/tools/misc/xen-ucode.c b/tools/misc/xen-ucode.c
index c6ae6498d659..0c0b2337b4ea 100644
--- a/tools/misc/xen-ucode.c
+++ b/tools/misc/xen-ucode.c
@@ -17,6 +17,15 @@ static xc_interface *xch;
 static const char intel_id[] = "GenuineIntel";
 static const char   amd_id[] = "AuthenticAMD";
 
+static void usage(const char *name)
+{
+    printf("%s: Xen microcode updating tool\n"
+           "Usage: %s [microcode file] [options]\n"
+           "Options:\n"
+           "show-cou-info   show CPU information and exit\n",
+           name, name);
+}
+
 static void show_curr_cpu(FILE *f)
 {
     int ret;
-- 
2.42.0

[PATCH v2 4/5] x86: Use getopt to handle command line args

[Fouad Hilly]

Use getopt_long() to handle command line arguments.
Introduce ext_err for common exit with errors.

[v2]
1- Removed unnecessary NULL initialization.
2- Used static at the beginning of type struct declaration.
3- Corrected switch\case indentations.
4- Removed redundant checks.
5- Corrected label indentation.

Signed-off-by: Fouad Hilly <fouad.hilly@cloud.com>
---
 tools/misc/xen-ucode.c | 43 ++++++++++++++++++++++++++++++------------
 1 file changed, 31 insertions(+), 12 deletions(-)

diff --git a/tools/misc/xen-ucode.c b/tools/misc/xen-ucode.c
index 0c0b2337b4ea..e3c1943e3633 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;
 
@@ -22,7 +23,8 @@ static void usage(const char *name)
     printf("%s: Xen microcode updating tool\n"
            "Usage: %s [microcode file] [options]\n"
            "Options:\n"
-           "show-cou-info   show CPU information and exit\n",
+           "  -h, --help            display this help and exit\n"
+           "  -s, --show-cpu-info   show CPU information and exit\n",
            name, name);
 }
 
@@ -86,6 +88,13 @@ int main(int argc, char *argv[])
     char *filename, *buf;
     size_t len;
     struct stat st;
+    int opt;
+
+    static const struct option options[] = {
+        {"help", no_argument, NULL, 'h'},
+        {"show-cpu-info", no_argument, NULL, 's'},
+        {NULL, no_argument, NULL, 0}
+    };
 
     xch = xc_interface_open(NULL, NULL, 0);
     if ( xch == NULL )
@@ -95,19 +104,22 @@ int main(int argc, char *argv[])
         exit(1);
     }
 
-    if ( argc < 2 )
-    {
-        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);
-    }
+    if ( argc != 2 )
+        goto ext_err;
 
-    if ( !strcmp(argv[1], "show-cpu-info") )
+    while ( (opt = getopt_long(argc, argv, "hs", options, NULL)) != -1 )
     {
-        show_curr_cpu(stdout);
-        return 0;
+        switch (opt)
+        {
+        case 'h':
+            usage(argv[0]);
+            exit(EXIT_SUCCESS);
+        case 's':
+            show_curr_cpu(stdout);
+            exit(EXIT_SUCCESS);
+        default:
+            goto ext_err;
+        }
     }
 
     filename = argv[1];
@@ -152,4 +164,11 @@ int main(int argc, char *argv[])
     close(fd);
 
     return 0;
+
+ ext_err:
+    fprintf(stderr,
+            "xen-ucode: Xen microcode updating tool\n"
+            "Usage: %s [microcode file] [options]\n", argv[0]);
+    show_curr_cpu(stderr);
+    exit(STDERR_FILENO);
 }
-- 
2.42.0

[PATCH v2 5/5] x86: Add --force option to xen-ucode to override microcode version check

[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.

[v2]
1- Changed data type from uint32_t to unsigned int.
2- Corrected line length.
3- Removed XENPF_UCODE_FLAG_FORCE_NOT_SET.
4- Corrected indentations.
5- Changed command line options to have the file name as first argument when applicable.
6- --force option doesn't require an argument anymore.
7- Used optint to access filename in argv.

Signed-off-by: Fouad Hilly <fouad.hilly@cloud.com>
---

Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
 tools/include/xenctrl.h   |  3 ++-
 tools/libs/ctrl/xc_misc.c | 13 +++++++++++--
 tools/misc/xen-ucode.c    | 18 +++++++++++++-----
 3 files changed, 26 insertions(+), 8 deletions(-)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 2ef8b4e05422..49d2f19c0d77 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 5ecdfa2c7934..fbc17cefa82e 100644
--- a/tools/libs/ctrl/xc_misc.c
+++ b/tools/libs/ctrl/xc_misc.c
@@ -203,7 +203,8 @@ 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 = {};
@@ -215,7 +216,15 @@ int xc_microcode_update(xc_interface *xch, const void *buf, size_t len)
 
     memcpy(uc, buf, len);
 
-    platform_op.cmd = XENPF_microcode_update;
+    if ( flags > 0 )
+    {
+        platform_op.cmd = XENPF_microcode_update2;
+        platform_op.u.microcode.flags = flags;
+    }
+    else
+    {
+        platform_op.cmd = XENPF_microcode_update;
+    }
     platform_op.u.microcode.length = len;
     set_xen_guest_handle(platform_op.u.microcode.data, uc);
 
diff --git a/tools/misc/xen-ucode.c b/tools/misc/xen-ucode.c
index e3c1943e3633..4178fd2221ea 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";
@@ -24,7 +26,8 @@ static void usage(const char *name)
            "Usage: %s [microcode file] [options]\n"
            "Options:\n"
            "  -h, --help            display this help and exit\n"
-           "  -s, --show-cpu-info   show CPU information and exit\n",
+           "  -s, --show-cpu-info   show CPU information and exit\n"
+           "  -f, --force           force to skip micorocde version check\n",
            name, name);
 }
 
@@ -89,10 +92,12 @@ int main(int argc, char *argv[])
     size_t len;
     struct stat st;
     int opt;
+    uint32_t ucode_flag = 0;
 
     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}
     };
 
@@ -104,10 +109,10 @@ int main(int argc, char *argv[])
         exit(1);
     }
 
-    if ( argc != 2 )
+    if ( argc < 2 || argc > 3)
         goto ext_err;
 
-    while ( (opt = getopt_long(argc, argv, "hs", options, NULL)) != -1 )
+    while ( (opt = getopt_long(argc, argv, "hsf", options, NULL)) != -1 )
     {
         switch (opt)
         {
@@ -117,12 +122,15 @@ int main(int argc, char *argv[])
         case 's':
             show_curr_cpu(stdout);
             exit(EXIT_SUCCESS);
+        case 'f':
+            ucode_flag = XENPF_UCODE_FLAG_FORCE_SET;
+            break;
         default:
             goto ext_err;
         }
     }
 
-    filename = argv[1];
+    filename = argv[optind];
     fd = open(filename, O_RDONLY);
     if ( fd < 0 )
     {
@@ -146,7 +154,7 @@ int main(int argc, char *argv[])
         exit(1);
     }
 
-    ret = xc_microcode_update(xch, buf, len);
+    ret = xc_microcode_update(xch, buf, len, ucode_flag);
     if ( ret )
     {
         fprintf(stderr, "Failed to update microcode. (err: %s)\n",
-- 
2.42.0

Re: [PATCH v2 3/5] x86: Add usage() to print out usage message

[Jan Beulich]

On 16.04.2024 11:15, Fouad Hilly wrote:
> Refactor xen-ucode tool by adding usage() to handle usage\help messages
> As we add more command options this will keep help\usage messages in common block
> 
> [v2]
> 1- Improved message description.
> 2- Fixed formatting and indentation.
> 3- Error message to print to stderr.

I can't spot any use of stderr in the change here.

> --- a/tools/misc/xen-ucode.c
> +++ b/tools/misc/xen-ucode.c
> @@ -17,6 +17,15 @@ static xc_interface *xch;
>  static const char intel_id[] = "GenuineIntel";
>  static const char   amd_id[] = "AuthenticAMD";
>  
> +static void usage(const char *name)
> +{
> +    printf("%s: Xen microcode updating tool\n"
> +           "Usage: %s [microcode file] [options]\n"
> +           "Options:\n"
> +           "show-cou-info   show CPU information and exit\n",
> +           name, name);
> +}
> +
>  static void show_curr_cpu(FILE *f)
>  {
>      int ret;

Without a caller this is going to cause a compiler warning (unused static
function) and, with -Werror, a build failure.

Jan

Re: [PATCH v2 1/5] x86: Update x86 low level version check of microcode

[Andrew Cooper]

On 16/04/2024 10:15 am, Fouad Hilly wrote:
> Update microcode version check at Intel and AMD Level by:
> Preventing the low level code from sending errors if the microcode
> version provided is not a newer version. Other errors will be sent like before.
> When the provided microcode version is the same as the current one, code
> to point to microcode provided.
> Microcode version check happens at higher and common level in core.c.
> Keep all the required code at low level that checks for signature and CPU compatibility
>
> [v2]
> Update message description to better describe the changes
>
> Signed-off-by: Fouad Hilly <fouad.hilly@cloud.com>
> ---


As a general note, your v2/v3/etc changelog needs to go under this --- line.

~Andrew

Re: [PATCH v2 1/5] x86: Update x86 low level version check of microcode

[Jan Beulich]

On 16.04.2024 11:15, Fouad Hilly wrote:
> Update microcode version check at Intel and AMD Level by:
> Preventing the low level code from sending errors if the microcode
> version provided is not a newer version.

And why is this change (a) wanted and (b) correct?

> Other errors will be sent like before.
> When the provided microcode version is the same as the current one, code
> to point to microcode provided.

I'm afraid I can't interpret this sentence.

> Microcode version check happens at higher and common level in core.c.
> Keep all the required code at low level that checks for signature and CPU compatibility
> 
> [v2]
> Update message description to better describe the changes

This belongs ...

> Signed-off-by: Fouad Hilly <fouad.hilly@cloud.com>
> ---

... below the separator.

> --- a/xen/arch/x86/cpu/microcode/amd.c
> +++ b/xen/arch/x86/cpu/microcode/amd.c
> @@ -383,12 +383,8 @@ static struct microcode_patch *cf_check cpu_request_microcode(
>                  goto skip;
>              }
>  
> -            /*
> -             * If the new ucode covers current CPU, compare ucodes and store the
> -             * one with higher revision.
> -             */
> -            if ( (microcode_fits(mc->patch) != MIS_UCODE) &&
> -                 (!saved || (compare_header(mc->patch, saved) == NEW_UCODE)) )
> +            /* If the provided ucode covers current CPU, then store its revision. */
> +            if ( (microcode_fits(mc->patch) != MIS_UCODE) && !saved )
>              {
>                  saved = mc->patch;
>                  saved_size = mc->len;
> --- a/xen/arch/x86/cpu/microcode/intel.c
> +++ b/xen/arch/x86/cpu/microcode/intel.c
> @@ -294,8 +294,7 @@ static int cf_check apply_microcode(const struct microcode_patch *patch)
>  
>      result = microcode_update_match(patch);
>  
> -    if ( result != NEW_UCODE &&
> -         !(opt_ucode_allow_same && result == SAME_UCODE) )
> +    if ( result != NEW_UCODE && result != SAME_UCODE )
>          return -EINVAL;

Unlike the other two adjustments this one results in still permitting
only same-or-newer. How does this fit with the AMD change above and
the other Intel change ...

> @@ -354,12 +353,8 @@ static struct microcode_patch *cf_check cpu_request_microcode(
>          if ( error )
>              break;
>  
> -        /*
> -         * If the new update covers current CPU, compare updates and store the
> -         * one with higher revision.
> -         */
> -        if ( (microcode_update_match(mc) != MIS_UCODE) &&
> -             (!saved || compare_revisions(saved->rev, mc->rev) == NEW_UCODE) )
> +        /* If the provided ucode covers current CPU, then store its revision. */
> +        if ( (microcode_update_match(mc) != MIS_UCODE) && !saved )
>              saved = mc;

... here?

Jan

Re: [PATCH v2 2/5] x86: Refactor microcode_update() hypercall with flags

[Jan Beulich]

On 16.04.2024 11:15, Fouad Hilly wrote:
> Refactor microcode_update() hypercall by adding flags field.
> Introduce XENPF_microcode_update2 hypercall to handle flags field.
> struct xenpf_microcode_update updated to have uint32_t flags at
> the end of the sturcture.
> 
> [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.

Non-style comments on v1 have remained un-addressed. Specifically, to
give an example, while 1 says you now highlight the interface change,
the request was to explain why changing an existing structure is okay
(hint: it likely isn't, as the structure size changes for compat [aka
32-bit] callers).

I'm not going to give the same comments again; I'll rather expect you to
respond to them by either adjustments to the patch (or its description),
or by verbal replies.

Jan

Re: [PATCH v2 3/5] x86: Add usage() to print out usage message

[Anthony PERARD]

On Tue, Apr 16, 2024 at 10:15:44AM +0100, Fouad Hilly wrote:
> Refactor xen-ucode tool by adding usage() to handle usage\help messages
> As we add more command options this will keep help\usage messages in common block
> 
> [v2]
> 1- Improved message description.
> 2- Fixed formatting and indentation.
> 3- Error message to print to stderr.
> 
> Signed-off-by: Fouad Hilly <fouad.hilly@cloud.com>
> ---
>  tools/misc/xen-ucode.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/tools/misc/xen-ucode.c b/tools/misc/xen-ucode.c
> index c6ae6498d659..0c0b2337b4ea 100644
> --- a/tools/misc/xen-ucode.c
> +++ b/tools/misc/xen-ucode.c
> @@ -17,6 +17,15 @@ static xc_interface *xch;
>  static const char intel_id[] = "GenuineIntel";
>  static const char   amd_id[] = "AuthenticAMD";
>  
> +static void usage(const char *name)
> +{
> +    printf("%s: Xen microcode updating tool\n"
> +           "Usage: %s [microcode file] [options]\n"
> +           "Options:\n"
> +           "show-cou-info   show CPU information and exit\n",

Don't change the usage message just yet. It still is
    "Usage: %s [<microcode file> | show-cpu-info]"

The current one mean we can run one of:
    ./xen-ucode ucode.bin
    ./xen-ucode show-cpu-info

The proposed help message in this patch mean we could have one of:
    ./xen-ucode ucode.bin
    ./xen-ucode show-cpu-info
    ./xen-ucode ucode.bin show-cpu-info
But that last one is an error.

Thanks,

-- 
Anthony PERARD

Re: [PATCH v2 4/5] x86: Use getopt to handle command line args

[Anthony PERARD]

On Tue, Apr 16, 2024 at 10:15:45AM +0100, Fouad Hilly wrote:
> diff --git a/tools/misc/xen-ucode.c b/tools/misc/xen-ucode.c
> index 0c0b2337b4ea..e3c1943e3633 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;
>  
> @@ -22,7 +23,8 @@ static void usage(const char *name)
>      printf("%s: Xen microcode updating tool\n"
>             "Usage: %s [microcode file] [options]\n"
>             "Options:\n"
> -           "show-cou-info   show CPU information and exit\n",
> +           "  -h, --help            display this help and exit\n"
> +           "  -s, --show-cpu-info   show CPU information and exit\n",
>             name, name);
>  }
>  
> @@ -86,6 +88,13 @@ int main(int argc, char *argv[])
>      char *filename, *buf;
>      size_t len;
>      struct stat st;
> +    int opt;
> +
> +    static const struct option options[] = {
> +        {"help", no_argument, NULL, 'h'},
> +        {"show-cpu-info", no_argument, NULL, 's'},
> +        {NULL, no_argument, NULL, 0}
> +    };
>  
>      xch = xc_interface_open(NULL, NULL, 0);
>      if ( xch == NULL )
> @@ -95,19 +104,22 @@ int main(int argc, char *argv[])
>          exit(1);
>      }
>  
> -    if ( argc < 2 )
> -    {
> -        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);
> -    }
> +    if ( argc != 2 )
> +        goto ext_err;

Why only two arguments allowed? And why check `argc` before calling
getopt_long() ?


>  
> -    if ( !strcmp(argv[1], "show-cpu-info") )
> +    while ( (opt = getopt_long(argc, argv, "hs", options, NULL)) != -1 )
>      {
> -        show_curr_cpu(stdout);
> -        return 0;
> +        switch (opt)
> +        {
> +        case 'h':
> +            usage(argv[0]);
> +            exit(EXIT_SUCCESS);
> +        case 's':
> +            show_curr_cpu(stdout);
> +            exit(EXIT_SUCCESS);
> +        default:
> +            goto ext_err;
> +        }
>      }
>  
>      filename = argv[1];

So, after calling getopt_long(), the variable `optind` point to the next
argument that getopt_long() didn't recognize as an argument. It would be
a good time to start using it, and check that the are actually argument
left on the command line, even if in the current patch the only possible
outcome is that argv[1] has something that isn't an option.

> @@ -152,4 +164,11 @@ int main(int argc, char *argv[])
>      close(fd);
>  
>      return 0;
> +
> + ext_err:
> +    fprintf(stderr,
> +            "xen-ucode: Xen microcode updating tool\n"
> +            "Usage: %s [microcode file] [options]\n", argv[0]);

Isn't there a usage() function that we could use?

> +    show_curr_cpu(stderr);

Why call show_curr_cpu() on the error path?

> +    exit(STDERR_FILENO);

Still not an exit value.

Thanks,

-- 
Anthony PERARD

Re: [PATCH v2 5/5] x86: Add --force option to xen-ucode to override microcode version check

[Anthony PERARD]

On Tue, Apr 16, 2024 at 10:15:46AM +0100, 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.
> 
> [v2]
> 1- Changed data type from uint32_t to unsigned int.
> 2- Corrected line length.
> 3- Removed XENPF_UCODE_FLAG_FORCE_NOT_SET.
> 4- Corrected indentations.
> 5- Changed command line options to have the file name as first argument when applicable.
> 6- --force option doesn't require an argument anymore.
> 7- Used optint to access filename in argv.
> 
> Signed-off-by: Fouad Hilly <fouad.hilly@cloud.com>
> ---
> 
> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>

You might want to move that tag before the '---' separation otherwise it
wont be part of the commit message. `git am` discard every thing after
the '---' line. Also add the tag before your SOB.

> ---
>  tools/include/xenctrl.h   |  3 ++-
>  tools/libs/ctrl/xc_misc.c | 13 +++++++++++--
>  tools/misc/xen-ucode.c    | 18 +++++++++++++-----
>  3 files changed, 26 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/misc/xen-ucode.c b/tools/misc/xen-ucode.c
> index e3c1943e3633..4178fd2221ea 100644
> --- a/tools/misc/xen-ucode.c
> +++ b/tools/misc/xen-ucode.c
> @@ -24,7 +26,8 @@ static void usage(const char *name)
>             "Usage: %s [microcode file] [options]\n"

Now, that usage line is wrong. The options needs to go before the file.
That could be fix on the previous patch. With that usage line, we would
want to run `./xen-ucode ucode.bin --force`, but I don't think that
would work.

>             "Options:\n"
>             "  -h, --help            display this help and exit\n"
> -           "  -s, --show-cpu-info   show CPU information and exit\n",
> +           "  -s, --show-cpu-info   show CPU information and exit\n"
> +           "  -f, --force           force to skip micorocde version check\n",
>             name, name);
>  }
>  

Thanks,

-- 
Anthony PERARD

Re: [PATCH v2 2/5] x86: Refactor microcode_update() hypercall with flags

[Fouad Hilly]

On Mon, Apr 22, 2024 at 3:18 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 16.04.2024 11:15, Fouad Hilly wrote:
> > Refactor microcode_update() hypercall by adding flags field.
> > Introduce XENPF_microcode_update2 hypercall to handle flags field.
> > struct xenpf_microcode_update updated to have uint32_t flags at
> > the end of the sturcture.
> >
> > [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.
>
> Non-style comments on v1 have remained un-addressed. Specifically, to
> give an example, while 1 says you now highlight the interface change,
> the request was to explain why changing an existing structure is okay
> (hint: it likely isn't, as the structure size changes for compat [aka
> 32-bit] callers).

I see your point now, I will keep the stable ABI as is.
>
> I'm not going to give the same comments again; I'll rather expect you to
> respond to them by either adjustments to the patch (or its description),
> or by verbal replies.
I will respond to your V1 comment on the previous email to keep things inlined
>
> Jan

Thanks,

Fouad

Re: [PATCH v2 1/5] x86: Update x86 low level version check of microcode

[Fouad Hilly]

On Mon, Apr 22, 2024 at 3:09 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 16.04.2024 11:15, Fouad Hilly wrote:
> > Update microcode version check at Intel and AMD Level by:
> > Preventing the low level code from sending errors if the microcode
> > version provided is not a newer version.
>
> And why is this change (a) wanted and (b) correct?
I will improve the message description to cover more details and reasoning.
>
> > Other errors will be sent like before.
> > When the provided microcode version is the same as the current one, code
> > to point to microcode provided.
>
> I'm afraid I can't interpret this sentence.
"provided" is the firmware presented\provided to the code for firmware
flashing. As above, I will provide more comprehensive description.
>
> > Microcode version check happens at higher and common level in core.c.
> > Keep all the required code at low level that checks for signature and CPU compatibility
> >
> > [v2]
> > Update message description to better describe the changes
>
> This belongs ...
>
> > Signed-off-by: Fouad Hilly <fouad.hilly@cloud.com>
> > ---
>
> ... below the separator.
>
> > --- a/xen/arch/x86/cpu/microcode/amd.c
> > +++ b/xen/arch/x86/cpu/microcode/amd.c
> > @@ -383,12 +383,8 @@ static struct microcode_patch *cf_check cpu_request_microcode(
> >                  goto skip;
> >              }
> >
> > -            /*
> > -             * If the new ucode covers current CPU, compare ucodes and store the
> > -             * one with higher revision.
> > -             */
> > -            if ( (microcode_fits(mc->patch) != MIS_UCODE) &&
> > -                 (!saved || (compare_header(mc->patch, saved) == NEW_UCODE)) )
> > +            /* If the provided ucode covers current CPU, then store its revision. */
> > +            if ( (microcode_fits(mc->patch) != MIS_UCODE) && !saved )
> >              {
> >                  saved = mc->patch;
> >                  saved_size = mc->len;
> > --- a/xen/arch/x86/cpu/microcode/intel.c
> > +++ b/xen/arch/x86/cpu/microcode/intel.c
> > @@ -294,8 +294,7 @@ static int cf_check apply_microcode(const struct microcode_patch *patch)
> >
> >      result = microcode_update_match(patch);
> >
> > -    if ( result != NEW_UCODE &&
> > -         !(opt_ucode_allow_same && result == SAME_UCODE) )
> > +    if ( result != NEW_UCODE && result != SAME_UCODE )
> >          return -EINVAL;
>
> Unlike the other two adjustments this one results in still permitting
> only same-or-newer. How does this fit with the AMD change above and
> the other Intel change ...
To be fixed in V3
>
> > @@ -354,12 +353,8 @@ static struct microcode_patch *cf_check cpu_request_microcode(
> >          if ( error )
> >              break;
> >
> > -        /*
> > -         * If the new update covers current CPU, compare updates and store the
> > -         * one with higher revision.
> > -         */
> > -        if ( (microcode_update_match(mc) != MIS_UCODE) &&
> > -             (!saved || compare_revisions(saved->rev, mc->rev) == NEW_UCODE) )
> > +        /* If the provided ucode covers current CPU, then store its revision. */
> > +        if ( (microcode_update_match(mc) != MIS_UCODE) && !saved )
> >              saved = mc;
>
> ... here?
I assume this refers to the previous comment? Which will be fixed in V3
>
> Jan

Thanks,

Fouad

Re: [PATCH v2 1/5] x86: Update x86 low level version check of microcode

[Fouad Hilly]

On Thu, Apr 18, 2024 at 11:05 AM Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
>
> On 16/04/2024 10:15 am, Fouad Hilly wrote:
> > Update microcode version check at Intel and AMD Level by:
> > Preventing the low level code from sending errors if the microcode
> > version provided is not a newer version. Other errors will be sent like before.
> > When the provided microcode version is the same as the current one, code
> > to point to microcode provided.
> > Microcode version check happens at higher and common level in core.c.
> > Keep all the required code at low level that checks for signature and CPU compatibility
> >
> > [v2]
> > Update message description to better describe the changes
> >
> > Signed-off-by: Fouad Hilly <fouad.hilly@cloud.com>
> > ---
>
>
> As a general note, your v2/v3/etc changelog needs to go under this --- line.
Noted.
>
> ~Andrew

Thanks,

Fouad

Re: [PATCH v2 4/5] x86: Use getopt to handle command line args

[Fouad Hilly]

On Mon, Apr 22, 2024 at 6:48 PM Anthony PERARD <anthony.perard@cloud.com> wrote:
>
> On Tue, Apr 16, 2024 at 10:15:45AM +0100, Fouad Hilly wrote:
> > diff --git a/tools/misc/xen-ucode.c b/tools/misc/xen-ucode.c
> > index 0c0b2337b4ea..e3c1943e3633 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;
> >
> > @@ -22,7 +23,8 @@ static void usage(const char *name)
> >      printf("%s: Xen microcode updating tool\n"
> >             "Usage: %s [microcode file] [options]\n"
> >             "Options:\n"
> > -           "show-cou-info   show CPU information and exit\n",
> > +           "  -h, --help            display this help and exit\n"
> > +           "  -s, --show-cpu-info   show CPU information and exit\n",
> >             name, name);
> >  }
> >
> > @@ -86,6 +88,13 @@ int main(int argc, char *argv[])
> >      char *filename, *buf;
> >      size_t len;
> >      struct stat st;
> > +    int opt;
> > +
> > +    static const struct option options[] = {
> > +        {"help", no_argument, NULL, 'h'},
> > +        {"show-cpu-info", no_argument, NULL, 's'},
> > +        {NULL, no_argument, NULL, 0}
> > +    };
> >
> >      xch = xc_interface_open(NULL, NULL, 0);
> >      if ( xch == NULL )
> > @@ -95,19 +104,22 @@ int main(int argc, char *argv[])
> >          exit(1);
> >      }
> >
> > -    if ( argc < 2 )
> > -    {
> > -        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);
> > -    }
> > +    if ( argc != 2 )
> > +        goto ext_err;
>
> Why only two arguments allowed? And why check `argc` before calling
> getopt_long() ?
At this stage of the patch series, xen-ucode expects either firmware
file or a single argument, that is why argc should be 2.
If there is no argument or many arguments that are not supposed to be,
we exit with error other than try to parse the arguments.
>
>
> >
> > -    if ( !strcmp(argv[1], "show-cpu-info") )
> > +    while ( (opt = getopt_long(argc, argv, "hs", options, NULL)) != -1 )
> >      {
> > -        show_curr_cpu(stdout);
> > -        return 0;
> > +        switch (opt)
> > +        {
> > +        case 'h':
> > +            usage(argv[0]);
> > +            exit(EXIT_SUCCESS);
> > +        case 's':
> > +            show_curr_cpu(stdout);
> > +            exit(EXIT_SUCCESS);
> > +        default:
> > +            goto ext_err;
> > +        }
> >      }
> >
> >      filename = argv[1];
>
> So, after calling getopt_long(), the variable `optind` point to the next
> argument that getopt_long() didn't recognize as an argument. It would be
> a good time to start using it, and check that the are actually argument
> left on the command line, even if in the current patch the only possible
> outcome is that argv[1] has something that isn't an option.
>
> > @@ -152,4 +164,11 @@ int main(int argc, char *argv[])
> >      close(fd);
> >
> >      return 0;
> > +
> > + ext_err:
> > +    fprintf(stderr,
> > +            "xen-ucode: Xen microcode updating tool\n"
> > +            "Usage: %s [microcode file] [options]\n", argv[0]);
>
> Isn't there a usage() function that we could use?
As there is an error, stderr should be used, which was a comment on V1.
>
> > +    show_curr_cpu(stderr);
>
> Why call show_curr_cpu() on the error path?
Informative, but could be removed.
>
> > +    exit(STDERR_FILENO);
>
> Still not an exit value.
>
> Thanks,
>
> --
> Anthony PERARD

Thanks,

Fouad

Re: [PATCH v2 5/5] x86: Add --force option to xen-ucode to override microcode version check

[Fouad Hilly]

On Mon, Apr 22, 2024 at 6:57 PM Anthony PERARD <anthony.perard@cloud.com> wrote:
>
> On Tue, Apr 16, 2024 at 10:15:46AM +0100, 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.
> >
> > [v2]
> > 1- Changed data type from uint32_t to unsigned int.
> > 2- Corrected line length.
> > 3- Removed XENPF_UCODE_FLAG_FORCE_NOT_SET.
> > 4- Corrected indentations.
> > 5- Changed command line options to have the file name as first argument when applicable.
> > 6- --force option doesn't require an argument anymore.
> > 7- Used optint to access filename in argv.
> >
> > Signed-off-by: Fouad Hilly <fouad.hilly@cloud.com>
> > ---
> >
> > Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
>
> You might want to move that tag before the '---' separation otherwise it
> wont be part of the commit message. `git am` discard every thing after
> the '---' line. Also add the tag before your SOB.
Noted.
>
> > ---
> >  tools/include/xenctrl.h   |  3 ++-
> >  tools/libs/ctrl/xc_misc.c | 13 +++++++++++--
> >  tools/misc/xen-ucode.c    | 18 +++++++++++++-----
> >  3 files changed, 26 insertions(+), 8 deletions(-)
> >
> > diff --git a/tools/misc/xen-ucode.c b/tools/misc/xen-ucode.c
> > index e3c1943e3633..4178fd2221ea 100644
> > --- a/tools/misc/xen-ucode.c
> > +++ b/tools/misc/xen-ucode.c
> > @@ -24,7 +26,8 @@ static void usage(const char *name)
> >             "Usage: %s [microcode file] [options]\n"
>
> Now, that usage line is wrong. The options needs to go before the file.
I am not sure what you mean "wrong"? from parsing perspective, both
scenarios can be successfully executed:
xen-ucode firmware-file --force
xen-ucode --force firmware-file
it becomes wrong if there is an argument expects the file as an input.
> That could be fix on the previous patch. With that usage line, we would
> want to run `./xen-ucode ucode.bin --force`, but I don't think that
> would work.
>
> >             "Options:\n"
> >             "  -h, --help            display this help and exit\n"
> > -           "  -s, --show-cpu-info   show CPU information and exit\n",
> > +           "  -s, --show-cpu-info   show CPU information and exit\n"
> > +           "  -f, --force           force to skip micorocde version check\n",
> >             name, name);
> >  }
> >
>
> Thanks,
>
> --
> Anthony PERARD

Thanks,

Fouad

Re: [PATCH v2 4/5] x86: Use getopt to handle command line args

[Anthony PERARD]

On Tue, Apr 23, 2024 at 04:16:10PM +0100, Fouad Hilly wrote:
> On Mon, Apr 22, 2024 at 6:48 PM Anthony PERARD <anthony.perard@cloud.com> wrote:
> > On Tue, Apr 16, 2024 at 10:15:45AM +0100, Fouad Hilly wrote:
> > > +    if ( argc != 2 )
> > > +        goto ext_err;
> >
> > Why only two arguments allowed? And why check `argc` before calling
> > getopt_long() ?
> At this stage of the patch series, xen-ucode expects either firmware
> file or a single argument, that is why argc should be 2.
> If there is no argument or many arguments that are not supposed to be,
> we exit with error other than try to parse the arguments.

Right, but what's the point of introducing getopt_long() if we are not
going to use it? The patch tries to make it nicer to introduce more
options to the tool but then put an extra check that make this actually
harder. This condition is even change in the next patch, that's one more
reason that says that it's a wrong check.

You can let getopt_long() parse all the options, deal with them, then
you can deal with the nonoptions after that, and check that's there's
only one nonoption.

> > > + ext_err:
> > > +    fprintf(stderr,
> > > +            "xen-ucode: Xen microcode updating tool\n"
> > > +            "Usage: %s [microcode file] [options]\n", argv[0]);
> >
> > Isn't there a usage() function that we could use?
> As there is an error, stderr should be used, which was a comment on V1.
> >
> > > +    show_curr_cpu(stderr);
> >
> > Why call show_curr_cpu() on the error path?
> Informative, but could be removed.

We are on the error path, it's looks like the usage message is printed
before the cpu information, so if the error is due to wrong options
then that information is lost. We should print why there's an error, not
much else would be useful. Error messages should be as late as possible
or they are getting lost in the scroll of the terminal. So yes, it's
probably best to remove.

Thanks,

-- 
Anthony PERARD

Re: [PATCH v2 5/5] x86: Add --force option to xen-ucode to override microcode version check

[Anthony PERARD]

On Tue, Apr 23, 2024 at 04:26:53PM +0100, Fouad Hilly wrote:
> On Mon, Apr 22, 2024 at 6:57 PM Anthony PERARD <anthony.perard@cloud.com> wrote:
> > On Tue, Apr 16, 2024 at 10:15:46AM +0100, Fouad Hilly wrote:
> > > diff --git a/tools/misc/xen-ucode.c b/tools/misc/xen-ucode.c
> > > index e3c1943e3633..4178fd2221ea 100644
> > > --- a/tools/misc/xen-ucode.c
> > > +++ b/tools/misc/xen-ucode.c
> > > @@ -24,7 +26,8 @@ static void usage(const char *name)
> > >             "Usage: %s [microcode file] [options]\n"
> >
> > Now, that usage line is wrong. The options needs to go before the file.
> I am not sure what you mean "wrong"? from parsing perspective, both
> scenarios can be successfully executed:
> xen-ucode firmware-file --force
> xen-ucode --force firmware-file
> it becomes wrong if there is an argument expects the file as an input.

Oh, sorry, I didn't know that getopt() would look for options on all
arguments, even when separated by nonoptions. I'm used to CLI programs
that takes options first, then files, even if that might work with a
different order.

Cheers,

-- 
Anthony PERARD