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

[Chao Gao <chao.gao@intel.com>]

On Tue, Jan 29, 2019 at 12:27:30PM +0100, Roger Pau Monné wrote:
>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@intel.com>
>
>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)

Because find_patch() is expected to return a pointer to a ucode patch.
If a thread is to load this ucode patch, we should hold the lock to
avoid it being freed by another thread.

Thanks
Chao

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