| Message ID | 20230228173932.28510-4-sergey.dyasli@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen-ucode: print information about currently loaded ucode | expand |
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? > --- 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.) > +static int hypfs_read_uint(struct xenhypfs_handle *hdl, const char *path, > + unsigned int *var) > +{ > + char *result; > + result = xenhypfs_read(hdl, path); > + if ( !result ) > + return -1; > + > + errno = 0; > + *var = strtol(result, NULL, 10); Better use strtoul() (as you're after an unsigned quantity), and perhaps also better to not assume 10 as the radix (neither signature nor ucode revision are very useful to expose as decimal numbers in hypfs)? Plus perhaps deal with the result not fitting in "unsigned int"? > + if ( errno ) > + return -1; > + > + return 0; > +} > + > +static void show_curr_cpu(FILE *f) > +{ > + int ret; > + struct xenhypfs_handle *hdl; > + xc_interface *xch; > + struct xenpf_pcpu_version cpu_ver = {0}; > + bool intel = false, amd = false; > + unsigned int cpu_signature, pf, ucode_revision; > + > + hdl = xenhypfs_open(NULL, 0); > + if ( !hdl ) > + return; > + > + xch = xc_interface_open(0, 0, 0); > + if ( xch == NULL ) > + return; > + > + ret = xc_get_cpu_version(xch, &cpu_ver); > + if ( ret ) > + return; > + > + if ( memcmp(cpu_ver.vendor_id, intel_id, > + sizeof(cpu_ver.vendor_id)) == 0 ) > + intel = true; > + else if ( memcmp(cpu_ver.vendor_id, amd_id, > + sizeof(cpu_ver.vendor_id)) == 0 ) > + amd = true; > + > + if ( hypfs_read_uint(hdl, sig_path, &cpu_signature) != 0 ) > + return; > + > + if ( hypfs_read_uint(hdl, rev_path, &ucode_revision) != 0 ) > + return; You consume these two fields only when "intel || amd"; without having looked at patch 1 yet I'd also assume the node might not be present for other vendors, and hence no output would be produced at all then, due to the failure here. > + if ( intel && hypfs_read_uint(hdl, pf_path, &pf) != 0 ) Nit: Stray double blank before "&pf". > + return; > + > + /* > + * 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 ( intel ) > + { > + fprintf(f, "Current CPU signature is: %02x-%02x-%02x (raw %#x)\n", > + cpu_ver.family, cpu_ver.model, cpu_ver.stepping, > + cpu_signature); > + } > + else if ( amd ) > + { > + fprintf(f, "Current CPU signature is: fam%xh (raw %#x)\n", > + cpu_ver.family, cpu_signature); > + } else fprintf("...", cpu_ver.vendor_id, cpu_ver.family, cpu_ver.model, cpu_ver.stepping); ? Otherwise some kind of error message would imo be needed, such that the tool won't exit successfully without providing any output at all. Recall that we consider Hygon an alive target CPU, just with (at present) no ucode support. > + if ( intel || amd ) > + fprintf(f, "Current CPU microcode revision is: %#x\n", ucode_revision); > + > + if ( intel ) > + fprintf(f, "Current CPU processor flags are: %#x\n", pf); > + > + xc_interface_close(xch); > + xenhypfs_close(hdl); Maybe do these earlier, as soon as you're done with each of the two items? Jan
On Wed, Mar 1, 2023 at 11:31 AM Jan Beulich <jbeulich@suse.com> 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
On 01.03.2023 19:01, Sergey Dyasli wrote: > On Wed, Mar 1, 2023 at 11:31 AM Jan Beulich <jbeulich@suse.com> 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
On 01.03.2023 19:01, Sergey Dyasli wrote: > On Wed, Mar 1, 2023 at 11:31 AM Jan Beulich <jbeulich@suse.com> 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. I've confirmed with Andrew that I was remembering correctly and he would not want to see a dependency on hypfs in such a tool. Since it's optional in the hypervisor, you'd need an alternative source of the same info anyway, and hence you can as well go just that non-hypfs route. FTAOD: This isn't to say that some CPU info wouldn't be useful to expose in hypfs. But that's then an independent task. Jan
diff --git a/tools/misc/Makefile b/tools/misc/Makefile index 1c6e1d6a04..e345ac76db 100644 --- a/tools/misc/Makefile +++ b/tools/misc/Makefile @@ -136,6 +136,6 @@ xencov: xencov.o $(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenctrl) $(APPEND_LDFLAGS) xen-ucode: xen-ucode.o - $(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenctrl) $(APPEND_LDFLAGS) + $(CC) $(LDFLAGS) -o $@ $< $(LDLIBS_libxenctrl) $(LDLIBS_libxenhypfs) $(APPEND_LDFLAGS) -include $(DEPS_INCLUDE) diff --git a/tools/misc/xen-ucode.c b/tools/misc/xen-ucode.c index ad32face2b..7e657689f4 100644 --- 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"; + +static int hypfs_read_uint(struct xenhypfs_handle *hdl, const char *path, + unsigned int *var) +{ + char *result; + result = xenhypfs_read(hdl, path); + if ( !result ) + return -1; + + errno = 0; + *var = strtol(result, NULL, 10); + if ( errno ) + return -1; + + return 0; +} + +static void show_curr_cpu(FILE *f) +{ + int ret; + struct xenhypfs_handle *hdl; + xc_interface *xch; + struct xenpf_pcpu_version cpu_ver = {0}; + bool intel = false, amd = false; + unsigned int cpu_signature, pf, ucode_revision; + + hdl = xenhypfs_open(NULL, 0); + if ( !hdl ) + return; + + xch = xc_interface_open(0, 0, 0); + if ( xch == NULL ) + return; + + ret = xc_get_cpu_version(xch, &cpu_ver); + if ( ret ) + return; + + if ( memcmp(cpu_ver.vendor_id, intel_id, + sizeof(cpu_ver.vendor_id)) == 0 ) + intel = true; + else if ( memcmp(cpu_ver.vendor_id, amd_id, + sizeof(cpu_ver.vendor_id)) == 0 ) + amd = true; + + if ( hypfs_read_uint(hdl, sig_path, &cpu_signature) != 0 ) + return; + + if ( hypfs_read_uint(hdl, rev_path, &ucode_revision) != 0 ) + return; + + if ( intel && hypfs_read_uint(hdl, pf_path, &pf) != 0 ) + return; + + /* + * 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 ( intel ) + { + fprintf(f, "Current CPU signature is: %02x-%02x-%02x (raw %#x)\n", + cpu_ver.family, cpu_ver.model, cpu_ver.stepping, + cpu_signature); + } + else if ( amd ) + { + fprintf(f, "Current CPU signature is: fam%xh (raw %#x)\n", + cpu_ver.family, cpu_signature); + } + + if ( intel || amd ) + fprintf(f, "Current CPU microcode revision is: %#x\n", ucode_revision); + + if ( intel ) + fprintf(f, "Current CPU processor flags are: %#x\n", pf); + + xc_interface_close(xch); + xenhypfs_close(hdl); +} int main(int argc, char *argv[]) { @@ -25,9 +115,16 @@ int main(int argc, char *argv[]) fprintf(stderr, "xen-ucode: Xen microcode updating tool\n" "Usage: %s <microcode blob>\n", argv[0]); + show_curr_cpu(stderr); 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 )
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. Example output: Intel: Current CPU signature is: 06-55-04 (raw 0x50654) Current CPU microcode revision is: 0x2006e05 Current CPU processor flags are: 0x1 AMD: Current CPU signature is: fam19h (raw 0xa00f11) Current CPU microcode revision is: 0xa0011a8 Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com> --- tools/misc/Makefile | 2 +- tools/misc/xen-ucode.c | 97 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 98 insertions(+), 1 deletion(-)