Re: [Xen-devel] [PATCH v11 1/7] microcode: split out apply_microcode() from cpu_request_microcode()

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.09.2019 15:53, Chao Gao wrote:
> @@ -249,49 +249,82 @@ 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));
>  
> -    spin_lock(&microcode_mutex);
> +    if ( unlikely(err) )
> +        return err;
>  
> -    err = microcode_ops->collect_cpu_info(sig);
> -    if ( likely(!err) )
> -        err = microcode_ops->cpu_request_microcode(buf, size);
> +    spin_lock(&microcode_mutex);
> +    if ( patch )
> +        err = microcode_ops->apply_microcode(patch);
> +    else if ( microcode_cache )
> +    {
> +        err = microcode_ops->apply_microcode(microcode_cache);
> +        if ( err == -EIO )
> +        {
> +            microcode_free_patch(microcode_cache);
> +            microcode_cache = NULL;
> +        }
> +    }
> +    else
> +        /* No patch to update */
> +        err = -ENOENT;
>      spin_unlock(&microcode_mutex);

This is still not optimal (because of the locked region being
larger than really needed), but close enough to the original
code, i.e. I guess fine for now.

> -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;
> +    int ret = microcode_update_cpu(patch);
>  
> -    error = microcode_update_cpu(info->buffer, info->buffer_size);
> -    if ( error )
> -        info->error = error;
> +    /* Store the patch after a successful loading */
> +    if ( !ret && patch )
> +    {
> +        spin_lock(&microcode_mutex);
> +        microcode_update_cache(patch);
> +        spin_unlock(&microcode_mutex);
> +        patch = NULL;
> +    }
>  
>      if ( microcode_ops->end_update_percpu )
>          microcode_ops->end_update_percpu();
>  
> -    info->cpu = cpumask_next(info->cpu, &cpu_online_map);
> -    if ( info->cpu < nr_cpu_ids )
> -        return continue_hypercall_on_cpu(info->cpu, do_microcode_update, 
> info);
> +    /*
> +     * Each thread tries to load ucode. Only the first thread of a core
> +     * would succeed while other threads would encounter -EINVAL which
> +     * indicates current ucode revision is equal to or newer than the
> +     * given patch. It is actually expected; so ignore this error.
> +     */
> +    if ( ret == -EINVAL )
> +        ret = 0;

-EINVAL is a pretty generic error, and hence I'm wondering: Is the
described situation really the only one where -EINVAL would come
back? I'm again willing to accept this for now, but I think this
wants further refinement (i.e. use of a truly dedicated error code
for just the one condition you want to filter here, maybe -EEXIST.)

> @@ -556,19 +558,22 @@ static int cpu_request_microcode(const void *buf, 
> size_t bufsize)
>              break;
>          }
>  
> -        /* Update cache if this patch covers current CPU */
> -        if ( microcode_fits(new_patch->mc_amd) != MIS_UCODE )
> -            microcode_update_cache(new_patch);
> -        else
> -            microcode_free_patch(new_patch);
> -
> -        if ( match_cpu(microcode_get_cache()) )
> +        /*
> +         * If the new patch covers current CPU, compare patches and store the
> +         * one with higher revision.
> +         */
> +        if ( (microcode_fits(new_patch->mc_amd) != MIS_UCODE) &&
> +             (!patch || (compare_patch(new_patch, patch) == NEW_UCODE)) )
>          {
> -            error = apply_microcode(microcode_get_cache());
> -            if ( error )
> -                break;
> +            struct microcode_patch *tmp = patch;
> +
> +            patch = new_patch;
> +            new_patch = tmp;

Looks like you're open-coding SWAP() here.

> @@ -398,26 +380,44 @@ static long get_next_ucode_from_buffer(void **mc, const 
> u8 *buf,
>      return offset + total_size;
>  }
>  
> -static int cpu_request_microcode(const void *buf, size_t size)
> +static struct microcode_patch *cpu_request_microcode(const void *buf,
> +                                                     size_t size)
>  {
>      long offset = 0;
>      int error = 0;
>      void *mc;
> +    struct microcode_patch *patch = NULL;
>  
>      while ( (offset = get_next_ucode_from_buffer(&mc, buf, size, offset)) > 
> 0 )
>      {
> +        struct microcode_patch *new_patch;
> +
>          error = microcode_sanity_check(mc);
>          if ( error )
>              break;
> -        error = get_matching_microcode(mc);
> -        if ( error < 0 )
> +
> +        new_patch = alloc_microcode_patch(mc);
> +        if ( IS_ERR(new_patch) )
> +        {
> +            error = PTR_ERR(new_patch);
>              break;
> +        }
> +
>          /*
> -         * It's possible the data file has multiple matching ucode,
> -         * lets keep searching till the latest version
> +         * If the new patch covers current CPU, compare patches and store the
> +         * one with higher revision.
>           */
> -        if ( error == 1 )
> -            error = 0;
> +        if ( (microcode_update_match(&new_patch->mc_intel->hdr) != 
> MIS_UCODE) &&
> +             (!patch || (compare_patch(new_patch, patch) == NEW_UCODE)) )
> +        {
> +            struct microcode_patch *tmp = patch;
> +
> +            patch = new_patch;
> +            new_patch = tmp;

And again here. Preferably with these last two taken care of
(which I'll take the liberty to do if I end up committing
this)
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan

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