Re: [Xen-devel] [PATCH v8 12/16] microcode: split out apply_microcode() from cpu_request_microcode()

[Jan Beulich <JBeulich@xxxxxxxx>]

On 01.08.2019 12:22, Chao Gao wrote:
> --- a/xen/arch/x86/microcode.c
> +++ b/xen/arch/x86/microcode.c
> @@ -189,12 +189,20 @@ static DEFINE_SPINLOCK(microcode_mutex);
>   
>   DEFINE_PER_CPU(struct cpu_signature, cpu_sig);
>   
> -struct microcode_info {
> -    unsigned int cpu;
> -    uint32_t buffer_size;
> -    int error;
> -    char buffer[1];
> -};
> +/*
> + * Return a patch that covers current CPU. If there are multiple patches,
> + * return the one with the highest revision number. Return error If no
> + * patch is found and an error occurs during the parsing process. Otherwise
> + * return NULL.
> + */
> +static struct microcode_patch *microcode_parse_blob(const char *buf,
> +                                                    uint32_t len)

Btw - you'd have less issues with line length if you omitted the
"microcode_" prefix from static functions.

> @@ -250,49 +251,88 @@ bool microcode_update_cache(struct microcode_patch 
> *patch)
>       return true;
>   }
>   
> -static int microcode_update_cpu(const void *buf, size_t size)
> +/*
> + * Load a microcode update to current CPU.
> + *
> + * If no patch is provided, the cached patch will be loaded. Microcode update
> + * during APs bringup and CPU resuming falls into this case.
> + */
> +static int microcode_update_cpu(const struct microcode_patch *patch)
>   {
> -    int err;
> -    unsigned int cpu = smp_processor_id();
> -    struct cpu_signature *sig = &per_cpu(cpu_sig, cpu);
> +    int err = microcode_ops->collect_cpu_info(&this_cpu(cpu_sig));
> +
> +    if ( unlikely(err) )
> +        return err;
>   
>       spin_lock(&microcode_mutex);
>   
> -    err = microcode_ops->collect_cpu_info(sig);
> -    if ( likely(!err) )
> -        err = microcode_ops->cpu_request_microcode(buf, size);
> +    if ( patch )
> +    {
> +        /*
> +         * If a patch is specified, it should has newer revision than
> +         * that of the patch cached.
> +         */
> +        if ( microcode_cache &&
> +             microcode_ops->compare_patch(patch, microcode_cache) != 
> NEW_UCODE )

While I see that you've taken care of the one case in Intel specific
code, this is again a case where I don't think you can validly call
this hook in the Intel case. Albeit maybe it is okay, provided that
the caller has already verified it against the CPU signature. Then
again I wonder why this check gets done here rather than in the
caller (next to that other check) anyway. Afaics this way you'd
also avoid re-checking on every CPU a CPU-independent property.

> -static long do_microcode_update(void *_info)
> +static long do_microcode_update(void *patch)
>   {
> -    struct microcode_info *info = _info;
> -    int error;
> -
> -    BUG_ON(info->cpu != smp_processor_id());
> +    unsigned int cpu;
>   
> -    error = microcode_update_cpu(info->buffer, info->buffer_size);
> -    if ( error )
> -        info->error = error;
> +    /* store the patch after a successful loading */

Nit: Comments should start with an uppercase letter.

Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel