mbox series

[v10,00/16] improve late microcode loading

Message ID 1568272949-1086-1-git-send-email-chao.gao@intel.com (mailing list archive)
Headers show
Series improve late microcode loading | expand

Message

Chao Gao Sept. 12, 2019, 7:22 a.m. UTC
Major changes in version 10:
 - add back the patch to call wbinvd() conditionally
 - add a patch to disable late loading due to BDF90
 - rendezvous CPUs in NMI handler and load ucode. But provide an option
 to disable this behavior.
 - avoid the call of self_nmi() on the control thread because it may
 trigger the unknown_nmi_error() in do_nmi().
 - ensure ->start_update is called during system resuming from
 suspension

Sergey, could you help to test this series on an AMD machine?
Regarding changes to AMD side, I didn't do any test for them due to
lack of hardware. At least, two basic tests are needed:
* do a microcode update after system bootup
* don't bring all pCPUs up at bootup by specifying maxcpus option in xen
  command line and then do a microcode update and online all offlined
  CPUs via 'xen-hptool'.

The intention of this series is to make the late microcode loading
more reliable by rendezvousing all cpus in stop_machine context.
This idea comes from Ashok. I am porting his linux patch to Xen
(see patch 12 more details).

This series includes below changes:
 1. Patch 1-11: introduce a global microcode cache and some cleanup
 2. Patch 12: synchronize late microcode loading
 3. Patch 13: support parallel microcodes update on different cores
 4. Patch 14: block #NMI handling during microcode loading
 5. Patch 15: disable late ucode loading due to BDF90
 6. Patch 16: call wbinvd() conditionally

Currently, late microcode loading does a lot of things including
parsing microcode blob, checking the signature/revision and performing
update. Putting all of them into stop_machine context is a bad idea
because of complexity (one issue I observed is memory allocation
triggered one assertion in stop_machine context). To simplify the
load process, parsing microcode is moved out of the load process.
Remaining parts of load process is put to stop_machine context.

Previous change log:
Changes in version 9:
 - add Jan's Reviewed-by
 - rendevzous threads in NMI handler to disable NMI. Note that NMI can
 be served as usual on threads that are chosen to initiate ucode loading
 on each core.
 - avoid unnecessary memory allocation or copy when creating a microcode
 patch (patch 12)
 - rework patch 1 to avoid microcode_update_match() being used to
 compare two arbitrary updates.
 - call .end_update in early loading path.

Changes in version 8:
 - block #NMI handling during microcode loading (Patch 16)
 - Don't assume that all CPUs in the system have loaded a same ucode.
 So when parsing a blob, we attempt to save a patch as long as it matches
 with current cpu signature regardless of the revision of the patch.
 And also for loading, we only require the patch to be loaded isn't old
 than the cached one.
 - store an update after the first successful loading on a CPU
 - remove the patch that calls wbinvd() unconditionally before microcode
 loading. It is under internal discussion.
 - divide two big patches into several patches to improve readability.

Changes in version 7:
 - cache one microcode update rather than a list of it. Assuming that all CPUs
 (including those will be plugged in later) in the system have the same
 signature, one update matches with one CPU should match with others. Thus, one
 update is enough for microcode updating during CPU hot-plug and resuming.
 - To handle load failure, microcode update is cached after it is applied to
 avoid a broken update overriding a validated one. Unvalidated microcode updates
 are passed by arguments rather than another global variable, where this series
 slightly differs from Roger's suggestion in:
 https://lists.xen.org/archives/html/xen-devel/2019-03/msg00776.html
 - incorporate Sergey's patch (patch 10) to fix a bug: we maintain a variable
 to reflect current microcode revision. But in some cases, this variable isn't
 initialized during system boot time, which results in falsely reporting that
 processor is susceptible to some known vulnerabilities.
 - fix issues reported by Sergey:
 https://lists.xenproject.org/archives/html/xen-devel/2019-03/msg00901.html
 - Responses to Sergey/Roger/Wei/Ashok's other comments.

Major changes in version 6:
 - run wbinvd before updating microcode (patch 10)
 - add an userspace tool for late microcode update (patch 1)
 - scale time to wait by the number of remaining CPUs to respond 
 - remove 'cpu' parameters from some related callbacks and functins
 - save an ucode patch only if its supported CPU is allowed to mix with
   current cpu.

Changes in version 5:
 - support parallel microcode updates for all cores (see patch 8)
 - Address Roger's comments on the last version.

Chao Gao (16):
  microcode/intel: extend microcode_update_match()
  microcode/amd: distinguish old and mismatched ucode in
    microcode_fits()
  microcode: introduce a global cache of ucode patch
  microcode: clean up microcode_resume_cpu
  microcode: remove struct ucode_cpu_info
  microcode: remove pointless 'cpu' parameter
  microcode/amd: call svm_host_osvw_init() in common code
  microcode: pass a patch pointer to apply_microcode()
  microcode: split out apply_microcode() from cpu_request_microcode()
  microcode: unify ucode loading during system bootup and resuming
  microcode: reduce memory allocation and copy when creating a patch
  x86/microcode: Synchronize late microcode loading
  microcode: remove microcode_update_lock
  microcode: rendezvous CPUs in NMI handler and load ucode
  microcode: disable late loading if CPUs are affected by BDF90
  microcode/intel: writeback and invalidate cache conditionally

 docs/misc/xen-command-line.pandoc |  10 +
 xen/arch/x86/acpi/power.c         |   2 +-
 xen/arch/x86/apic.c               |   2 +-
 xen/arch/x86/microcode.c          | 569 ++++++++++++++++++++++++++++++--------
 xen/arch/x86/microcode_amd.c      | 282 +++++++++----------
 xen/arch/x86/microcode_intel.c    | 269 +++++++++++-------
 xen/arch/x86/smpboot.c            |   5 +-
 xen/arch/x86/spec_ctrl.c          |   2 +-
 xen/arch/x86/traps.c              |   6 +-
 xen/include/asm-x86/microcode.h   |  43 +--
 xen/include/asm-x86/nmi.h         |   3 +
 xen/include/asm-x86/processor.h   |   4 +-
 12 files changed, 788 insertions(+), 409 deletions(-)

