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

["Roger Pau Monné" <roger.pau@citrix.com>]

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.

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

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

>      }
>      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?

Thanks, Roger.

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