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

[Anthony PERARD <anthony.perard@xxxxxxxxx>]

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