Re: [PATCH v6 11/12] x86/microcode: Synchronize late microcode loading

From: Chao Gao <chao.gao@intel.com>
To: Jan Beulich <JBeulich@suse.com>
Cc: Sergey Dyasli <sergey.dyasli@citrix.com>,
	Kevin Tian <kevin.tian@intel.com>, Wei Liu <wei.liu2@citrix.com>,
	Ashok Raj <ashok.raj@intel.com>,
	Andrew Cooper <andrew.cooper3@citrix.com>,
	Jun Nakajima <jun.nakajima@intel.com>,
	xen-devel <xen-devel@lists.xenproject.org>,
	tglx@linutronix.de, Borislav Petkov <bp@suse.de>,
	Roger Pau Monne <roger.pau@citrix.com>
Subject: Re: [PATCH v6 11/12] x86/microcode: Synchronize late microcode loading
Date: Thu, 14 Mar 2019 21:01:01 +0800	[thread overview]
Message-ID: <20190314130100.GA5054@gao-cwp> (raw)
In-Reply-To: <5C88B92F020000780021DF64@prv1-mh.provo.novell.com>

On Wed, Mar 13, 2019 at 02:02:55AM -0600, Jan Beulich wrote:
>>>> On 13.03.19 at 08:54, <JBeulich@suse.com> wrote:
>>>>> On 13.03.19 at 06:02, <chao.gao@intel.com> wrote:
>>> On Tue, Mar 12, 2019 at 05:07:51PM -0700, Raj, Ashok wrote:
>>>>On Mon, Mar 11, 2019 at 03:57:35PM +0800, Chao Gao wrote:
>>>>> +    if ( cpu == cpumask_first(per_cpu(cpu_sibling_mask, cpu)) )
>>>>> +        ret = microcode_update_cpu();
>>>>
>>>>Does ret have any useful things on if the update failed? Doesn't seem 
>>>>to be used before you overwrite later in collect_cpu_info()?
>>> 
>>> It has the reason of failure on error. Actally, there are two reasons:
>>> one is no patch of newer revision, the other is we tried to update but
>>> the microcode revision didn't change. I can check this return value and
>>> print more informative message to admin. And furthermore, for the
>>> latter, we can remove the ucode patch from caches to address Roger's
>>> concern expressed in comments to patch 4 & 5.
>> 
>> Btw, I'm not sure removing such ucode from the cache is appropriate:
>> It may well apply elsewhere, unless there's a clear indication that the
>> blob is broken.

Yes. Got it. Can we just assume we won't encounter that ucode update
succeeded only on part of cpus and warn a reboot is needed if it happened?
We definitely want to tolerate some kinds of hardware misbehavior. But
for such cases which are unlikely to happen, I prefer to improve this code
when we meet this kind of issue.

>> So perhaps there needs to be special casing of -EIO,
>> which gets returned when the ucode rev reported by the CPU after
>> the update does not match expectations.
>
>An to go one step further, perhaps we should also store more than
>just the newest variant for a given pf. If the newest fails to apply
>but there is another one newer than what's on a CPU, updating to
>that may work, and once that intermediate update worked, the
>update to the newest version may then work too.

Intel SDM doesn't mention this dependency (to apply an ucode relies on a
specific old ucode applied). Perhaps we can also assume we won't fall
into this case.

Hi Ashok,

Do you know whether Intel's ucode update mechanism has such dependency?

Thanks
Chao

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