Re: [Xen-devel] [PATCH v5 8/8] microcode: update microcode on cores in parallel

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

On Mon, Jan 28, 2019 at 03:06:50PM +0800, Chao Gao wrote:
> Currently, microcode_update_lock and microcode_mutex prevent cores
> from updating microcode in parallel. Below changes are made to support
> parallel microcode update on cores.

Oh, that's what I missed from the previous patch then, and what
serialises the applying of the microcode update.

> 
> microcode_update_lock is removed. The purpose of this 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 prevents parallel
> microcode updating on different cores. The original purpose of
> microcode_update_lock is already enforced at the level of
> apply_microcode()'s caller:
> 1. For late microcode update, only one sibiling thread of a core will
> call the apply_microcode().
> 2. For microcode update during system startup or CPU-hotplug, each
> logical thread is woken up one-by-one.
> 3. get/put_cpu_bitmaps() prevents the concurrency of CPU-hotplug and
> late microcode update.
> 
> microcode_mutex is replaced by a rwlock. microcode_mutex was used to
> prevent concurrent accesses to 'uci' and microcode_cache. Now the
> per-cpu variable, 'uci', won't be accessed by remote cpus after most
> fields in 'uci' have been removed; The only shared resource which
> needs to be protected is the microcode_cache. A rwlock allows multiple
> readers (one thread of each core) to access the global cache and
> update microcode simultaneously. Because the rwlock may be held in
> stop_machine context, where interrupt is disabled, irq{save, restore}
> variants are used to get/release the rwlock.
> 
> 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>

Thanks, this LGTM, just one question below.

> @@ -285,10 +307,11 @@ static int parse_microcode_blob(const void *buffer, 
> size_t len)
>  static int microcode_update_cpu(void)
>  {
>      int ret;
> +    unsigned long flag;
>  
> -    spin_lock(&microcode_mutex);
> +    read_lock_irqsave(&cache_rwlock, flag);
>      ret = microcode_ops->apply_microcode(smp_processor_id());
> -    spin_unlock(&microcode_mutex);
> +    read_unlock_irqrestore(&cache_rwlock, flag);

Why do you take the lock here, wouldn't it be better to just take it
for find_patch? (ie: like you do for save_patch)

Thanks, Roger.

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