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

[Sergey Dyasli <sergey.dyasli@xxxxxxxxx>]

On Wed, Mar 1, 2023 at 11:31 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> 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?

Ideal situation would be microcode information in Dom0's /proc/cpuinfo
updated after late load, since that file already has most of the
information about the cpu. And the closest thing to /proc is xenhypfs.
It allows the user to query information directly, e.g.

    # xenhypfs cat /cpuinfo/microcode-revision
    33554509

Which could be used manually or in scripts, instead of relying on
xen-ucode utility. Though printing the value in hex would be nicer.
That was my motivation to go hypfs route. In general it feels like cpu
information is a good fit for hypfs, but agreement on its format and
exposed values is needed.
I can always switch back to a platform op if that would be the preference.

> > --- 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.)

Yes, that comes from the BSP. Xen must make sure that all CPUs have
the same ucode revision for the system to work correctly.

Sergey