Re: [PATCH v4 2/3] x86/platform: introduce XENPF_get_ucode_revision

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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