| Message ID | 20230404160655.2354-4-sergey.dyasli@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen-ucode: print information about currently loaded ucode | expand |
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? 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.) > --- 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). > + ret = xc_get_cpu_version(xch, &cpu_ver); > + if ( ret ) > + { > + fprintf(f, "Failed to get CPU information. (err: %s)\n", > + strerror(errno)); > + exit(1); Errors want to go to stderr, I would say (just like is already the case in main()). > + } > + > + ret = xc_get_ucode_revision(xch, &ucode_rev); > + if ( ret ) > + { > + fprintf(f, "Failed to get microcode information. (err: %s)\n", > + strerror(errno)); > + exit(1); > + } > + > + /* > + * Print signature in a form that allows to quickly identify which ucode > + * blob to load, e.g.: > + * > + * Intel: /lib/firmware/intel-ucode/06-55-04 > + * AMD: /lib/firmware/amd-ucode/microcode_amd_fam19h.bin > + */ > + if ( memcmp(cpu_ver.vendor_id, intel_id, > + sizeof(cpu_ver.vendor_id)) == 0 ) > + { > + fprintf(f, "CPU signature %02x-%02x-%02x (raw 0x%08x) pf %#x revision 0x%08x\n", > + cpu_ver.family, cpu_ver.model, cpu_ver.stepping, > + ucode_rev.signature, ucode_rev.pf, ucode_rev.revision); Nit: Indentation (also below). I think you mean fprintf(f, "CPU signature %02x-%02x-%02x (raw 0x%08x) pf %#x revision 0x%08x\n", cpu_ver.family, cpu_ver.model, cpu_ver.stepping, ucode_rev.signature, ucode_rev.pf, ucode_rev.revision); > + } > + 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. Furthermore, considering the possibility of hitting an error path in show_curr_cpu(), I think ... > + fprintf(stderr, "Usage: %s <microcode blob>\n", argv[0]); ... this would need printing first, and perhaps ... > exit(2); ... the exit code also shouldn't be anything other than 2 in that event. Jan
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
diff --git a/tools/misc/xen-ucode.c b/tools/misc/xen-ucode.c index ad32face2b..bd0bfaaa00 100644 --- 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 }; + + ret = xc_get_cpu_version(xch, &cpu_ver); + if ( ret ) + { + fprintf(f, "Failed to get CPU information. (err: %s)\n", + strerror(errno)); + exit(1); + } + + ret = xc_get_ucode_revision(xch, &ucode_rev); + if ( ret ) + { + fprintf(f, "Failed to get microcode information. (err: %s)\n", + strerror(errno)); + exit(1); + } + + /* + * Print signature in a form that allows to quickly identify which ucode + * blob to load, e.g.: + * + * Intel: /lib/firmware/intel-ucode/06-55-04 + * AMD: /lib/firmware/amd-ucode/microcode_amd_fam19h.bin + */ + if ( memcmp(cpu_ver.vendor_id, intel_id, + sizeof(cpu_ver.vendor_id)) == 0 ) + { + fprintf(f, "CPU signature %02x-%02x-%02x (raw 0x%08x) pf %#x revision 0x%08x\n", + cpu_ver.family, cpu_ver.model, cpu_ver.stepping, + ucode_rev.signature, ucode_rev.pf, ucode_rev.revision); + } + 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); + fprintf(stderr, "Usage: %s <microcode blob>\n", argv[0]); exit(2); } + if ( !strcmp(argv[1], "show-cpu-info") ) + { + show_curr_cpu(stdout); + return 0; + } + filename = argv[1]; fd = open(filename, O_RDONLY); if ( fd < 0 ) @@ -52,14 +119,6 @@ int main(int argc, char *argv[]) exit(1); } - xch = xc_interface_open(NULL, NULL, 0); - if ( xch == NULL ) - { - fprintf(stderr, "Error opening xc interface. (err: %s)\n", - strerror(errno)); - exit(1); - } - ret = xc_microcode_update(xch, buf, len); if ( ret ) {
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 Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com> --- v3 --> v4: - changed the output to be 1-line long - made xc_interface *xch global - added error checking to xc calls - added error for unsupported CPU vendor - changed printf format to 0x%08x for raw signature and revision values --- tools/misc/xen-ucode.c | 83 ++++++++++++++++++++++++++++++++++++------ 1 file changed, 71 insertions(+), 12 deletions(-)