Mailing List Archive

[PATCH 5/5] x86: Add --force option to xen-ucode to override microcode version check
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.

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

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

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 2ef8b4e05422..5dbe3e63374a 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -1171,7 +1171,7 @@ 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, uint32_t 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..edce45bc2a17 100644
--- a/tools/libs/ctrl/xc_misc.c
+++ b/tools/libs/ctrl/xc_misc.c
@@ -203,7 +203,7 @@ 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, uint32_t flags)
{
int ret;
struct xen_platform_op platform_op = {};
@@ -215,7 +215,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 > XENPF_UCODE_FLAG_FORCE_NOT_SET )
+ {
+ 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 9bde991c5df5..469ce3299381 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";
@@ -21,10 +23,11 @@ static const char amd_id[] = "AuthenticAMD";
static void usage(const char *name)
{
printf("%s: Xen microcode updating tool\n"
- "Usage: %s [<microcode file> | --show-cpu-info]\n"
+ "Usage: %s [[--force] <microcode file> | --show-cpu-info]\n"
"\n"
" -h, --help display this help and exit\n"
" -s, --show-cpu-info show CPU information and exit\n"
+ " -f, --force force to skip micorocde version check\n"
"\n"
, name, name);
}
@@ -89,11 +92,13 @@ int main(int argc, char *argv[])
char *filename = NULL, *buf;
size_t len;
struct stat st;
+ uint32_t ucode_flag = XENPF_UCODE_FLAG_FORCE_NOT_SET;
int opt;

const static struct option options[] = {
{"help", no_argument, NULL, 'h'},
{"show-cpu-info", no_argument, NULL, 's'},
+ {"force", required_argument, NULL, 'f'},
{NULL, no_argument, NULL, 0}
};

@@ -105,10 +110,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)
{
@@ -120,12 +125,17 @@ int main(int argc, char *argv[])
goto ext_err;
show_curr_cpu(stdout);
exit(EXIT_SUCCESS);
+ case 'f':
+ ucode_flag = XENPF_UCODE_FLAG_FORCE_SET;
+ filename = optarg;
+ break;
default:
goto ext_err;
}
}

- filename = argv[1];
+ if ( argc == 2 )
+ filename = argv[1];
if ( filename == NULL )
{
printf("File name error\n");
@@ -155,7 +165,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 5/5] x86: Add --force option to xen-ucode to override microcode version check [ In reply to ]
On 05.04.2024 14:11, Fouad Hilly wrote:
> --- a/tools/libs/ctrl/xc_misc.c
> +++ b/tools/libs/ctrl/xc_misc.c
> @@ -203,7 +203,7 @@ 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, uint32_t flags)

I don't think uint32_t needs using here; unsigned int will be just fine. The
line also looks a little long.

> @@ -215,7 +215,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 > XENPF_UCODE_FLAG_FORCE_NOT_SET )

I was wondering about the purpose of XENPF_UCODE_FLAG_FORCE_NOT_SET. I don't
think this constant is needed: Just like with any flags, simply using 0 when
none are set is fine both at the producing and consuming sides.

> @@ -120,12 +125,17 @@ int main(int argc, char *argv[])
> goto ext_err;
> show_curr_cpu(stdout);
> exit(EXIT_SUCCESS);
> + case 'f':
> + ucode_flag = XENPF_UCODE_FLAG_FORCE_SET;

Nit: Bad indentation.

Jan
Re: [PATCH 5/5] x86: Add --force option to xen-ucode to override microcode version check [ In reply to ]
On Fri, Apr 05, 2024 at 01:11:28PM +0100, Fouad Hilly wrote:
> diff --git a/tools/libs/ctrl/xc_misc.c b/tools/libs/ctrl/xc_misc.c
> index 5ecdfa2c7934..edce45bc2a17 100644
> --- a/tools/misc/xen-ucode.c
> +++ b/tools/misc/xen-ucode.c
> @@ -21,10 +23,11 @@ static const char amd_id[] = "AuthenticAMD";
> static void usage(const char *name)
> {
> printf("%s: Xen microcode updating tool\n"
> - "Usage: %s [<microcode file> | --show-cpu-info]\n"
> + "Usage: %s [[--force] <microcode file> | --show-cpu-info]\n"

How about "Usage: %s [OPTIONS..] [MICROCODE FILE]" ?

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

typo: microcode ;-)

> "\n"
> , name, name);
> }
> @@ -89,11 +92,13 @@ int main(int argc, char *argv[])
> char *filename = NULL, *buf;
> size_t len;
> struct stat st;
> + uint32_t ucode_flag = XENPF_UCODE_FLAG_FORCE_NOT_SET;
> int opt;
>
> const static struct option options[] = {
> {"help", no_argument, NULL, 'h'},
> {"show-cpu-info", no_argument, NULL, 's'},
> + {"force", required_argument, NULL, 'f'},

This is weird, could you do without the argument?

It is weird because sometime "microcode file" is an argument of
"--force", sometime it is part of the rests of the options.

> {NULL, no_argument, NULL, 0}
> };
>
> @@ -105,10 +110,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)
> {
> @@ -120,12 +125,17 @@ int main(int argc, char *argv[])
> goto ext_err;
> show_curr_cpu(stdout);
> exit(EXIT_SUCCESS);
> + case 'f':
> + ucode_flag = XENPF_UCODE_FLAG_FORCE_SET;
> + filename = optarg;
> + break;
> default:
> goto ext_err;
> }
> }
>
> - filename = argv[1];
> + if ( argc == 2 )
> + filename = argv[1];

Sometime we take filename from argv[1], sometime we don't? The logic is
going to be very confusing, takeout of context, only set `filename` from a
single place please.


Thanks,

--
Anthony PERARD