Re: [PATCH v5 5/8] microcode: split out apply_microcode() from cpu_request_microcode()

[Chao Gao <chao.gao@intel.com>]

On Tue, Jan 29, 2019 at 10:58:11AM +0100, Roger Pau Monné wrote:
>On Mon, Jan 28, 2019 at 03:06:47PM +0800, Chao Gao wrote:
>> During late microcode update, apply_microcode() is invoked in
>> cpu_request_microcode(). To make late microcode update more reliable,
>> we want to put the apply_microcode() into stop_machine context. So
>> we split out it from cpu_request_microcode(). As a consequence,
>> apply_microcode() should be invoked explicitly in the common code.
>> 
>> Also with the global ucode cache, microcode parsing only needs
>> to be done once; cpu_request_microcode() is also moved out of
>> microcode_update_cpu().
>> 
>> On AMD side, svm_host_osvw_init() is supposed to be called after
>> microcode update. As apply_micrcode() won't be called by
>> cpu_request_microcode() now, svm_host_osvw_init() is also moved to the
>> end of apply_microcode().
>> 
>> Signed-off-by: Chao Gao <chao.gao@intel.com>
>> ---
>>  xen/arch/x86/microcode.c       | 80 +++++++++++++++++++++++++-----------------
>>  xen/arch/x86/microcode_amd.c   | 23 +++++++-----
>>  xen/arch/x86/microcode_intel.c | 20 ++---------
>>  3 files changed, 64 insertions(+), 59 deletions(-)
>> 
>> diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
>> index 0c77e90..936f0b8 100644
>> --- a/xen/arch/x86/microcode.c
>> +++ b/xen/arch/x86/microcode.c
>> @@ -254,47 +254,42 @@ struct microcode_patch *find_patch(unsigned int cpu)
>>      return NULL;
>>  }
>>  
>> -int microcode_resume_cpu(unsigned int cpu)
>> +/*
>> + * Return the number of ucode patch inserted to the global cache.
>> + * Return negtive value on error.
>> + */
>> +static int parse_microcode_blob(const void *buffer, size_t len)
>>  {
>> -    int err;
>> +    unsigned int cpu = smp_processor_id();
>>      struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
>> -
>> -    if ( !microcode_ops )
>> -        return 0;
>> +    int ret;
>>  
>>      spin_lock(&microcode_mutex);
>> -
>> -    err = microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);
>> -    if ( err )
>> -    {
>> +    ret = microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);
>> +    if ( likely(!ret) )
>> +        ret = microcode_ops->cpu_request_microcode(cpu, buffer, len);
>> +    else
>>          microcode_fini_cpu(cpu);
>> -        spin_unlock(&microcode_mutex);
>> -        return err;
>> -    }
>> -
>> -    err = microcode_ops->apply_microcode(cpu);
>>      spin_unlock(&microcode_mutex);
>>  
>> -    return err;
>> +    return ret;
>>  }
>>  
>> -static int microcode_update_cpu(const void *buf, size_t size)
>> +static int microcode_update_cpu(void)
>>  {
>> -    int err;
>> -    unsigned int cpu = smp_processor_id();
>> -    struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
>> +    int ret;
>>  
>>      spin_lock(&microcode_mutex);
>> -
>> -    err = microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);
>> -    if ( likely(!err) )
>> -        err = microcode_ops->cpu_request_microcode(cpu, buf, size);
>> -    else
>> -        microcode_fini_cpu(cpu);
>> -
>> +    ret = microcode_ops->apply_microcode(smp_processor_id());
>
>I've realized that most of this helpers take a cpu id parameter
>(apply_microcode, collect_cpu_info and cpu_request_microcode), but
>that at least on Intel they are required to be called on the current
>CPU, at which point I wonder about their purpose. IMO they just make
>the interface more messy, without adding any functionality.

I agree with you and will remove the 'cpu' parameter from the interfaces
you mentioned above.

