Re: [PATCH v4 2/3] x86/platform: introduce XENPF_get_ucode_revision
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
Re: [PATCH v4 2/3] x86/platform: introduce XENPF_get_ucode_revision
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
Re: [PATCH v4 2/3] x86/platform: introduce XENPF_get_ucode_revision
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
Reviewed-by: Jan Beulich
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
[PATCH v4 2/3] x86/platform: introduce XENPF_get_ucode_revision
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
---
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(+)
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_
