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

[Jan Beulich <jbeulich@xxxxxxxx>]

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@xxxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
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