Re: [Xen-devel] [PATCH v8 13/16] microcode: unify loading update during CPU resuming and AP wakeup

[Jan Beulich <JBeulich@xxxxxxxx>]

On 01.08.2019 12:22, Chao Gao wrote:
> Both are loading the cached patch. Since APs call the unified function,
> microcode_update_one(), during wakeup, the 'start_update' parameter
> which originally used to distinguish BSP and APs is redundant. So remove
> this parameter.
> 
> Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>
> ---
> Changes in v8:
>   - split out from the previous patch
> ---
>   xen/arch/x86/acpi/power.c       |  2 +-
>   xen/arch/x86/microcode.c        | 36 +++++++++++-------------------------
>   xen/arch/x86/smpboot.c          |  5 +----
>   xen/include/asm-x86/processor.h |  4 ++--
>   4 files changed, 15 insertions(+), 32 deletions(-)
> 
> diff --git a/xen/arch/x86/acpi/power.c b/xen/arch/x86/acpi/power.c
> index 4f21903..24798d5 100644
> --- a/xen/arch/x86/acpi/power.c
> +++ b/xen/arch/x86/acpi/power.c
> @@ -253,7 +253,7 @@ static int enter_state(u32 state)
>   
>       console_end_sync();
>   
> -    microcode_resume_cpu();
> +    microcode_update_one();
>   
>       if ( !recheck_cpu_features(0) )
>           panic("Missing previously available feature(s)\n");
> diff --git a/xen/arch/x86/microcode.c b/xen/arch/x86/microcode.c
> index f0b1e39..cbaf13d 100644
> --- a/xen/arch/x86/microcode.c
> +++ b/xen/arch/x86/microcode.c
> @@ -204,24 +204,6 @@ static struct microcode_patch 
> *microcode_parse_blob(const char *buf,
>       return NULL;
>   }
>   
> -int microcode_resume_cpu(void)
> -{
> -    int err;
> -    struct cpu_signature *sig = &this_cpu(cpu_sig);
> -
> -    if ( !microcode_ops )
> -        return 0;
> -
> -    spin_lock(&microcode_mutex);
> -
> -    err = microcode_ops->collect_cpu_info(sig);
> -    if ( likely(!err) )
> -        err = microcode_ops->apply_microcode(microcode_cache);
> -    spin_unlock(&microcode_mutex);
> -
> -    return err;
> -}
> -
>   void microcode_free_patch(struct microcode_patch *microcode_patch)
>   {
>       microcode_ops->free_patch(microcode_patch->mc);
> @@ -402,7 +384,16 @@ static int __init microcode_init(void)
>   }
>   __initcall(microcode_init);
>   
> -int __init early_microcode_update_cpu(bool start_update)
> +/* Load a cached update to current cpu */
> +int microcode_update_one(void)
> +{
> +    return microcode_ops ? microcode_update_cpu(NULL) : 0;
> +}

With both callers ignoring the return value, I wonder if the
function should return void. Else it might be better (but I'm
not entirely certain) for it to return -EOPNOTSUPP rather
than 0.

> +/*
> + * BSP calls this function to parse ucode blob and then apply an update.
> + */

This is a single line comment, and hence wants its style
changed accordingly.

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