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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.01.2023 12:56, Sergey Dyasli wrote:
> Currently it's impossible to get CPU's microcode revision after late
> loading without looking into Xen logs which is not always convenient.
> Add an option to xen-ucode tool to print the currently loaded ucode
> version and also print it during usage info.
> 
> Add a new platform op in order to get the required data from Xen.
> Print CPU signature and processor flags as well.
> 
> Example output:
>     Intel:
>     Current CPU signature is: 06-55-04 (raw 0x50654)
>     Current CPU microcode revision is: 0x2006e05
>     Current CPU processor flags are: 0x1
> 
>     AMD:
>     Current CPU signature is: fam19h (raw 0xa00f11)

So quite a bit less precise information than on Intel in the non-raw
part. Is there a reason for this?

> --- a/tools/libs/ctrl/xc_misc.c
> +++ b/tools/libs/ctrl/xc_misc.c
> @@ -226,6 +226,11 @@ int xc_microcode_update(xc_interface *xch, const void 
> *buf, size_t len)
>      return ret;
>  }
>  
> +int xc_platform_op(xc_interface *xch, struct xen_platform_op *op)
> +{
> +    return do_platform_op(xch, op);
> +}

Wouldn't it make sense to simply rename do_platform_op()?

> --- a/tools/misc/xen-ucode.c
> +++ b/tools/misc/xen-ucode.c
> @@ -12,6 +12,67 @@
>  #include <fcntl.h>
>  #include <xenctrl.h>
>  
> +static const char *intel_id = "GenuineIntel";
> +static const char *amd_id   = "AuthenticAMD";

Do these need to be (non-const) pointers, rather than const char[]?

> +void show_curr_cpu(FILE *f)
> +{
> +    int ret;
> +    xc_interface *xch;
> +    struct xen_platform_op op_cpu = {0}, op_ucode = {0};

Instead of the dummy initializers, can't you make ...

> +    struct xenpf_pcpu_version *cpu_ver = &op_cpu.u.pcpu_version;
> +    struct xenpf_ucode_version *ucode_ver = &op_ucode.u.ucode_version;
> +    bool intel = false, amd = false;
> +
> +    xch = xc_interface_open(0, 0, 0);
> +    if ( xch == NULL )
> +        return;
> +
> +    op_cpu.cmd = XENPF_get_cpu_version;
> +    op_cpu.interface_version = XENPF_INTERFACE_VERSION;
> +    op_cpu.u.pcpu_version.xen_cpuid = 0;

... this and ...

> +    ret = xc_platform_op(xch, &op_cpu);
> +    if ( ret )
> +        return;
> +
> +    op_ucode.cmd = XENPF_get_ucode_version;
> +    op_ucode.interface_version = XENPF_INTERFACE_VERSION;
> +    op_ucode.u.pcpu_version.xen_cpuid = 0;

... this the initializers?

> @@ -20,11 +81,18 @@ int main(int argc, char *argv[])
>      struct stat st;
>      xc_interface *xch;
>  
> +    if ( argc >= 2 && !strcmp(argv[1], "show-cpu-info") )
> +    {
> +        show_curr_cpu(stdout);
> +        return 0;
> +    }
> +
>      if ( argc < 2 )
>      {
>          fprintf(stderr,
>                  "xen-ucode: Xen microcode updating tool\n"
>                  "Usage: %s <microcode blob>\n", argv[0]);
> +        show_curr_cpu(stderr);
>          exit(2);
>      }

Personally I'd find it mode logical if this remained first and you
inserted your new fragment right afterwards. That way you also don't
need to check argc twice.

> --- a/xen/arch/x86/platform_hypercall.c
> +++ b/xen/arch/x86/platform_hypercall.c
> @@ -640,6 +640,38 @@ ret_t do_platform_op(
>      }
>      break;
>  
> +    case XENPF_get_ucode_version:
> +    {
> +        struct xenpf_ucode_version *ver = &op->u.ucode_version;
> +
> +        if ( !get_cpu_maps() )
> +        {
> +            ret = -EBUSY;
> +            break;
> +        }
> +
> +        if ( (ver->xen_cpuid >= nr_cpu_ids) || !cpu_online(ver->xen_cpuid) )
> +        {
> +            ver->cpu_signature = 0;
> +            ver->pf = 0;
> +            ver->ucode_revision = 0;

Better return -ENOENT in this case?

> +        }
> +        else
> +        {
> +            const struct cpu_signature *sig = &per_cpu(cpu_sig, 
> ver->xen_cpuid);
> +
> +            ver->cpu_signature = sig->sig;
> +            ver->pf = sig->pf;
> +            ver->ucode_revision = sig->rev;

Here you read what is actually present, which ...

> --- a/xen/include/public/platform.h
> +++ b/xen/include/public/platform.h
> @@ -610,6 +610,19 @@ DEFINE_XEN_GUEST_HANDLE(xenpf_symdata_t);
>  typedef struct dom0_vga_console_info xenpf_dom0_console_t;
>  DEFINE_XEN_GUEST_HANDLE(xenpf_dom0_console_t);
>  
> +#define XENPF_get_ucode_version 65
> +struct xenpf_ucode_version {
> +    uint32_t xen_cpuid;       /* IN:  CPU number to get the revision from.  
> */
> +                              /*      Return data should be equal among all 
> */
> +                              /*      the CPUs.                             
> */

... doesn't necessarily match the promise here. Perhaps weaken the
"should", or clarify what the conditionsare for this to be the case?
Also your addition to xen-ucode builds on this, which can easily
end up misleading when it's not really the case.

> --- a/xen/include/xlat.lst
> +++ b/xen/include/xlat.lst
> @@ -157,6 +157,7 @@
>  ?    xenpf_pcpuinfo                  platform.h
>  ?    xenpf_pcpu_version              platform.h
>  ?    xenpf_resource_entry            platform.h
> +?    xenpf_ucode_version             platform.h

You also want to invoke the resulting macro, so that the intended checking
actually occurs.

Jan