Re: [Xen-devel] [PATCH v12] microcode: rendezvous CPUs in NMI handler and load ucode

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.09.2019 18:12, Chao Gao wrote:
> @@ -105,23 +110,40 @@ void __init microcode_set_module(unsigned int idx)
>  }
>  
>  /*
> - * The format is '[<integer>|scan]'. Both options are optional.
> - * If the EFI has forced which of the multiboot payloads is to be used,
> - * no parsing will be attempted.
> + * The format is '[<integer>|scan=<bool>, nmi=<bool>]'. Both options are
> + * optional. If the EFI has forced which of the multiboot payloads is to be
> + * used, only nmi=<bool> is parsed.
>   */
>  static int __init parse_ucode(const char *s)
>  {
> -    const char *q = NULL;
> +    const char *ss;
> +    int val, rc = 0;
>  
> -    if ( ucode_mod_forced ) /* Forced by EFI */
> -       return 0;
> +    do {
> +        ss = strchr(s, ',');
> +        if ( !ss )
> +            ss = strchr(s, '\0');
>  
> -    if ( !strncmp(s, "scan", 4) )
> -        ucode_scan = 1;
> -    else
> -        ucode_mod_idx = simple_strtol(s, &q, 0);
> +        if ( (val = parse_boolean("nmi", s, ss)) >= 0 )
> +            ucode_in_nmi = val;
> +        else if ( !ucode_mod_forced ) /* Not forced by EFI */
> +        {
> +            if ( (val = parse_boolean("scan", s, ss)) >= 0 )
> +                ucode_scan = val;
> +            else
> +            {
> +                const char *q = NULL;

I don't think the initializer is needed here.

>  static int primary_thread_fn(const struct microcode_patch *patch)
>  {
> -    int ret = 0;
> -
>      if ( !wait_for_state(LOADING_CALLIN) )
>          return -EBUSY;
>  
> -    cpumask_set_cpu(smp_processor_id(), &cpu_callin_map);
> +    if ( ucode_in_nmi )
> +    {
> +        self_nmi();
>  
> -    if ( !wait_for_state(LOADING_ENTER) )
> -        return -EBUSY;
> +        /*
> +         * Wait for ucode loading is done in case that the NMI does not 
> arrive
> +         * synchronously, which may lead to a not-yet-updated error is 
> returned
> +         * below.
> +         */
> +        if ( unlikely(!wait_for_state(LOADING_EXIT)) )
> +            ASSERT_UNREACHABLE();
>  
> -    ret = microcode_ops->apply_microcode(patch);
> -    if ( !ret )
> -        atomic_inc(&cpu_updated);
> -    atomic_inc(&cpu_out);
> +        return this_cpu(loading_err);
> +    }
>  
> -    return ret;
> +    return primary_thread_work(patch);
>  }

A remark on the code structure - the overall amount of indentation
would have been less if you negated the if() expression.

> @@ -405,6 +489,10 @@ static int control_thread_fn(const struct 
> microcode_patch *patch)
>       */
>      watchdog_disable();
>  
> +    nmi_patch = patch;
> +    smp_wmb();
> +    saved_nmi_callback = set_nmi_callback(microcode_nmi_callback);
> +
>      /* Allow threads to call in */
>      set_state(LOADING_CALLIN);

Seeing the blank line you keep here after watchdog_disable(), ...

> @@ -455,6 +552,9 @@ static int control_thread_fn(const struct microcode_patch 
> *patch)
>      /* Mark loading is done to unblock other threads */
>      set_state(LOADING_EXIT);
>  
> +    set_nmi_callback(saved_nmi_callback);
> +    smp_wmb();
> +    nmi_patch = ZERO_BLOCK_PTR;
>      watchdog_enable();

... for consistency there would better have been one left ahead of
watchdog_enable() here as well.

Preferably with at least the first and last items taken care of
(which ought to be easy enough to do while committing)
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

Jan

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