| Message ID | 20240405121128.260493-6-fouad.hilly@cloud.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86/xen-ucode: Introduce --force option | expand |
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
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,
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",
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(-)