>
>>      spin_unlock(&microcode_mutex);
>>  
>> -    return err;
>> +    return ret;
>> +}
>> +
>> +int microcode_resume_cpu(unsigned int cpu)
>> +{
>> +    BUG_ON(cpu != smp_processor_id());
>
>Same here, what's the point of passing a cpu id as parameter when
>there's only one valid value?
>
>> +    return microcode_ops ? microcode_update_cpu() : 0;
>>  }
>>  
>>  static long do_microcode_update(void *_info)
>> @@ -304,7 +299,7 @@ static long do_microcode_update(void *_info)
>>  
>>      BUG_ON(info->cpu != smp_processor_id());
>>  
>> -    error = microcode_update_cpu(info->buffer, info->buffer_size);
>> +    error = microcode_update_cpu();
>>      if ( error )
>>          info->error = error;
>>  
>> @@ -339,10 +334,6 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
>>          return ret;
>>      }
>>  
>> -    info->buffer_size = len;
>> -    info->error = 0;
>> -    info->cpu = cpumask_first(&cpu_online_map);
>> -
>>      if ( microcode_ops->start_update )
>>      {
>>          ret = microcode_ops->start_update();
>> @@ -353,6 +344,18 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
>>          }
>>      }
>>  
>> +    ret = parse_microcode_blob(info->buffer, len);
>> +    if ( ret <= 0 )
>> +    {
>> +        printk(XENLOG_ERR "No valid or newer ucode found. Update abort!\n");
>> +        xfree(info);
>> +        return -EINVAL;
>> +    }
>> +
>> +    info->buffer_size = len;
>> +    info->error = 0;
>> +    info->cpu = cpumask_first(&cpu_online_map);
>
>It looks like you can also get rid of the cpu field in microcode_info,
>since it's always smp_processor_id?

Yes. Will remove the 'cpu' field.

>
>> +
>>      return continue_hypercall_on_cpu(info->cpu, do_microcode_update, info);
>>  }
>>  
>> @@ -396,13 +399,24 @@ int __init early_microcode_update_cpu(bool start_update)
>>      }
>>      if ( data )
>>      {
>> +        static bool parsed = false;
>> +
>>          if ( start_update && microcode_ops->start_update )
>>              rc = microcode_ops->start_update();
>>  
>>          if ( rc )
>>              return rc;
>>  
>> -        return microcode_update_cpu(data, len);
>> +        if ( !parsed )
>> +        {
>> +            rc = parse_microcode_blob(data, len);
>> +            parsed = true;
>> +
>> +            if ( rc <= 0 )
>> +                return -EINVAL;
>> +        }
>> +
>> +        return microcode_update_cpu();
>
>Hm, the usage of parsed here is kind of dangerous. I assume this works
>fine because early_microcode_update_cpu is called from the BSP in
>early_microcode_init, and then concurrent calls done by the APs always
>see parsed as true.

I think APs are woken up one-by-one. See the call site of cpu_up in
__start_xen.

>
>I would however recommend that you move the parsing to
>early_microcode_init, and that early_microcode_update_cpu always
>assume the blob has been parsed.
>
>If that doesn't work for some reason, I would then recommend that you
>gate the parsing based on cpu_id, so "smp_processor_id() == 0"

Do you think it is better:
remove the parsed flag and check whether current cpu is bsp in
early_microcode_init(); bsp calls the early_microcode_update_cpu(). APs
just call microcode_update_cpu().

>
>>      }
>>      else
>>          return -ENOMEM;
>> diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
>> index d86a596..80e274e 100644
>> --- a/xen/arch/x86/microcode_amd.c
>> +++ b/xen/arch/x86/microcode_amd.c
>> @@ -297,6 +297,10 @@ static int apply_microcode(unsigned int cpu)
>>  
>>      uci->cpu_sig.rev = rev;
>>  
>> +#if CONFIG_HVM
>> +    svm_host_osvw_init();
>> +#endif
>> +
>>      return 0;
>>  }
>>  
>> @@ -463,6 +467,7 @@ static int cpu_request_microcode(unsigned int cpu, const void *buf,
>>      struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
>>      unsigned int current_cpu_id;
>>      unsigned int equiv_cpu_id;
>> +    unsigned int matched_cnt = 0;
>>  
>>      /* We should bind the task to the CPU */
>>      BUG_ON(cpu != raw_smp_processor_id());
>> @@ -564,11 +569,15 @@ static int cpu_request_microcode(unsigned int cpu, const void *buf,
>>           * this ucode patch before checking whether it matches with
>>           * current CPU.
>>           */
>> -        if ( save_patch(microcode_patch) && microcode_fits(mc_amd, cpu) )
>> +        if ( save_patch(microcode_patch) )
>>          {
>> -            error = apply_microcode(cpu);
>> -            if ( error )
>> -                break;
>> +            matched_cnt++;
>> +            if ( microcode_fits(mc_amd, cpu) )
>> +            {
>> +                error = apply_microcode(cpu);
>> +                if ( error )
>> +                    break;
>> +            }
>
>In the commit message you mention that apply_microcode won't be called
>by cpu_request_microcode, yet it seems it's called?

lines below "matched_cnt++" should be removed. They slipped in when I
did a rebasing.

Thanks
Chao

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