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

From: "Jan Beulich" <JBeulich@suse.com>
To: "Chao Gao" <chao.gao@intel.com>
Cc: Sergey Dyasli <sergey.dyasli@citrix.com>,
	Ashok Raj <ashok.raj@intel.com>, WeiLiu <wl@xen.org>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	xen-devel <xen-devel@lists.xenproject.org>,
	Roger Pau Monne <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH v7 09/10] microcode: remove microcode_update_lock
Date: Thu, 13 Jun 2019 08:08:46 -0600	[thread overview]
Message-ID: <5D0258EE0200007800237EEB@prv1-mh.provo.novell.com> (raw)
In-Reply-To: <20190613140524.GA2355@gao-cwp>

>>> 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, I'd then wonder
if it couldn't also be used for the #MC one.

Jan

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