On 05/04/2023 10:11 am, Jan Beulich wrote:
> On 04.04.2023 18:06, Sergey Dyasli wrote:
>> Add an option to xen-ucode tool to print the currently loaded ucode
>> revision and also print it during usage info. Print CPU signature and
>> platform flags as well. The raw data comes from XENPF_get_cpu_version
>> and XENPF_get_ucode_revision platform ops.
>>
>> Example output:
>> Intel:
>> CPU signature 06-55-04 (raw 0x00050654) pf 0x1 revision 0x02006e05
>>
>> AMD:
>> CPU signature fam19h (raw 0x00a00f11) revision 0x0a0011ce
> Wouldn't it make sense to also report the model number here, even if
> ucode blob file names (currently) don't include it?
I have debated FF-MM-SS on AMD several times.
AMD do document all their CPUs consistently in hex, and I know the model
tables alarmingly well now. Furthermore, it's not a straight nibble
shuffle from the raw value.
So yeah, it probably is worth including. Given that we're not guessing
the Linux path of the microcode, it can replace the fam19h without any
loss of information.
> While for the revision always printing 8 digits is definitely helpful, I
> wonder whether the two top nibbles of the raw value wouldn't better be
> omitted. (I understand Andrew did ask for this, but it's unclear to me
> why he extended this request to the raw fields.)
For precisely the same reason. (With a question like this, you clearly
don't work frequently with microcode crossing a wide range of CPUs...)
>> --- a/tools/misc/xen-ucode.c
>> +++ b/tools/misc/xen-ucode.c
>> @@ -12,22 +12,89 @@
>> #include <fcntl.h>
>> #include <xenctrl.h>
>>
>> +static xc_interface *xch;
>> +
>> +static const char intel_id[] = "GenuineIntel";
>> +static const char amd_id[] = "AuthenticAMD";
>> +
>> +static void show_curr_cpu(FILE *f)
>> +{
>> + int ret;
>> + struct xenpf_pcpu_version cpu_ver = { .xen_cpuid = 0 };
>> + struct xenpf_ucode_revision ucode_rev = { .cpu = 0 };
> As mentioned before - the current state of the system may be
> inconsistent, so I question the entire lack of a way to know of
> this by using this tool (even if via a specific command line
> option, defaulting to CPU0-only).
It's a theoretical problem, not a practical one, and definitely not
something worthy to block this patch with.
Software always has more up-to-date microcode than firmware these days,
and it has probably been a decade since there was a system released
which could usefully function with asymmetric microcode.
I know we seen it in the past, but only on truly ancient systems.
>> + ret = xc_get_cpu_version(xch, &cpu_ver);
>> + if ( ret )
>> + {
>> + fprintf(f, "Failed to get CPU information. (err: %s)\n",
>> + strerror(errno));
>> + exit(1);
Use err(), which removes all the boilerplate here.
>> + }
>> + else if ( memcmp(cpu_ver.vendor_id, amd_id,
>> + sizeof(cpu_ver.vendor_id)) == 0 )
>> + {
>> + fprintf(f, "CPU signature fam%xh (raw 0x%08x) revision 0x%08x\n",
>> + cpu_ver.family, ucode_rev.signature, ucode_rev.revision);
>> + }
>> + else
>> + {
>> + fprintf(f, "Unsupported CPU vendor: %s\n", cpu_ver.vendor_id);
>> + exit(3);
>> + }
>> +}
>> +
>> int main(int argc, char *argv[])
>> {
>> int fd, ret;
>> char *filename, *buf;
>> size_t len;
>> struct stat st;
>> - xc_interface *xch;
>> +
>> + xch = xc_interface_open(NULL, NULL, 0);
>> + if ( xch == NULL )
>> + {
>> + fprintf(stderr, "Error opening xc interface. (err: %s)\n",
>> + strerror(errno));
>> + exit(1);
>> + }
>>
>> if ( argc < 2 )
>> {
>> - fprintf(stderr,
>> - "xen-ucode: Xen microcode updating tool\n"
>> - "Usage: %s <microcode blob>\n", argv[0]);
>> + fprintf(stderr, "xen-ucode: Xen microcode updating tool\n");
>> + show_curr_cpu(stderr);
> I recall you had it this way before, but I don't see why this information
> needs to be part of argument error handling.
Because it is specifically useful information to be given when running
xen-ucode with no arguments.
But also because this patch predates Spectre/Meltdown going public, and
is documented this way for XenServer.
That said, the information does need correcting because it's changed to be:
"Usage: %s [show-cpu-info | <microcode file>]"
in this patch. (and file is specifically more relevant than calling it
a blob).
~Andrew
