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
