Re: [PATCH v4 4/6] microcode: don't call apply_microcode() in cpu_request_microcode()

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

On Wed, Nov 28, 2018 at 04:02:25PM +0100, Roger Pau Monné wrote:
>On Wed, Nov 28, 2018 at 01:34:14PM +0800, Chao Gao wrote:
>> cpu_request_microcode() will only parse microcode file and save
>> suitable microcodes to microcode_cache. To update microcode,
>> apply_microcode() should be invoked explicitly.
>> 
>> 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().
>
>I don't understand the previous usage of cpu_request_microcode, was it
>used to update the microcode?

Yes. I am moving microcode loading out of cpu_request_microcode hook.

>The name seems to suggest it's used to
>get a microcode version without applying anything to the CPU.
>
>> 
>> Signed-off-by: Chao Gao <chao.gao@intel.com>
>> ---
>>  xen/arch/x86/microcode.c       | 58 ++++++++++++++++++++++++++++++------------
>>  xen/arch/x86/microcode_amd.c   | 15 +++++------
>>  xen/arch/x86/microcode_intel.c |  5 +---
>>  3 files changed, 50 insertions(+), 28 deletions(-)
>> 
>> diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
>> index 8350d22..cca7b2c 100644
>> --- a/xen/arch/x86/microcode.c
>> +++ b/xen/arch/x86/microcode.c
>> @@ -233,20 +233,12 @@ int microcode_resume_cpu(unsigned int cpu)
>>      return err;
>>  }
>>  
>> -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);
>>  
>>      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);
>> -
>> +    err = microcode_ops->apply_microcode(smp_processor_id());
>>      spin_unlock(&microcode_mutex);
>>  
>>      return err;
>> @@ -259,7 +251,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();
>
>Why don't you just set info->error = microcode_update_cpu()?
>
>AFAICT this is done to attempt to update the remaining CPUs if one
>update failed?

Yes. But this patch doesn't change the logic here. Actually, if HT is
enabled and microcode is shared between the logical threads of the same
core, so if one thread updates microcode successfully, its sibling would
always fail in current logic. I am trying to explain why we cannot abort
the update even though an error is met in current logic. It definitely
can be solved by tweaking the logic slightly. 

>
>Is there anyway to rollback to the previous state so all CPUs have the
>same microcode?

Seems it is not allowed to load a microcode with numeratically smaller
revision according to 9.11.7.2.

With patch 6, a panic() would be triggered if some cpus failed to do the
update. I didn't try to change the logic here.

>I assume nothing good will come out of running a
>system with CPUs using different microcode versions, but maybe that's
>not so bad?

It is better that all CPUs have the same microcode revision. 

Linux kernel rejects late microcode update if finding some CPUs
offlined. I may port this patch to Xen too in a separate patch.

>
>>      if ( error )
>>          info->error = error;
>>  
>> @@ -276,6 +268,8 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
>>  {
>>      int ret;
>>      struct microcode_info *info;
>> +    unsigned int cpu = smp_processor_id();
>> +    struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
>>  
>>      if ( len != (uint32_t)len )
>>          return -E2BIG;
>> @@ -294,10 +288,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();
>> @@ -308,6 +298,26 @@ int microcode_update(XEN_GUEST_HANDLE_PARAM(const_void) buf, unsigned long len)
>>          }
>>      }
>>  
>> +    spin_lock(&microcode_mutex);
>> +
>> +    ret = microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);
>> +    if ( likely(!ret) )
>> +        ret = microcode_ops->cpu_request_microcode(cpu, info->buffer, len);
>> +    else
>> +        __microcode_fini_cpu(cpu);
>> +
>> +    spin_unlock(&microcode_mutex);
>
>Why do you need to hold the lock here?
>
>microcode_update is already serialized and should only be executed on
>a CPU at a time due to the usage of chained
>continue_hypercall_on_cpu.

microcode_resume_cpu() also uses the 'uci' and the global microcode cache.
This lock is to prevent them happening simultaneously (someone is
adding/replacing entries to a list and another is reading the list).
All existing call sites of collec_cpu_info() and cpu_request_microcode()
are protected with this lock.

>
>> +
>> +    if ( ret <= 0 )
>> +    {
>> +        printk("No valid or newer microcode found. Update abort!\n");
>> +        return -EINVAL;
>> +    }
>> +
>> +    info->buffer_size = len;
>> +    info->error = 0;
>> +    info->cpu = cpumask_first(&cpu_online_map);
>> +
>>      return continue_hypercall_on_cpu(info->cpu, do_microcode_update, info);
>>  }
>>  
>> @@ -370,13 +380,29 @@ int __init early_microcode_update_cpu(bool start_update)
>>      }
>>      if ( data )
>>      {
>> +        unsigned int cpu = smp_processor_id();
>> +        struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
>> +
>>          if ( start_update && microcode_ops->start_update )
>>              rc = microcode_ops->start_update();
>>  
>>          if ( rc )
>>              return rc;
>>  
>> -        return microcode_update_cpu(data, len);
>> +        spin_lock(&microcode_mutex);
>> +
>> +        rc = microcode_ops->collect_cpu_info(cpu, &uci->cpu_sig);
>> +        if ( likely(!rc) )
>> +            rc = microcode_ops->cpu_request_microcode(cpu, data, len);
>> +        else
>> +            __microcode_fini_cpu(cpu);
>> +
>> +        spin_unlock(&microcode_mutex);
>> +
>> +        if ( rc <= 0 )
>> +            return -EINVAL;
>> +
>> +        return microcode_update_cpu();
>>      }
>>      else
>>          return -ENOMEM;
>> diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
>> index 6e6598a..6d860f3 100644
>> --- a/xen/arch/x86/microcode_amd.c
>> +++ b/xen/arch/x86/microcode_amd.c
>> @@ -299,6 +299,10 @@ static int apply_microcode(unsigned int cpu)
>>  
>>      uci->cpu_sig.rev = rev;
>>  
>> +#if CONFIG_HVM
>> +    svm_host_osvw_init();
>> +#endif
>> +
>>      return 0;
>>  }
>>  
>> @@ -466,6 +470,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());
>> @@ -572,9 +577,7 @@ static int cpu_request_microcode(unsigned int cpu, const void *buf,
>>  
>>          if ( microcode_fits(mc_amd, cpu) )
>>          {
>> -            error = apply_microcode(cpu);
>> -            if ( error )
>> -                break;
>> +            matched_cnt++;
>>              applied_offset = last_offset;
>>          }
>>  
>> @@ -609,17 +612,13 @@ static int cpu_request_microcode(unsigned int cpu, const void *buf,
>>      }
>>  
>>    out:
>> -#if CONFIG_HVM
>> -    svm_host_osvw_init();
>> -#endif
>> -
>>      /*
>>       * In some cases we may return an error even if processor's microcode has
>>       * been updated. For example, the first patch in a container file is loaded
>>       * successfully but subsequent container file processing encounters a
>>       * failure.
>>       */
>> -    return error;
>> +    return !error ? matched_cnt : error;
>
>You can use error ?: matched_cnt; which is shorter.

I was told that It would be good to put the likely case in the first place.

Thanks
Chao

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