| Message ID | 20230404160655.2354-3-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: > Currently it's impossible to get CPU's microcode revision from Xen after > late loading without looking into Xen logs which is not always convenient. > > Add a new platform op in order to get the required data from Xen and > provide a wrapper for libxenctrl. > > Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com> Reviewed-by: Jan Beulich <jbeulich@suse.com> with two remarks: > --- a/tools/libs/ctrl/xc_misc.c > +++ b/tools/libs/ctrl/xc_misc.c > @@ -243,6 +243,24 @@ int xc_get_cpu_version(xc_interface *xch, struct xenpf_pcpu_version *cpu_ver) > return 0; > } > > +int xc_get_ucode_revision(xc_interface *xch, > + struct xenpf_ucode_revision *ucode_rev) > +{ > + int ret; > + struct xen_platform_op op = { > + .cmd = XENPF_get_ucode_revision, > + .u.ucode_revision.cpu = ucode_rev->cpu, > + }; > + > + ret = do_platform_op(xch, &op); > + if ( ret != 0 ) > + return ret; Is there anything wrong with omitting this if() and ... > + *ucode_rev = op.u.ucode_revision; > + > + return 0; ... using "return ret" here? > --- a/xen/arch/x86/platform_hypercall.c > +++ b/xen/arch/x86/platform_hypercall.c > @@ -640,6 +640,35 @@ ret_t do_platform_op( > } > break; > > + case XENPF_get_ucode_revision: > + { > + struct xenpf_ucode_revision *rev = &op->u.ucode_revision; > + > + if ( !get_cpu_maps() ) > + { > + ret = -EBUSY; > + break; > + } > + > + /* TODO: make it possible to know ucode revisions for parked CPUs */ > + if ( (rev->cpu >= nr_cpu_ids) || !cpu_online(rev->cpu) ) > + ret = -ENOENT; While the cpu_online() check needs to be done under lock, it's kind of misleading for the caller to tell it to try again later when it has passed an out-of-range CPU number. Jan
On 05/04/2023 9:56 am, Jan Beulich wrote: > On 04.04.2023 18:06, Sergey Dyasli wrote: >> --- a/tools/libs/ctrl/xc_misc.c >> +++ b/tools/libs/ctrl/xc_misc.c >> @@ -243,6 +243,24 @@ int xc_get_cpu_version(xc_interface *xch, struct xenpf_pcpu_version *cpu_ver) >> return 0; >> } >> >> +int xc_get_ucode_revision(xc_interface *xch, >> + struct xenpf_ucode_revision *ucode_rev) >> +{ >> + int ret; >> + struct xen_platform_op op = { >> + .cmd = XENPF_get_ucode_revision, >> + .u.ucode_revision.cpu = ucode_rev->cpu, >> + }; >> + >> + ret = do_platform_op(xch, &op); >> + if ( ret != 0 ) >> + return ret; > Is there anything wrong with omitting this if() and ... > >> + *ucode_rev = op.u.ucode_revision; >> + >> + return 0; > ... using "return ret" here? Conceptually, yes. *ucode_rev oughtn't to be written to on failure. More importantly though, what Sergey wrote is consistent with the vast majority of libxc, and consistency is far more important than a marginal perf improvement which you won't be able to measure. >> --- a/xen/arch/x86/platform_hypercall.c >> +++ b/xen/arch/x86/platform_hypercall.c >> @@ -640,6 +640,35 @@ ret_t do_platform_op( >> } >> break; >> >> + case XENPF_get_ucode_revision: >> + { >> + struct xenpf_ucode_revision *rev = &op->u.ucode_revision; >> + >> + if ( !get_cpu_maps() ) >> + { >> + ret = -EBUSY; >> + break; >> + } >> + >> + /* TODO: make it possible to know ucode revisions for parked CPUs */ >> + if ( (rev->cpu >= nr_cpu_ids) || !cpu_online(rev->cpu) ) >> + ret = -ENOENT; > While the cpu_online() check needs to be done under lock, it's kind of > misleading for the caller to tell it to try again later when it has > passed an out-of-range CPU number. Honestly, I think you over-estimate the likelihood of the cpu map being contended, and over-estimate by 100% the chances that an out-of-range CPU is going to be passed. ~Andrew
On 06.04.2023 01:02, Andrew Cooper wrote: > On 05/04/2023 9:56 am, Jan Beulich wrote: >> On 04.04.2023 18:06, Sergey Dyasli wrote: >>> --- a/tools/libs/ctrl/xc_misc.c >>> +++ b/tools/libs/ctrl/xc_misc.c >>> @@ -243,6 +243,24 @@ int xc_get_cpu_version(xc_interface *xch, struct xenpf_pcpu_version *cpu_ver) >>> return 0; >>> } >>> >>> +int xc_get_ucode_revision(xc_interface *xch, >>> + struct xenpf_ucode_revision *ucode_rev) >>> +{ >>> + int ret; >>> + struct xen_platform_op op = { >>> + .cmd = XENPF_get_ucode_revision, >>> + .u.ucode_revision.cpu = ucode_rev->cpu, >>> + }; >>> + >>> + ret = do_platform_op(xch, &op); >>> + if ( ret != 0 ) >>> + return ret; >> Is there anything wrong with omitting this if() and ... >> >>> + *ucode_rev = op.u.ucode_revision; >>> + >>> + return 0; >> ... using "return ret" here? > > Conceptually, yes. *ucode_rev oughtn't to be written to on failure. > > More importantly though, what Sergey wrote is consistent with the vast > majority of libxc, and consistency is far more important than a marginal > perf improvement which you won't be able to measure. My remark was entirely unrelated to performance, and instead solely to (source) code size. Jan
diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h index 34b3b25289..1149f805ba 100644 --- a/tools/include/xenctrl.h +++ b/tools/include/xenctrl.h @@ -1187,6 +1187,8 @@ int xc_cputopoinfo(xc_interface *xch, unsigned *max_cpus, xc_cputopo_t *cputopo); int xc_microcode_update(xc_interface *xch, const void *buf, size_t len); int xc_get_cpu_version(xc_interface *xch, struct xenpf_pcpu_version *cpu_ver); +int xc_get_ucode_revision(xc_interface *xch, + struct xenpf_ucode_revision *ucode_rev); int xc_numainfo(xc_interface *xch, unsigned *max_nodes, xc_meminfo_t *meminfo, uint32_t *distance); int xc_pcitopoinfo(xc_interface *xch, unsigned num_devs, diff --git a/tools/libs/ctrl/xc_misc.c b/tools/libs/ctrl/xc_misc.c index 90d50faa4f..4159294b2e 100644 --- a/tools/libs/ctrl/xc_misc.c +++ b/tools/libs/ctrl/xc_misc.c @@ -243,6 +243,24 @@ int xc_get_cpu_version(xc_interface *xch, struct xenpf_pcpu_version *cpu_ver) return 0; } +int xc_get_ucode_revision(xc_interface *xch, + struct xenpf_ucode_revision *ucode_rev) +{ + int ret; + struct xen_platform_op op = { + .cmd = XENPF_get_ucode_revision, + .u.ucode_revision.cpu = ucode_rev->cpu, + }; + + ret = do_platform_op(xch, &op); + if ( ret != 0 ) + return ret; + + *ucode_rev = op.u.ucode_revision; + + return 0; +} + int xc_cputopoinfo(xc_interface *xch, unsigned *max_cpus, xc_cputopo_t *cputopo) { diff --git a/xen/arch/x86/platform_hypercall.c b/xen/arch/x86/platform_hypercall.c index a2d9526355..9ff2da8fc3 100644 --- a/xen/arch/x86/platform_hypercall.c +++ b/xen/arch/x86/platform_hypercall.c @@ -640,6 +640,35 @@ ret_t do_platform_op( } break; + case XENPF_get_ucode_revision: + { + struct xenpf_ucode_revision *rev = &op->u.ucode_revision; + + if ( !get_cpu_maps() ) + { + ret = -EBUSY; + break; + } + + /* TODO: make it possible to know ucode revisions for parked CPUs */ + if ( (rev->cpu >= nr_cpu_ids) || !cpu_online(rev->cpu) ) + ret = -ENOENT; + else + { + const struct cpu_signature *sig = &per_cpu(cpu_sig, rev->cpu); + + rev->signature = sig->sig; + rev->pf = sig->pf; + rev->revision = sig->rev; + } + + put_cpu_maps(); + + if ( __copy_field_to_guest(u_xenpf_op, op, u.ucode_revision) ) + ret = -EFAULT; + } + break; + case XENPF_cpu_online: { int cpu = op->u.cpu_ol.cpuid; diff --git a/xen/arch/x86/x86_64/platform_hypercall.c b/xen/arch/x86/x86_64/platform_hypercall.c index 5bf6b958d2..99440f4076 100644 --- a/xen/arch/x86/x86_64/platform_hypercall.c +++ b/xen/arch/x86/x86_64/platform_hypercall.c @@ -28,6 +28,10 @@ CHECK_pf_pcpuinfo; CHECK_pf_pcpu_version; #undef xen_pf_pcpu_version +#define xen_pf_ucode_revision xenpf_ucode_revision +CHECK_pf_ucode_revision; +#undef xen_pf_pucode_revision + #define xen_pf_enter_acpi_sleep xenpf_enter_acpi_sleep CHECK_pf_enter_acpi_sleep; #undef xen_pf_enter_acpi_sleep diff --git a/xen/include/public/platform.h b/xen/include/public/platform.h index 60caa5ce7e..15777b5416 100644 --- a/xen/include/public/platform.h +++ b/xen/include/public/platform.h @@ -614,6 +614,16 @@ DEFINE_XEN_GUEST_HANDLE(xenpf_symdata_t); typedef struct dom0_vga_console_info xenpf_dom0_console_t; DEFINE_XEN_GUEST_HANDLE(xenpf_dom0_console_t); +#define XENPF_get_ucode_revision 65 +struct xenpf_ucode_revision { + uint32_t cpu; /* IN: CPU number to get the revision from. */ + uint32_t signature; /* OUT: CPU signature (CPUID.1.EAX). */ + uint32_t pf; /* OUT: Platform Flags (Intel only) */ + uint32_t revision; /* OUT: Microcode Revision. */ +}; +typedef struct xenpf_ucode_revision xenpf_ucode_revision_t; +DEFINE_XEN_GUEST_HANDLE(xenpf_ucode_revision_t); + /* * ` enum neg_errnoval * ` HYPERVISOR_platform_op(const struct xen_platform_op*); @@ -645,6 +655,7 @@ struct xen_platform_op { xenpf_resource_op_t resource_op; xenpf_symdata_t symdata; xenpf_dom0_console_t dom0_console; + xenpf_ucode_revision_t ucode_revision; uint8_t pad[128]; } u; }; diff --git a/xen/include/xlat.lst b/xen/include/xlat.lst index d601a8a984..9c41948514 100644 --- a/xen/include/xlat.lst +++ b/xen/include/xlat.lst @@ -157,6 +157,7 @@ ? xenpf_pcpuinfo platform.h ? xenpf_pcpu_version platform.h ? xenpf_resource_entry platform.h +? xenpf_ucode_revision platform.h ? pmu_data pmu.h ? pmu_params pmu.h ! sched_poll sched.h
Currently it's impossible to get CPU's microcode revision from Xen after late loading without looking into Xen logs which is not always convenient. Add a new platform op in order to get the required data from Xen and provide a wrapper for libxenctrl. Signed-off-by: Sergey Dyasli <sergey.dyasli@citrix.com> --- v3 --> v4: - clarified the commit message - Renamed "ucode version" to "ucode revision" - Removed DECLARE_PLATFORM_OP and NULL checks - Added a TODO comment about parked CPUs - Renamed struct xenpf_ucode_revision fields --- tools/include/xenctrl.h | 2 ++ tools/libs/ctrl/xc_misc.c | 18 +++++++++++++++ xen/arch/x86/platform_hypercall.c | 29 ++++++++++++++++++++++++ xen/arch/x86/x86_64/platform_hypercall.c | 4 ++++ xen/include/public/platform.h | 11 +++++++++ xen/include/xlat.lst | 1 + 6 files changed, 65 insertions(+)