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

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

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

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

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