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

[Chao Gao <chao.gao@xxxxxxxxx>]

On Thu, Sep 12, 2019 at 04:07:16PM +0200, Jan Beulich wrote:
>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,

Yes. microcode_cache is protected by microcode_mutex;

>then (a) the check of it being non-NULL would need to be done with
>the lock held as well and

Will do.

>(b) you'd need to explain why the non-
>locked call to ->apply_microcode() is okay.

->apply_microcode() was always called with this lock held was because
it always read the old per-cpu cache which was protected by the lock.
It gave us an impression that ->apply_microcode() was protected by the
lock.

The patch before this one makes ->apply_microcode() accept a patch
pointer. With this change, if the patch being passed should be accessed
with some lock held (like the secondary call site above), we acquire
the lock. Otherwise, no lock is taken and the caller of
microcode_update_cpu() is supposed to guarantee the patch won't be
changed by others.

>
>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).

It is my bad.

>
>> +    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.

will do.

>
>> +    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.

Yes. Good idea.

Thanks
Chao

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