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