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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 28.02.2023 18:39, Sergey Dyasli wrote:
> Add an option to xen-ucode tool to print the currently loaded ucode
> version and also print it during usage info.  Print CPU signature and
> processor flags as well.  The raw data comes from cpuinfo directory in
> xenhypfs and from XENPF_get_cpu_version platform op.

While I don't mind the use of the platform-op, I'm little puzzled by the
mix. If CPU information is to be exposed in hypfs, can't we expose there
everything that's needed here?

Then again, perhaps in a different context, Andrew pointed out that hypfs
is an optional component, so relying on its presence in the underlying
hypervisor will need weighing against the alternative of adding a new
platform-op for the ucode-related data (as you had it in v1). Since I'm
unaware of a request to switch, are there specific reasons you did?

> --- a/tools/misc/xen-ucode.c
> +++ b/tools/misc/xen-ucode.c
> @@ -11,6 +11,96 @@
>  #include <sys/stat.h>
>  #include <fcntl.h>
>  #include <xenctrl.h>
> +#include <xenhypfs.h>
> +
> +static const char intel_id[] = "GenuineIntel";
> +static const char   amd_id[] = "AuthenticAMD";
> +
> +static const char sig_path[] = "/cpuinfo/cpu-signature";
> +static const char rev_path[] = "/cpuinfo/microcode-revision";
> +static const char  pf_path[] = "/cpuinfo/processor-flags";

Together with the use below I conclude (without having looked at patch 1
yet) that you only expose perhaps the BSP's data, rather than such for
all CPUs. (And I was actually going to put up the question whether data
like the one presented here might not also be of interest for parked
CPUs.)

> +static int hypfs_read_uint(struct xenhypfs_handle *hdl, const char *path,
> +                           unsigned int *var)
> +{
> +    char *result;
> +    result = xenhypfs_read(hdl, path);
> +    if ( !result )
> +        return -1;
> +
> +    errno = 0;
> +    *var = strtol(result, NULL, 10);

Better use strtoul() (as you're after an unsigned quantity), and perhaps
also better to not assume 10 as the radix (neither signature nor ucode
revision are very useful to expose as decimal numbers in hypfs)? Plus
perhaps deal with the result not fitting in "unsigned int"?

> +    if ( errno )
> +        return -1;
> +
> +    return 0;
> +}
> +
> +static void show_curr_cpu(FILE *f)
> +{
> +    int ret;
> +    struct xenhypfs_handle *hdl;
> +    xc_interface *xch;
> +    struct xenpf_pcpu_version cpu_ver = {0};
> +    bool intel = false, amd = false;
> +    unsigned int cpu_signature, pf, ucode_revision;
> +
> +    hdl = xenhypfs_open(NULL, 0);
> +    if ( !hdl )
> +        return;
> +
> +    xch = xc_interface_open(0, 0, 0);
> +    if ( xch == NULL )
> +        return;
> +
> +    ret = xc_get_cpu_version(xch, &cpu_ver);
> +    if ( ret )
> +        return;
> +
> +    if ( memcmp(cpu_ver.vendor_id, intel_id,
> +                sizeof(cpu_ver.vendor_id)) == 0 )
> +        intel = true;
> +    else if ( memcmp(cpu_ver.vendor_id, amd_id,
> +                     sizeof(cpu_ver.vendor_id)) == 0 )
> +        amd = true;
> +
> +    if ( hypfs_read_uint(hdl, sig_path, &cpu_signature) != 0 )
> +        return;
> +
> +    if ( hypfs_read_uint(hdl, rev_path, &ucode_revision) != 0 )
> +        return;

You consume these two fields only when "intel || amd"; without having
looked at patch 1 yet I'd also assume the node might not be present for
other vendors, and hence no output would be produced at all then, due
to the failure here.

> +    if ( intel && hypfs_read_uint(hdl, pf_path,  &pf) != 0 )

Nit: Stray double blank before "&pf".

> +        return;
> +
> +    /*
> +     * 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 ( intel )
> +    {
> +        fprintf(f, "Current CPU signature is: %02x-%02x-%02x (raw %#x)\n",
> +                   cpu_ver.family, cpu_ver.model, cpu_ver.stepping,
> +                   cpu_signature);
> +    }
> +    else if ( amd )
> +    {
> +        fprintf(f, "Current CPU signature is: fam%xh (raw %#x)\n",
> +                   cpu_ver.family, cpu_signature);
> +    }

    else
        fprintf("...", cpu_ver.vendor_id,
                cpu_ver.family, cpu_ver.model, cpu_ver.stepping);

? Otherwise some kind of error message would imo be needed, such that the
tool won't exit successfully without providing any output at all. Recall
that we consider Hygon an alive target CPU, just with (at present) no
ucode support.

> +    if ( intel || amd )
> +        fprintf(f, "Current CPU microcode revision is: %#x\n", 
> ucode_revision);
> +
> +    if ( intel )
> +        fprintf(f, "Current CPU processor flags are: %#x\n", pf);
> +
> +    xc_interface_close(xch);
> +    xenhypfs_close(hdl);

Maybe do these earlier, as soon as you're done with each of the two items?

Jan