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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.03.2023 19:01, Sergey Dyasli wrote:
> 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.

If that was to represent host-wide information, Dom0 would need to gain
a parallel mechanism (e.g. /proc/pcpuinfo) covering pCPU-s instead of
the vCPU-s it has got.

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

I agree exposing a certain amount of per-CPU information in hypfs is
desirable. But whether that is to be the source for a tool like
xen-ucode is a separate question. If Andrew doesn't respond to this
aspect here, you may want to talk to him directly.

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

Yet Xen may not be in the position to do so, and representing the "may
not work correctly" case may be helpful in diagnosing problem reports.

Jan