Re: [Xen-devel] [PATCH v9 10/15] microcode: split out apply_microcode() from cpu_request_microcode()

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

On Mon, Aug 19, 2019 at 09:25:23AM +0800, Chao Gao wrote:
> During late microcode loading, 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(). In general, for both
> early loading on BSP and late loading, cpu_request_microcode() is
> called first to get the matching microcode update contained by
> the blob and then apply_microcode() is invoked explicitly on each
> cpu in common code.
> 
> Given that all CPUs are supposed to have the same signature, parsing
> microcode only needs to be done once. So cpu_request_microcode() is
> also moved out of microcode_update_cpu().
> 
> In some cases (e.g. a broken bios), the system may have multiple
> revisions of microcode update. So we would try to load a microcode
> update as long as it covers current cpu. And if a cpu loads this patch
> successfully, the patch would be stored into the patch cache.
> 
> Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

> ---
> Changes in v9:
>  - remove the calling of ->compare_patch in microcode_update_cpu().
>  - drop "microcode_" prefix for static function - microcode_parse_blob().
>  - rebase and fix conflict
> 
> Changes in v8:
>  - divide the original patch into three patches to improve readability
>  - load an update on each cpu as long as the update covers current cpu
>  - store an update after the first successful loading on a CPU
>  - Make sure the current CPU (especially pf value) is covered
>  by updates.
> 
> changes in v7:
>  - to handle load failure, unvalidated patches won't be cached. They
>  are passed as function arguments. So if update failed, we needn't
>  any cleanup to microcode cache.
> ---
>  xen/arch/x86/microcode.c        | 177 
> ++++++++++++++++++++++++++--------------
>  xen/arch/x86/microcode_amd.c    |  38 +++++----
>  xen/arch/x86/microcode_intel.c  |  66 +++++++--------
>  xen/include/asm-x86/microcode.h |   5 +-
>  4 files changed, 172 insertions(+), 114 deletions(-)
> 
> diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
> index 0e9322a..a2febc7 100644
> --- a/xen/arch/x86/microcode.c
> +++ b/xen/arch/x86/microcode.c
> @@ -189,12 +189,19 @@ static DEFINE_SPINLOCK(microcode_mutex);
>  
>  DEFINE_PER_CPU(struct cpu_signature, cpu_sig);
>  
> -struct microcode_info {
> -    unsigned int cpu;
> -    uint32_t buffer_size;
> -    int error;
> -    char buffer[1];
> -};
> +/*
> + * Return a patch that covers current CPU. If there are multiple patches,
> + * return the one with the highest revision number. Return error If no
> + * patch is found and an error occurs during the parsing process. Otherwise
> + * return NULL.
> + */
> +static struct microcode_patch *parse_blob(const char *buf, uint32_t len)

Nit: size_t would be more appropriate for len. AFAICT there's no need
for it to be 32bits anyway.

> +{
> +    if ( likely(!microcode_ops->collect_cpu_info(&this_cpu(cpu_sig))) )
> +        return microcode_ops->cpu_request_microcode(buf, len);
> +
> +    return NULL;
> +}
>  
>  int microcode_resume_cpu(void)
>  {
> @@ -220,13 +227,6 @@ void microcode_free_patch(struct microcode_patch 
> *microcode_patch)
>      xfree(microcode_patch);
>  }
>  
> -const struct microcode_patch *microcode_get_cache(void)
> -{
> -    ASSERT(spin_is_locked(&microcode_mutex));
> -
> -    return microcode_cache;
> -}
> -
>  /* Return true if cache gets updated. Otherwise, return false */
>  bool microcode_update_cache(struct microcode_patch *patch)
>  {
> @@ -250,49 +250,71 @@ 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);
> +    }
> +    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;
>  
> -    error = microcode_update_cpu(info->buffer, info->buffer_size);
> -    if ( error )
> -        info->error = error;
> +    /* Store the patch after a successful loading */
> +    if ( !microcode_update_cpu(patch) && patch )

Aren't you loosing the error code returned by microcode_update_cpu
here?

Seeing how this works I'm not sure what's the best option here. As
updating will be attempted on other CPUs, I'm not sure if it's OK to
return an error if the update succeed on some CPUs but failed on
others.

Thanks, Roger.

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