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

Sergey Dyasli posted 1 patch 1 year, 2 months ago
Test gitlab-ci failed
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/20230113115630.22264-1-sergey.dyasli@citrix.com
tools/include/xenctrl.h           |  1 +
tools/libs/ctrl/xc_misc.c         |  5 +++
tools/misc/xen-ucode.c            | 68 +++++++++++++++++++++++++++++++
xen/arch/x86/platform_hypercall.c | 32 +++++++++++++++
xen/include/public/platform.h     | 14 +++++++
xen/include/xlat.lst              |  1 +
6 files changed, 121 insertions(+)
[PATCH] tools/xen-ucode: print information about currently loaded ucode
Posted by Sergey Dyasli 1 year, 2 months ago
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)
    Current CPU microcode revision is: 0xa0011a8

Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com>
---
CC: Wei Liu <wl@xen.org>
CC: Anthony PERARD <anthony.perard@citrix.com>
CC: Juergen Gross <jgross@suse.com>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: George Dunlap <george.dunlap@citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Julien Grall <julien@xen.org>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: "Roger Pau Monné" <roger.pau@citrix.com>
---
 tools/include/xenctrl.h           |  1 +
 tools/libs/ctrl/xc_misc.c         |  5 +++
 tools/misc/xen-ucode.c            | 68 +++++++++++++++++++++++++++++++
 xen/arch/x86/platform_hypercall.c | 32 +++++++++++++++
 xen/include/public/platform.h     | 14 +++++++
 xen/include/xlat.lst              |  1 +
 6 files changed, 121 insertions(+)

diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
index 23037874d3..e9911da5ea 100644
--- a/tools/include/xenctrl.h
+++ b/tools/include/xenctrl.h
@@ -1185,6 +1185,7 @@ 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_platform_op(xc_interface *xch, struct xen_platform_op *op);
 int xc_microcode_update(xc_interface *xch, const void *buf, size_t len);
 int xc_numainfo(xc_interface *xch, unsigned *max_nodes,
                 xc_meminfo_t *meminfo, uint32_t *distance);
diff --git a/tools/libs/ctrl/xc_misc.c b/tools/libs/ctrl/xc_misc.c
index 265f15ec2d..d03c240d14 100644
--- 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);
+}
+
 int xc_cputopoinfo(xc_interface *xch, unsigned *max_cpus,
                    xc_cputopo_t *cputopo)
 {
diff --git a/tools/misc/xen-ucode.c b/tools/misc/xen-ucode.c
index ad32face2b..c4cb4fbb50 100644
--- 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";
+
+void show_curr_cpu(FILE *f)
+{
+    int ret;
+    xc_interface *xch;
+    struct xen_platform_op op_cpu = {0}, op_ucode = {0};
+    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;
+
+    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;
+
+    ret = xc_platform_op(xch, &op_ucode);
+    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 ( intel )
+    {
+        fprintf(f, "Current CPU signature is: %02x-%02x-%02x (raw %#x)\n",
+                   cpu_ver->family, cpu_ver->model, cpu_ver->stepping,
+                   ucode_ver->cpu_signature);
+    }
+    else if ( amd )
+    {
+        fprintf(f, "Current CPU signature is: fam%xh (raw %#x)\n",
+                   cpu_ver->family, ucode_ver->cpu_signature);
+    }
+
+    if ( intel || amd )
+        fprintf(f, "Current CPU microcode revision is: %#x\n",
+                   ucode_ver->ucode_revision);
+
+    if ( intel )
+        fprintf(f, "Current CPU processor flags are: %#x\n", ucode_ver->pf);
+
+    xc_interface_close(xch);
+}
+
 int main(int argc, char *argv[])
 {
     int fd, ret;
@@ -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);
     }
 
diff --git a/xen/arch/x86/platform_hypercall.c b/xen/arch/x86/platform_hypercall.c
index 08ab2fea62..fcb7d81db1 100644
--- 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;
+        }
+        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;
+        }
+
+        put_cpu_maps();
+
+        if ( __copy_field_to_guest(u_xenpf_op, op, u.ucode_version) )
+            ret = -EFAULT;
+    }
+    break;
+
     case XENPF_cpu_online:
     {
         int cpu = op->u.cpu_ol.cpuid;
diff --git a/xen/include/public/platform.h b/xen/include/public/platform.h
index 14784dfa77..1a24dc24c0 100644
--- 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.                             */
+    uint32_t cpu_signature;   /* OUT: CPU signature (CPUID.1.EAX).          */
+    uint32_t pf;              /* OUT: Processor Flags.                      */
+                              /*      Only applicable to Intel.             */
+    uint32_t ucode_revision;  /* OUT: Microcode Revision.                   */
+};
+typedef struct xenpf_ucode_version xenpf_ucode_version_t;
+DEFINE_XEN_GUEST_HANDLE(xenpf_ucode_version_t);
+
 /*
  * ` enum neg_errnoval
  * ` HYPERVISOR_platform_op(const struct xen_platform_op*);
@@ -641,6 +654,7 @@ struct xen_platform_op {
         xenpf_resource_op_t           resource_op;
         xenpf_symdata_t               symdata;
         xenpf_dom0_console_t          dom0_console;
+        xenpf_ucode_version_t         ucode_version;
         uint8_t                       pad[128];
     } u;
 };
diff --git a/xen/include/xlat.lst b/xen/include/xlat.lst
index d601a8a984..164f700eb6 100644
--- 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
 ?	pmu_data			pmu.h
 ?	pmu_params			pmu.h
 !	sched_poll			sched.h
-- 
2.17.1


Re: [PATCH] tools/xen-ucode: print information about currently loaded ucode
Posted by Jan Beulich 1 year, 2 months ago
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