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
