Re: [PATCH v6 3/4] x86/ucode: Introduce --force option to xen-ucode

[Fouad Hilly <fouad.hilly@xxxxxxxxx>]

On Thu, Jul 25, 2024 at 9:44 AM Jan Beulich <jbeulich@xxxxxxxx> 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@xxxxxxxxx>
> Reviewed-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> [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