Re: [Xen-devel] [PATCH v11 7/7] microcode: reject late ucode loading if any core is parked

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>, "Wei Liu" <wl@xen.org>,
	"Andrew Cooper" <andrew.cooper3@citrix.com>,
	xen-devel@lists.xenproject.org,
	"Roger Pau Monné" <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH v11 7/7] microcode: reject late ucode loading if any core is parked
Date: Mon, 30 Sep 2019 10:12:11 +0200	[thread overview]
Message-ID: <cdbfa079-e3df-b085-c75c-b916bb5add29@suse.com> (raw)
In-Reply-To: <20190930064323.GA10598@gao-cwp>

On 30.09.2019 08:43, Chao Gao wrote:
> On Fri, Sep 27, 2019 at 01:19:16PM +0200, Jan Beulich wrote:
>> What I continue to be unconvinced of is for the chosen approach
>> to be better than briefly unparking a thread on each core, as
>> previously suggested.
> 
> It isn't so easy to go the same way as set_cx_pminfo().
> 
> 1. NMI handler on parked threads is changed to a nop. To load ucode in
> NMI handler, we have to switch back to normal NMI handler in
> default_idle(). But it conflicts with what the comments in play_dead()
> implies: it is not safe to call normal NMI handler after
> cpu_exit_clear().

Right - pointing at the Cx handling for reference was perhaps not
the best choice. Here we'd need to truly online a core, remembering
to offline it again right after the ucode update.

> 2. A precondition of unparking a thread on each core, we need to find
> out exactly all parked cores and wake up one thread of each of them.
> Then in theory, what this patch does is only part of unparking a thread
> on each core.

Possibly, but you've suggested a possibly better alternative further
down.

>> may not be an actual problem. But it wants mentioning in a code
>> comment, I think. Plus at the very least you depend on the used
>> cpu_data[] fields to not contain unduly large values (and hence
>> you e.g. depend on cpu_data[] not gaining an initializer,
>> setting the three fields of interest to their INVALID_* values,
>> as currently done by identify_cpu()).
> 
> Can we skip those threads whose socket ID is invalid and initialize
> the three fields in cpu_add()?

What would you initialize them to in cpu_add()? You don't know
their values yet, do you?

> Or maintain a bitmap for parked threads to help distinguish them from
> real offlined threads, and go through parked threads here?

I think this is the better approach in the long run. I've been at
least considering addition of such a bitmap for other reasons as well.

Jan

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