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

[Jan Beulich <jbeulich@xxxxxxxx>]

On 12.09.2019 09:22, Chao Gao wrote:
> @@ -249,49 +249,80 @@ 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_unlock(&microcode_mutex);
> +    if ( patch )
> +        err = microcode_ops->apply_microcode(patch);
> +    else if ( microcode_cache )
> +    {
> +        spin_lock(&microcode_mutex);
> +        err = microcode_ops->apply_microcode(microcode_cache);
> +        if ( err == -EIO )
> +        {
> +            microcode_free_patch(microcode_cache);
> +            microcode_cache = NULL;
> +        }
> +        spin_unlock(&microcode_mutex);
> +    }

I'm having trouble understanding the locking discipline here: Why
do you call ->apply_microcode() once with the lock held and once
without? If this is to guard against microcode_cache changing,
then (a) the check of it being non-NULL would need to be done with
the lock held as well and (b) you'd need to explain why the non-
locked call to ->apply_microcode() is okay.

It certainly wasn't this way in v8, yet the v9 revision log also
doesn't mention such a (not insignificant) change (which is part
of the reason why I didn't spot it in v9).

> +    else
> +        /* No patch to update */
> +        err = -ENOENT;
>  
>      return err;
>  }
>  
> -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 and only the first thread of a core
> +     * would succeed. Ignore error other than -EIO.
> +     */
> +    if ( ret != -EIO )
> +        ret = 0;

I don't think this is a good idea. Ignoring a _specific_ error
code (e.g. indicating "already loaded" or "newer patch already
loaded") is fine, but here you also ignore things like -ENOMEM
or -EINVAL.

> +    cpu = cpumask_next(smp_processor_id(), &cpu_online_map);
> +    if ( cpu < nr_cpu_ids )
> +        return continue_hypercall_on_cpu(cpu, do_microcode_update, patch) ?
> +                                                                          : 
> ret;

When there's no middle operand I don't think ? and : should be on
separate lines.

> @@ -299,32 +330,46 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) 
> buf, unsigned long len)
>      if ( microcode_ops == NULL )
>          return -EINVAL;
>  
> -    info = xmalloc_bytes(sizeof(*info) + len);
> -    if ( info == NULL )
> +    buffer = xmalloc_bytes(len);
> +    if ( !buffer )
>          return -ENOMEM;
>  
> -    ret = copy_from_guest(info->buffer, buf, len);
> -    if ( ret != 0 )
> +    if ( copy_from_guest(buffer, buf, len) )
> +    {
> +        ret = -EFAULT;
> +        goto free;
> +    }
> +
> +    patch = parse_blob(buffer, len);

You don't look to be using buffer anymore below this point. Why don't
you free it right here, avoiding the need for the "free" label below
and also further reducing the overall code churn as it seems.

Jan

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