Re: [PATCH v3 2/3] x86/platform: introduce XENPF_get_ucode_version

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 21/03/2023 11:47 am, 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.

It's not impossible (you can do `modprobe msr; rdmsr 0x8b`), but it is
inconvenient for library code.

>
> Add a new platform op in order to get the required data from Xen and
> provide a wrapper for libxenctrl.
>
> Signed-off-by: Sergey Dyasli <sergey.dyasli@xxxxxxxxxx>
> ---
>  tools/include/xenctrl.h                  |  2 ++
>  tools/libs/ctrl/xc_misc.c                | 21 +++++++++++++++++
>  xen/arch/x86/platform_hypercall.c        | 30 ++++++++++++++++++++++++
>  xen/arch/x86/x86_64/platform_hypercall.c |  4 ++++
>  xen/include/public/platform.h            | 12 ++++++++++
>  xen/include/xlat.lst                     |  1 +
>  6 files changed, 70 insertions(+)
>
> diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h
> index 8aa747dc2e..d3ef7a48a5 100644
> --- a/tools/include/xenctrl.h
> +++ b/tools/include/xenctrl.h
> @@ -1187,6 +1187,8 @@ int xc_cputopoinfo(xc_interface *xch, unsigned 
> *max_cpus,
>                     xc_cputopo_t *cputopo);
>  int xc_microcode_update(xc_interface *xch, const void *buf, size_t len);
>  int xc_get_cpu_version(xc_interface *xch, struct xenpf_pcpu_version 
> *cpu_ver);
> +int xc_get_ucode_version(xc_interface *xch,
> +                         struct xenpf_ucode_version *ucode_ver);

Throughout, we should use "revision" rather than "version" as that is
the terminology used by both Intel and AMD.

>  int xc_numainfo(xc_interface *xch, unsigned *max_nodes,
>                  xc_meminfo_t *meminfo, uint32_t *distance);
>  int xc_pcitopoinfo(xc_interface *xch, unsigned num_devs,
> diff --git a/tools/libs/ctrl/xc_misc.c b/tools/libs/ctrl/xc_misc.c
> index f2f6e4348e..b93477d189 100644
> --- a/tools/libs/ctrl/xc_misc.c
> +++ b/tools/libs/ctrl/xc_misc.c
> @@ -246,6 +246,27 @@ int xc_get_cpu_version(xc_interface *xch, struct 
> xenpf_pcpu_version *cpu_ver)
>      return 0;
>  }
>  
> +int xc_get_ucode_version(xc_interface *xch,
> +                         struct xenpf_ucode_version *ucode_ver)
> +{
> +    int ret;
> +    DECLARE_PLATFORM_OP;
> +
> +    if ( !xch || !ucode_ver )
> +        return -1;
> +
> +    platform_op.cmd = XENPF_get_ucode_version;
> +    platform_op.u.ucode_version.xen_cpuid = ucode_ver->xen_cpuid;

Same notes as per patch 1.

> diff --git a/xen/include/public/platform.h b/xen/include/public/platform.h
> index 60caa5ce7e..232df79d5f 100644
> --- a/xen/include/public/platform.h
> +++ b/xen/include/public/platform.h
> @@ -614,6 +614,17 @@ 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.  
> */

We commonly call this just "cpu".  As a platform op pertaining to
microcode, it cannot plausibly be confused with vcpus.

> +    uint32_t cpu_signature;   /* OUT: CPU signature (CPUID.1.EAX).          
> */

The cpu_ prefix can be dropped, as can ...

> +    uint32_t pf;              /* OUT: Processor Flags.                      
> */
> +                              /*      Only applicable to Intel.             
> */
> +    uint32_t ucode_revision;  /* OUT: Microcode Revision.                   
> */

the ucode_ prefix here.

I'm tempted to say that I'm happy to fix all of this on commit as it's
all mechanical changes.

~Andrew