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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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? 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@xxxxxxxxx>
> ---
>  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?

Is there anyway to rollback to the previous state so all CPUs have the
same microcode? I assume nothing good will come out of running a
system with CPUs using different microcode versions, but maybe that's
not so bad?

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

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

>  }
>  
>  static int start_update(void)
> diff --git a/xen/arch/x86/microcode_intel.c b/xen/arch/x86/microcode_intel.c
> index 1857332..a529623 100644
> --- a/xen/arch/x86/microcode_intel.c
> +++ b/xen/arch/x86/microcode_intel.c
> @@ -466,10 +466,7 @@ static int cpu_request_microcode(unsigned int cpu, const 
> void *buf,
>      if ( offset < 0 )
>          error = offset;
>  
> -    if ( !error && matching_count )
> -        error = apply_microcode(cpu);
> -
> -    return error;
> +    return !error ? matching_count : error;

Same here.

Thanks, Roger.

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