Comments

Jan Beulich Sept. 13, 2019, 8:47 a.m. UTC | #1
On 12.09.2019 09:22, Chao Gao wrote:
> This series includes below changes:
>  1. Patch 1-11: introduce a global microcode cache and some cleanup
>  2. Patch 12: synchronize late microcode loading
>  3. Patch 13: support parallel microcodes update on different cores
>  4. Patch 14: block #NMI handling during microcode loading
>  5. Patch 15: disable late ucode loading due to BDF90
>  6. Patch 16: call wbinvd() conditionally

I don't know why it didn't occur to me earlier, but what about
parked / offlined CPUs? They'll have their ucode updated when they
get brought back online, but until then their ucode will disagree
with that of the online CPUs. For truly offline CPUs this may be
fine, but parked ones should probably be updated, perhaps via the
same approach as used when C-state data becomes available (see
set_cx_pminfo())?

Jan
Chao Gao Sept. 17, 2019, 7:09 a.m. UTC | #2
On Fri, Sep 13, 2019 at 10:47:36AM +0200, Jan Beulich wrote:
>On 12.09.2019 09:22, Chao Gao wrote:
>> This series includes below changes:
>>  1. Patch 1-11: introduce a global microcode cache and some cleanup
>>  2. Patch 12: synchronize late microcode loading
>>  3. Patch 13: support parallel microcodes update on different cores
>>  4. Patch 14: block #NMI handling during microcode loading
>>  5. Patch 15: disable late ucode loading due to BDF90
>>  6. Patch 16: call wbinvd() conditionally
>
>I don't know why it didn't occur to me earlier, but what about
>parked / offlined CPUs? They'll have their ucode updated when they
>get brought back online, but until then their ucode will disagree
>with that of the online CPUs. For truly offline CPUs this may be
>fine, but parked ones should probably be updated, perhaps via the
>same approach as used when C-state data becomes available (see
>set_cx_pminfo())?

Yes. It provides a means to wake up the parked CPU and a chance to run
some code (like loading ucode). But parked CPUs are cleared from
sibling info and cpu_online_map (see __cpu_disable()). If parallel
ucode loading is expected on parked CPUs, we should be able to
determine the primary threads and the number of cores no matter it is
online or parked. To this end, a new sibling map should be maintained
for each CPU and this map isn't changed when a CPU gets parked.

In Linux kernel, the approach is quite simple: late loading is
prohibited if any CPU is parked; admin should online all parked CPU
before loading ucode.

Do you have any preference?

Thanks
Chao
Jan Beulich Sept. 17, 2019, 7:11 a.m. UTC | #3
On 17.09.2019 09:09, Chao Gao wrote:
> On Fri, Sep 13, 2019 at 10:47:36AM +0200, Jan Beulich wrote:
>> On 12.09.2019 09:22, Chao Gao wrote:
>>> This series includes below changes:
>>>  1. Patch 1-11: introduce a global microcode cache and some cleanup
>>>  2. Patch 12: synchronize late microcode loading
>>>  3. Patch 13: support parallel microcodes update on different cores
>>>  4. Patch 14: block #NMI handling during microcode loading
>>>  5. Patch 15: disable late ucode loading due to BDF90
>>>  6. Patch 16: call wbinvd() conditionally
>>
>> I don't know why it didn't occur to me earlier, but what about
>> parked / offlined CPUs? They'll have their ucode updated when they
>> get brought back online, but until then their ucode will disagree
>> with that of the online CPUs. For truly offline CPUs this may be
>> fine, but parked ones should probably be updated, perhaps via the
>> same approach as used when C-state data becomes available (see
>> set_cx_pminfo())?
> 
> Yes. It provides a means to wake up the parked CPU and a chance to run
> some code (like loading ucode). But parked CPUs are cleared from
> sibling info and cpu_online_map (see __cpu_disable()). If parallel
> ucode loading is expected on parked CPUs, we should be able to
> determine the primary threads and the number of cores no matter it is
> online or parked. To this end, a new sibling map should be maintained
> for each CPU and this map isn't changed when a CPU gets parked.

Would this really be necessary? If any thread in a core is not
parked, bringing up the parked threads is unnecessary. If all
threads of a core are parked, simply nudge the thread with ID
zero?

> In Linux kernel, the approach is quite simple: late loading is
> prohibited if any CPU is parked; admin should online all parked CPU
> before loading ucode.

Well, this is certainly an option, but (as per above) I think
this is too rigid: Refusing the operation would be necessary
only if there's a core with all of its threads parked. (I'd
in particular like late loading to work in SMT-disabled mode.)

Jan