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

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

On Thu, Jun 13, 2019 at 08:08:46AM -0600, Jan Beulich wrote:
>>>> On 13.06.19 at 16:05, <chao.gao@intel.com> wrote:
>> On Wed, Jun 12, 2019 at 01:38:31AM -0600, Jan Beulich wrote:
>>>>>> On 11.06.19 at 18:04, <ashok.raj@intel.com> wrote:
>>>> On Tue, Jun 11, 2019 at 08:46:04PM +0800, Chao Gao wrote:
>>>>> On Wed, Jun 05, 2019 at 08:53:46AM -0600, Jan Beulich wrote:
>>>>> >
>>>>> >> @@ -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 think the only sources would be the watchdog as you pointed out
>>>> which we already touch to keep it from expiring. The perf counters
>>>> i'm not an expert in, but i'll check. When we are in stop_machine() type
>>>> flow, its not clear if any of those would fire. (I might be wrong, but let
>>>> me check).
>>>
>>>Well, without disarming the watchdog NMI at the LAPIC / IO-APIC,
>>>how would it _not_ potentially fire?
>> 
>> We plan not to prevent NMI being fired. Instead, if one thread of a core
>> is updating microcode, other threads of this core would stop in the
>> handler of NMI until the update completion. Is this approach acceptable?
>
>Well, I have to return the question: It is you who knows what is or
>is not acceptable while an ucode update is in progress. In particular
>it obviously matters how much ucode is involved in the delivery of
>an NMI (and in allowing the handler to get to the point where you'd
>"stop" it).
>
>If the approach you suggest is fine for the NMI case,

Yes. It is fine. It is a suggestion from Ashok and what he is working
on in linux kernel. I just wanted to make sure you didn't oppose this
approach in Xen (considering disarming watchdog NMI might be an
alternative).

>I'd then wonder if it couldn't also be used for the #MC one.

I think no much pratical value for #MC because we still need to wait for
the callin of all threads. But as you and Ashok said, #MC is usually
fatal and machine goes down anyway.

Thanks
Chao

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