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

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

On Wed, Jun 05, 2019 at 08:53:46AM -0600, Jan Beulich wrote:
>>>> On 27.05.19 at 10:31, <chao.gao@intel.com> 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@intel.com>
>
>Reviewed-by: Jan Beulich <jbeulich@suse.com>

Thanks.

>
>> ---
>> 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.

I think they should be suppressed. Ashok, could you confirm it?

I will figure out how to suppress them in Xen.

Thanks
Chao

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