Re: [Xen-devel] [PATCH v7 09/10] microcode: remove microcode_update_lock

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 27.05.19 at 10:31, <chao.gao@xxxxxxxxx> wrote:
> microcode_update_lock is to prevent logic threads of a same core from
> updating microcode at the same time. But due to using a global lock, it
> also prevented parallel microcode updating on different cores.
> 
> Remove this lock in order to update microcode in parallel. It is safe
> because we have already ensured serialization of sibling threads at the
> caller side.
> 1.For late microcode update, do_microcode_update() ensures that only one
>   sibiling thread of a core can update microcode.
> 2.For microcode update during system startup or CPU-hotplug,
>   microcode_mutex() guarantees update serialization of logical threads.
> 3.get/put_cpu_bitmaps() prevents the concurrency of CPU-hotplug and
>   late microcode update.
> 
> Note that printk in apply_microcode() and svm_host_osvm_init() (for AMD
> only) are still processed sequentially.
> 
> Signed-off-by: Chao Gao <chao.gao@xxxxxxxxx>

Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

> ---
> Changes in v7:
>  - reworked. Remove complex lock logics introduced in v5 and v6. The microcode
>  patch to be applied is passed as an argument without any global variable. 
> Thus
>  no lock is added to serialize potential readers/writers. Callers of
>  apply_microcode() will guarantee the correctness: the patch poninted by the
>  arguments won't be changed by others.

Much better this way indeed.

> @@ -307,8 +303,7 @@ static int apply_microcode(const struct microcode_patch 
> *patch)
>  
>      mc_intel = patch->mc_intel;
>  
> -    /* serialize access to the physical write to MSR 0x79 */
> -    spin_lock_irqsave(&microcode_update_lock, flags);
> +    BUG_ON(local_irq_is_enabled());
>  
>      /*
>       * Writeback and invalidate caches before updating microcode to avoid

Thinking about it - what happens if we hit an NMI or #MC here?
watchdog_disable(), a call to which you add in an earlier patch,
doesn't really suppress the generation of NMIs, it only tells the
handler not to look at the accumulated statistics.

Jan



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