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(µcode_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
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
