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

Fouad Hilly posted 5 patches 7 months, 1 week ago
There is a newer version of this series
[PATCH v2 5/5] x86: Add --force option to xen-ucode to override microcode version check
Posted by Fouad Hilly 7 months, 1 week ago
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 5/5] x86: Add --force option to xen-ucode to override microcode version check
Posted by Anthony PERARD 7 months ago
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 5/5] x86: Add --force option to xen-ucode to override microcode version check
Posted by Fouad Hilly 7 months ago
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 5/5] x86: Add --force option to xen-ucode to override microcode version check
Posted by Anthony PERARD 7 months ago
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