Re: [PATCH v4 3/3] tools/xen-ucode: print information about currently loaded ucode

[Jan Beulich <jbeulich@xxxxxxxx>]

On 04.04.2023 18:06, Sergey Dyasli wrote:
> Add an option to xen-ucode tool to print the currently loaded ucode
> revision and also print it during usage info.  Print CPU signature and
> platform flags as well.  The raw data comes from XENPF_get_cpu_version
> and XENPF_get_ucode_revision platform ops.
> 
> Example output:
>     Intel:
>     CPU signature 06-55-04 (raw 0x00050654) pf 0x1 revision 0x02006e05
> 
>     AMD:
>     CPU signature fam19h (raw 0x00a00f11) revision 0x0a0011ce

Wouldn't it make sense to also report the model number here, even if
ucode blob file names (currently) don't include it?

While for the revision always printing 8 digits is definitely helpful, I
wonder whether the two top nibbles of the raw value wouldn't better be
omitted. (I understand Andrew did ask for this, but it's unclear to me
why he extended this request to the raw fields.)

> --- a/tools/misc/xen-ucode.c
> +++ b/tools/misc/xen-ucode.c
> @@ -12,22 +12,89 @@
>  #include <fcntl.h>
>  #include <xenctrl.h>
>  
> +static xc_interface *xch;
> +
> +static const char intel_id[] = "GenuineIntel";
> +static const char   amd_id[] = "AuthenticAMD";
> +
> +static void show_curr_cpu(FILE *f)
> +{
> +    int ret;
> +    struct xenpf_pcpu_version cpu_ver = { .xen_cpuid = 0 };
> +    struct xenpf_ucode_revision ucode_rev = { .cpu = 0 };

As mentioned before - the current state of the system may be
inconsistent, so I question the entire lack of a way to know of
this by using this tool (even if via a specific command line
option, defaulting to CPU0-only).

> +    ret = xc_get_cpu_version(xch, &cpu_ver);
> +    if ( ret )
> +    {
> +        fprintf(f, "Failed to get CPU information. (err: %s)\n",
> +                strerror(errno));
> +        exit(1);

Errors want to go to stderr, I would say (just like is already the case
in main()).

> +    }
> +
> +    ret = xc_get_ucode_revision(xch, &ucode_rev);
> +    if ( ret )
> +    {
> +        fprintf(f, "Failed to get microcode information. (err: %s)\n",
> +                strerror(errno));
> +        exit(1);
> +    }
> +
> +    /*
> +     * Print signature in a form that allows to quickly identify which ucode
> +     * blob to load, e.g.:
> +     *
> +     *      Intel:   /lib/firmware/intel-ucode/06-55-04
> +     *      AMD:     /lib/firmware/amd-ucode/microcode_amd_fam19h.bin
> +     */
> +    if ( memcmp(cpu_ver.vendor_id, intel_id,
> +                sizeof(cpu_ver.vendor_id)) == 0 )
> +    {
> +        fprintf(f, "CPU signature %02x-%02x-%02x (raw 0x%08x) pf %#x 
> revision 0x%08x\n",
> +                   cpu_ver.family, cpu_ver.model, cpu_ver.stepping,
> +                   ucode_rev.signature, ucode_rev.pf, ucode_rev.revision);

Nit: Indentation (also below). I think you mean

        fprintf(f,
                "CPU signature %02x-%02x-%02x (raw 0x%08x) pf %#x revision 
0x%08x\n",
                cpu_ver.family, cpu_ver.model, cpu_ver.stepping,
                ucode_rev.signature, ucode_rev.pf, ucode_rev.revision);

> +    }
> +    else if ( memcmp(cpu_ver.vendor_id, amd_id,
> +                     sizeof(cpu_ver.vendor_id)) == 0 )
> +    {
> +        fprintf(f, "CPU signature fam%xh (raw 0x%08x) revision 0x%08x\n",
> +                   cpu_ver.family, ucode_rev.signature, ucode_rev.revision);
> +    }
> +    else
> +    {
> +        fprintf(f, "Unsupported CPU vendor: %s\n", cpu_ver.vendor_id);
> +        exit(3);
> +    }
> +}
> +
>  int main(int argc, char *argv[])
>  {
>      int fd, ret;
>      char *filename, *buf;
>      size_t len;
>      struct stat st;
> -    xc_interface *xch;
> +
> +    xch = xc_interface_open(NULL, NULL, 0);
> +    if ( xch == NULL )
> +    {
> +        fprintf(stderr, "Error opening xc interface. (err: %s)\n",
> +                strerror(errno));
> +        exit(1);
> +    }
>  
>      if ( argc < 2 )
>      {
> -        fprintf(stderr,
> -                "xen-ucode: Xen microcode updating tool\n"
> -                "Usage: %s <microcode blob>\n", argv[0]);
> +        fprintf(stderr, "xen-ucode: Xen microcode updating tool\n");
> +        show_curr_cpu(stderr);

I recall you had it this way before, but I don't see why this information
needs to be part of argument error handling. Furthermore, considering the
possibility of hitting an error path in show_curr_cpu(), I think ...

> +        fprintf(stderr, "Usage: %s <microcode blob>\n", argv[0]);

... this would need printing first, and perhaps ...

>          exit(2);

... the exit code also shouldn't be anything other than 2 in that event.

Jan