[PATCH 0/5] Adding more robustness to microcode loading

[PATCH 0/5] Adding more robustness to microcode loading

[Ashok Raj]

Hi Boris and Thomas,

This is an attempt to move towards enabling late-load ON by default, and if
the taint flag can be removed after this patch series.

- Patch1: Documentation improvements. (to tainted-kernels.rst) and
  x86/microcode.rst.
- Patch2: (Intel) Fix in patch-match during an update left the old patch still in
  the list. This isn't necessary.
- Patch3: One key improvement is the addition of min_rev_id in the 
  microcode header.  This allows a way for CPU microcode to declare itself
  if this is suitable for late-loads.
- Patch4: Avoid any MCE's while a microcode update is in progress. This
  basically promotes any arriving MCE's to shutdown automatically.
- Patch5: Protect the secondary thread from entering NMI before a microcode
  update is complete in the primary thread.
  

Ashok Raj (5):
  x86/microcode: Add missing documentation that late-load will taint
    kernel
  x86/microcode/intel: Check against CPU signature before saving
    microcode
  x86/microcode/intel: Allow a late-load only if a min rev is specified
  x86/microcode: Avoid any chance of MCE's during microcode update
  x86/microcode: Handle NMI's during microcode update.

 Documentation/admin-guide/tainted-kernels.rst |  8 +-
 Documentation/x86/microcode.rst               | 95 +++++++++++++++++-
 arch/x86/include/asm/mce.h                    |  4 +
 arch/x86/include/asm/microcode_intel.h        |  4 +-
 arch/x86/kernel/cpu/mce/core.c                |  9 ++
 arch/x86/kernel/cpu/microcode/core.c          | 99 ++++++++++++++++++-
 arch/x86/kernel/cpu/microcode/intel.c         | 34 ++++++-
 7 files changed, 240 insertions(+), 13 deletions(-)

-- 
2.32.0

[PATCH 1/5] x86/microcode: Add missing documentation that late-load will taint kernel

[Ashok Raj]

Commit d23d33ea0fcd made late-load taint the kernel. Documentation for
tainted kernels was missed.

There is some history behind why x86 microcode started doing the late_load
stop_machine() rendezvous. It was suggested that it would be good
background to document.

No other functional change.

Signed-off-by: Ashok Raj <ashok.raj@intel.com>
---
 Documentation/admin-guide/tainted-kernels.rst |  8 +-
 Documentation/x86/microcode.rst               | 95 ++++++++++++++++++-
 2 files changed, 97 insertions(+), 6 deletions(-)

diff --git a/Documentation/admin-guide/tainted-kernels.rst b/Documentation/admin-guide/tainted-kernels.rst
index ceeed7b0798d..3513182b5dec 100644
--- a/Documentation/admin-guide/tainted-kernels.rst
+++ b/Documentation/admin-guide/tainted-kernels.rst
@@ -133,7 +133,13 @@ More detailed explanation for tainting
        scsi/snic on something else than x86_64, scsi/ips on non
        x86/x86_64/itanium, have broken firmware settings for the
        irqchip/irq-gic on arm64 ...).
-
+     - x86/x86_64: kernel "late-loads" of a microcode update
+       is dangerous and will result in tainting the kernel. Late loading
+       requires rendezvous to make sure we update when the system is
+       quiesce and not executing anything else. But the presence of
+       MCE/SMI/NMI can cause control flow away from the rendezvous
+       that can't be controlled
+e
  3)  ``R`` if a module was force unloaded by ``rmmod -f``, ``' '`` if all
      modules were unloaded normally.
 
diff --git a/Documentation/x86/microcode.rst b/Documentation/x86/microcode.rst
index a320d37982ed..941c48c26d84 100644
--- a/Documentation/x86/microcode.rst
+++ b/Documentation/x86/microcode.rst
@@ -6,6 +6,7 @@ The Linux Microcode Loader
 
 :Authors: - Fenghua Yu <fenghua.yu@intel.com>
           - Borislav Petkov <bp@suse.de>
+	  - Ashok Raj <ashok.raj@intel.com>
 
 The kernel has a x86 microcode loading facility which is supposed to
 provide microcode loading methods in the OS. Potential use cases are
@@ -92,12 +93,10 @@ vendor's site.
 Late loading
 ============
 
-There are two legacy user space interfaces to load microcode, either through
-/dev/cpu/microcode or through /sys/devices/system/cpu/microcode/reload file
+Loading microcode is through /sys/devices/system/cpu/microcode/reload file
 in sysfs.
 
-The /dev/cpu/microcode method is deprecated because it needs a special
-userspace tool for that.
+The /dev/cpu/microcode method is deprecated and removed since 5.19.
 
 The easier method is simply installing the microcode packages your distro
 supplies and running::
@@ -110,6 +109,92 @@ The loading mechanism looks for microcode blobs in
 /lib/firmware/{intel-ucode,amd-ucode}. The default distro installation
 packages already put them there.
 
+Why is late loading dangerous?
+==============================
+Starting 5.19, kernel configuration was changed to not compile
+late-loading by default.  Even if one were to enable the CONFIG, it
+will end up tainting the kernel.  Some of the reasons are explained
+below.
+
+Rendezvous all CPUs
+-------------------
+Microcode is a core resource, hence both threads of HT share the same
+microcode resource.  Updating on one thread of the core, the sibling
+automatically gets the update. There are some MSR's that are flow control
+(virtual) MSR's.  While the microcode update is in progress, these MSR's
+cease to exist.  This can result in unpredictable results, if the second
+sibling thread happens to be in the middle of an access to an MSR that
+is being patched at the same time. MSR's are just one common issue we
+trip over, but any other instruction that's being patched, can also result 
+in similar behavior.
+
+To eliminate this case, we introduced stop_machine() as a way to guarantee
+that the thread sibling will not execute any code  and just waits in a spin
+loop polling an atomic variable.
+
+While this took care of device or external interrupts, IPI's include LVT ones,
+such as CMCI etc, there are other special interrupts that can't be shut off.
+Those are Machine Checks (#MC), Systems Management Interrupt (#SMI) and
+Non-Maskable interrupts (#NMI).
+
+Machine Checks
+--------------
+Machine Checks (#MC) are non-maskable. There are two kinds of MCEs. Fatal
+un-recoverable MCEs and recoverable MCEs. While un-recoverable errors
+are fatal in behavior, recoverable errors can also happen in kernel context
+are also treated as fatal by the kernel.
+
+#MCE's are also broadcast to all threads in a system. If one thread is in
+the middle of wrmsr() MCE will be taken at the end of the flow. Either they will
+wait for this thread performing the wrmsr(0x79) to rendezvous in MCE handler
+and shutdown eventually if any of the threads in the system fail to checkin
+to the MCE rendezvous.
+
+To be paranoid and get predictable behavior, OS can choose to set
+MCG_STATUS.MCIP. Since MCE's can be at most one in a system, 
+if an MCE was signaled, the above condition will promote to a system reset
+automatically. OS can turn off MCIP at the end of the update for that core.
+
+System Management Interrupt
+---------------------------
+#SMI's are also broadcast via HW to all CPUs in the platform. Microcode
+updates requests exclusive access to the core before a wrmsr 0x79. So if it
+does happen such that, one thread is in wrmsr flow, and the 2nd got an SMI,
+that thread will be stopped in the first instruction in SMI handler.
+
+Since the secondary thread is stopped in the first instruction in SMI,
+there is very little chance that it would be in the middle of executing an
+instruction being patched. Plus OS has no way to stop SMI's from happening.
+
+Since there seems some reasonable behavior micro-architecturally (for
+Intel CPUs), it seems benign.
+
+Non-Maskable Interrupts
+-----------------------
+When thread0 of a core is doing the microcode update, if thread1 is pulled
+into NMI, that can cause unpredictable behavior due to reasons sighted
+above.
+
+OS can choose a variety of methods to avoid running into this situation.
+
+Is the microcode suitable for late loading?
+-------------------------------------------
+Late loading is done when system is fully operational and running real
+workloads. Late-loads behavior depends on what the base patch on the CPU is
+before upgrading to the new patch.
+
+This is true for Intel CPUs.
+
+For e.g. Consider CPU is on patch1 and we are updating to patch3.
+Between patch1 and patch3, patch2 might have deprecated a user visible
+feature. This can cause unexpected behavior in the kernel if the kernel
+happened to be using those features. For instance say MSR_X is no longer
+available after an update, accessing that MSR can cause #GP fault.
+
+Basically there is no way to declare a new microcode update suitable for
+late-loading. This is another one of the problems that caused late-loads to
+be disabled from being default enabled.
+
 Builtin microcode
 =================
 
@@ -134,7 +219,7 @@ This basically means, you have the following tree structure locally::
   |   |-- 06-3a-09
   ...
 
-so that the build system can find those files and integrate them into
+So that the build system can find those files and integrate them into
 the final kernel image. The early loader finds them and applies them.
 
 Needless to say, this method is not the most flexible one because it
-- 
2.32.0

[tip: x86/microcode] x86/microcode: Document the whole late loading problem

[tip-bot2 for Ashok Raj]

The following commit has been merged into the x86/microcode branch of tip:

Commit-ID:     35da8ad78e9b1a25d95a281966c439da1ef9a98a
Gitweb:        https://git.kernel.org/tip/35da8ad78e9b1a25d95a281966c439da1ef9a98a
Author:        Ashok Raj <ashok.raj@intel.com>
AuthorDate:    Sat, 13 Aug 2022 22:38:21 
Committer:     Borislav Petkov <bp@suse.de>
CommitterDate: Mon, 15 Aug 2022 21:24:54 +02:00

x86/microcode: Document the whole late loading problem

Commit

  d23d33ea0fcd ("x86/microcode: Taint and warn on late loading")

started tainting the kernel after microcode late loading.

There is some history behind why x86 microcode started doing the late
loading stop_machine() rendezvous. Document the whole situation.

No functional changes.

  [ bp: Fix typos, heavily massage. ]

Signed-off-by: Ashok Raj <ashok.raj@intel.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
Link: https://lore.kernel.org/r/20220813223825.3164861-2-ashok.raj@intel.com
---
 Documentation/admin-guide/tainted-kernels.rst |   8 +-
 Documentation/x86/microcode.rst               | 118 +++++++++++++++--
 2 files changed, 115 insertions(+), 11 deletions(-)

diff --git a/Documentation/admin-guide/tainted-kernels.rst b/Documentation/admin-guide/tainted-kernels.rst
index 7d80e8c..e59a710 100644
--- a/Documentation/admin-guide/tainted-kernels.rst
+++ b/Documentation/admin-guide/tainted-kernels.rst
@@ -134,7 +134,13 @@ More detailed explanation for tainting
        scsi/snic on something else than x86_64, scsi/ips on non
        x86/x86_64/itanium, have broken firmware settings for the
        irqchip/irq-gic on arm64 ...).
-
+     - x86/x86_64: Microcode late loading is dangerous and will result in
+       tainting the kernel. It requires that all CPUs rendezvous to make sure
+       the update happens when the system is as quiescent as possible. However,
+       a higher priority MCE/SMI/NMI can move control flow away from that
+       rendezvous and interrupt the update, which can be detrimental to the
+       machine.
+e
  3)  ``R`` if a module was force unloaded by ``rmmod -f``, ``' '`` if all
      modules were unloaded normally.
 
diff --git a/Documentation/x86/microcode.rst b/Documentation/x86/microcode.rst
index a320d37..21c56e0 100644
--- a/Documentation/x86/microcode.rst
+++ b/Documentation/x86/microcode.rst
@@ -6,6 +6,7 @@ The Linux Microcode Loader
 
 :Authors: - Fenghua Yu <fenghua.yu@intel.com>
           - Borislav Petkov <bp@suse.de>
+	  - Ashok Raj <ashok.raj@intel.com>
 
 The kernel has a x86 microcode loading facility which is supposed to
 provide microcode loading methods in the OS. Potential use cases are
@@ -92,15 +93,8 @@ vendor's site.
 Late loading
 ============
 
-There are two legacy user space interfaces to load microcode, either through
-/dev/cpu/microcode or through /sys/devices/system/cpu/microcode/reload file
-in sysfs.
-
-The /dev/cpu/microcode method is deprecated because it needs a special
-userspace tool for that.
-
-The easier method is simply installing the microcode packages your distro
-supplies and running::
+You simply install the microcode packages your distro supplies and
+run::
 
   # echo 1 > /sys/devices/system/cpu/microcode/reload
 
@@ -110,6 +104,110 @@ The loading mechanism looks for microcode blobs in
 /lib/firmware/{intel-ucode,amd-ucode}. The default distro installation
 packages already put them there.
 
+Since kernel 5.19, late loading is not enabled by default.
+
+The /dev/cpu/microcode method has been removed in 5.19.
+
+Why is late loading dangerous?
+==============================
+
+Synchronizing all CPUs
+----------------------
+
+The microcode engine which receives the microcode update is shared
+between the two logical threads in a SMT system. Therefore, when
+the update is executed on one SMT thread of the core, the sibling
+"automatically" gets the update.
+
+Since the microcode can "simulate" MSRs too, while the microcode update
+is in progress, those simulated MSRs transiently cease to exist. This
+can result in unpredictable results if the SMT sibling thread happens to
+be in the middle of an access to such an MSR. The usual observation is
+that such MSR accesses cause #GPs to be raised to signal that former are
+not present.
+
+The disappearing MSRs are just one common issue which is being observed.
+Any other instruction that's being patched and gets concurrently
+executed by the other SMT sibling, can also result in similar,
+unpredictable behavior.
+
+To eliminate this case, a stop_machine()-based CPU synchronization was
+introduced as a way to guarantee that all logical CPUs will not execute
+any code but just wait in a spin loop, polling an atomic variable.
+
+While this took care of device or external interrupts, IPIs including
+LVT ones, such as CMCI etc, it cannot address other special interrupts
+that can't be shut off. Those are Machine Check (#MC), System Management
+(#SMI) and Non-Maskable interrupts (#NMI).
+
+Machine Checks
+--------------
+
+Machine Checks (#MC) are non-maskable. There are two kinds of MCEs.
+Fatal un-recoverable MCEs and recoverable MCEs. While un-recoverable
+errors are fatal, recoverable errors can also happen in kernel context
+are also treated as fatal by the kernel.
+
+On certain Intel machines, MCEs are also broadcast to all threads in a
+system. If one thread is in the middle of executing WRMSR, a MCE will be
+taken at the end of the flow. Either way, they will wait for the thread
+performing the wrmsr(0x79) to rendezvous in the MCE handler and shutdown
+eventually if any of the threads in the system fail to check in to the
+MCE rendezvous.
+
+To be paranoid and get predictable behavior, the OS can choose to set
+MCG_STATUS.MCIP. Since MCEs can be at most one in a system, if an
+MCE was signaled, the above condition will promote to a system reset
+automatically. OS can turn off MCIP at the end of the update for that
+core.
+
+System Management Interrupt
+---------------------------
+
+SMIs are also broadcast to all CPUs in the platform. Microcode update
+requests exclusive access to the core before writing to MSR 0x79. So if
+it does happen such that, one thread is in WRMSR flow, and the 2nd got
+an SMI, that thread will be stopped in the first instruction in the SMI
+handler.
+
+Since the secondary thread is stopped in the first instruction in SMI,
+there is very little chance that it would be in the middle of executing
+an instruction being patched. Plus OS has no way to stop SMIs from
+happening.
+
+Non-Maskable Interrupts
+-----------------------
+
+When thread0 of a core is doing the microcode update, if thread1 is
+pulled into NMI, that can cause unpredictable behavior due to the
+reasons above.
+
+OS can choose a variety of methods to avoid running into this situation.
+
+
+Is the microcode suitable for late loading?
+-------------------------------------------
+
+Late loading is done when the system is fully operational and running
+real workloads. Late loading behavior depends on what the base patch on
+the CPU is before upgrading to the new patch.
+
+This is true for Intel CPUs.
+
+Consider, for example, a CPU has patch level 1 and the update is to
+patch level 3.
+
+Between patch1 and patch3, patch2 might have deprecated a software-visible
+feature.
+
+This is unacceptable if software is even potentially using that feature.
+For instance, say MSR_X is no longer available after an update,
+accessing that MSR will cause a #GP fault.
+
+Basically there is no way to declare a new microcode update suitable
+for late-loading. This is another one of the problems that caused late
+loading to be not enabled by default.
+
 Builtin microcode
 =================
 
@@ -134,7 +232,7 @@ This basically means, you have the following tree structure locally::
   |   |-- 06-3a-09
   ...
 
-so that the build system can find those files and integrate them into
+So that the build system can find those files and integrate them into
 the final kernel image. The early loader finds them and applies them.
 
 Needless to say, this method is not the most flexible one because it

Re: [tip: x86/microcode] x86/microcode: Document the whole late loading problem

[Ashok Raj]

Hi Boris,

Seems like there is an extraneous 'e' at the start of a line. Think this
existed in my patch patch, I noticed internally due to the 0day report that
a newline was missing.


On Mon, Aug 15, 2022 at 07:40:05PM -0000, tip-bot2 for Ashok Raj wrote:
[snip]
> diff --git a/Documentation/admin-guide/tainted-kernels.rst b/Documentation/admin-guide/tainted-kernels.rst
> index 7d80e8c..e59a710 100644
> --- a/Documentation/admin-guide/tainted-kernels.rst
> +++ b/Documentation/admin-guide/tainted-kernels.rst
> @@ -134,7 +134,13 @@ More detailed explanation for tainting
>         scsi/snic on something else than x86_64, scsi/ips on non
>         x86/x86_64/itanium, have broken firmware settings for the
>         irqchip/irq-gic on arm64 ...).
> -
> +     - x86/x86_64: Microcode late loading is dangerous and will result in
> +       tainting the kernel. It requires that all CPUs rendezvous to make sure
> +       the update happens when the system is as quiescent as possible. However,
> +       a higher priority MCE/SMI/NMI can move control flow away from that
> +       rendezvous and interrupt the update, which can be detrimental to the
> +       machine.
> +e

^^^^^^

Re: [tip: x86/microcode] x86/microcode: Document the whole late loading problem

[Borislav Petkov]

On Tue, Aug 16, 2022 at 03:21:11AM +0000, Ashok Raj wrote:
> Seems like there is an extraneous 'e' at the start of a line. Think this
> existed in my patch patch, I noticed internally due to the 0day report that
> a newline was missing.

Yeah, I saw it and then forgot about it. Fixed.

On Tue, Aug 16, 2022 at 08:51:12AM +0200, Ingo Molnar wrote:
> Small nit: this capitalization change is spurious. The 'so' is a 
> continuation of the sentence, not a separate sentence - now that paragraph 
> is actively weird & grammatically incorrect, starting with a 'So'.
> 
> Maybe:
> 
>    ... so that the build system can find those files and integrate them into

Yeah, fixed.

This change was really spurious and had nothing to do there.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette

Re: [tip: x86/microcode] x86/microcode: Document the whole late loading problem

[Ingo Molnar]

* tip-bot2 for Ashok Raj <tip-bot2@linutronix.de> wrote:

>  Builtin microcode
>  =================
>  
> @@ -134,7 +232,7 @@ This basically means, you have the following tree structure locally::
>    |   |-- 06-3a-09
>    ...
>  
> -so that the build system can find those files and integrate them into
> +So that the build system can find those files and integrate them into
>  the final kernel image. The early loader finds them and applies them.

Small nit: this capitalization change is spurious. The 'so' is a 
continuation of the sentence, not a separate sentence - now that paragraph 
is actively weird & grammatically incorrect, starting with a 'So'.

Maybe:

   ... so that the build system can find those files and integrate them into

Thanks,

	Ingo

[tip: x86/microcode] x86/microcode: Document the whole late loading problem

[tip-bot2 for Ashok Raj]

The following commit has been merged into the x86/microcode branch of tip:

Commit-ID:     764fc2a82fed0a4f0ce7444c34a3683bad06e403
Gitweb:        https://git.kernel.org/tip/764fc2a82fed0a4f0ce7444c34a3683bad06e403
Author:        Ashok Raj <ashok.raj@intel.com>
AuthorDate:    Sat, 13 Aug 2022 22:38:21 
Committer:     Borislav Petkov <bp@suse.de>
CommitterDate: Tue, 16 Aug 2022 09:37:24 +02:00

x86/microcode: Document the whole late loading problem

Commit

  d23d33ea0fcd ("x86/microcode: Taint and warn on late loading")

started tainting the kernel after microcode late loading.

There is some history behind why x86 microcode started doing the late
loading stop_machine() rendezvous. Document the whole situation.

No functional changes.

  [ bp: Fix typos, heavily massage. ]

Signed-off-by: Ashok Raj <ashok.raj@intel.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
Link: https://lore.kernel.org/r/20220813223825.3164861-2-ashok.raj@intel.com
---
 Documentation/admin-guide/tainted-kernels.rst |   6 +-
 Documentation/x86/microcode.rst               | 116 +++++++++++++++--
 2 files changed, 113 insertions(+), 9 deletions(-)

diff --git a/Documentation/admin-guide/tainted-kernels.rst b/Documentation/admin-guide/tainted-kernels.rst
index 7d80e8c..92a8a07 100644
--- a/Documentation/admin-guide/tainted-kernels.rst
+++ b/Documentation/admin-guide/tainted-kernels.rst
@@ -134,6 +134,12 @@ More detailed explanation for tainting
        scsi/snic on something else than x86_64, scsi/ips on non
        x86/x86_64/itanium, have broken firmware settings for the
        irqchip/irq-gic on arm64 ...).
+     - x86/x86_64: Microcode late loading is dangerous and will result in
+       tainting the kernel. It requires that all CPUs rendezvous to make sure
+       the update happens when the system is as quiescent as possible. However,
+       a higher priority MCE/SMI/NMI can move control flow away from that
+       rendezvous and interrupt the update, which can be detrimental to the
+       machine.
 
  3)  ``R`` if a module was force unloaded by ``rmmod -f``, ``' '`` if all
      modules were unloaded normally.
diff --git a/Documentation/x86/microcode.rst b/Documentation/x86/microcode.rst
index a320d37..b627c6f 100644
--- a/Documentation/x86/microcode.rst
+++ b/Documentation/x86/microcode.rst
@@ -6,6 +6,7 @@ The Linux Microcode Loader
 
 :Authors: - Fenghua Yu <fenghua.yu@intel.com>
           - Borislav Petkov <bp@suse.de>
+	  - Ashok Raj <ashok.raj@intel.com>
 
 The kernel has a x86 microcode loading facility which is supposed to
 provide microcode loading methods in the OS. Potential use cases are
@@ -92,15 +93,8 @@ vendor's site.
 Late loading
 ============
 
-There are two legacy user space interfaces to load microcode, either through
-/dev/cpu/microcode or through /sys/devices/system/cpu/microcode/reload file
-in sysfs.
-
-The /dev/cpu/microcode method is deprecated because it needs a special
-userspace tool for that.
-
-The easier method is simply installing the microcode packages your distro
-supplies and running::
+You simply install the microcode packages your distro supplies and
+run::
 
   # echo 1 > /sys/devices/system/cpu/microcode/reload
 
@@ -110,6 +104,110 @@ The loading mechanism looks for microcode blobs in
 /lib/firmware/{intel-ucode,amd-ucode}. The default distro installation
 packages already put them there.
 
+Since kernel 5.19, late loading is not enabled by default.
+
+The /dev/cpu/microcode method has been removed in 5.19.
+
+Why is late loading dangerous?
+==============================
+
+Synchronizing all CPUs
+----------------------
+
+The microcode engine which receives the microcode update is shared
+between the two logical threads in a SMT system. Therefore, when
+the update is executed on one SMT thread of the core, the sibling
+"automatically" gets the update.
+
+Since the microcode can "simulate" MSRs too, while the microcode update
+is in progress, those simulated MSRs transiently cease to exist. This
+can result in unpredictable results if the SMT sibling thread happens to
+be in the middle of an access to such an MSR. The usual observation is
+that such MSR accesses cause #GPs to be raised to signal that former are
+not present.
+
+The disappearing MSRs are just one common issue which is being observed.
+Any other instruction that's being patched and gets concurrently
+executed by the other SMT sibling, can also result in similar,
+unpredictable behavior.
+
+To eliminate this case, a stop_machine()-based CPU synchronization was
+introduced as a way to guarantee that all logical CPUs will not execute
+any code but just wait in a spin loop, polling an atomic variable.
+
+While this took care of device or external interrupts, IPIs including
+LVT ones, such as CMCI etc, it cannot address other special interrupts
+that can't be shut off. Those are Machine Check (#MC), System Management
+(#SMI) and Non-Maskable interrupts (#NMI).
+
+Machine Checks
+--------------
+
+Machine Checks (#MC) are non-maskable. There are two kinds of MCEs.
+Fatal un-recoverable MCEs and recoverable MCEs. While un-recoverable
+errors are fatal, recoverable errors can also happen in kernel context
+are also treated as fatal by the kernel.
+
+On certain Intel machines, MCEs are also broadcast to all threads in a
+system. If one thread is in the middle of executing WRMSR, a MCE will be
+taken at the end of the flow. Either way, they will wait for the thread
+performing the wrmsr(0x79) to rendezvous in the MCE handler and shutdown
+eventually if any of the threads in the system fail to check in to the
+MCE rendezvous.
+
+To be paranoid and get predictable behavior, the OS can choose to set
+MCG_STATUS.MCIP. Since MCEs can be at most one in a system, if an
+MCE was signaled, the above condition will promote to a system reset
+automatically. OS can turn off MCIP at the end of the update for that
+core.
+
+System Management Interrupt
+---------------------------
+
+SMIs are also broadcast to all CPUs in the platform. Microcode update
+requests exclusive access to the core before writing to MSR 0x79. So if
+it does happen such that, one thread is in WRMSR flow, and the 2nd got
+an SMI, that thread will be stopped in the first instruction in the SMI
+handler.
+
+Since the secondary thread is stopped in the first instruction in SMI,
+there is very little chance that it would be in the middle of executing
+an instruction being patched. Plus OS has no way to stop SMIs from
+happening.
+
+Non-Maskable Interrupts
+-----------------------
+
+When thread0 of a core is doing the microcode update, if thread1 is
+pulled into NMI, that can cause unpredictable behavior due to the
+reasons above.
+
+OS can choose a variety of methods to avoid running into this situation.
+
+
+Is the microcode suitable for late loading?
+-------------------------------------------
+
+Late loading is done when the system is fully operational and running
+real workloads. Late loading behavior depends on what the base patch on
+the CPU is before upgrading to the new patch.
+
+This is true for Intel CPUs.
+
+Consider, for example, a CPU has patch level 1 and the update is to
+patch level 3.
+
+Between patch1 and patch3, patch2 might have deprecated a software-visible
+feature.
+
+This is unacceptable if software is even potentially using that feature.
+For instance, say MSR_X is no longer available after an update,
+accessing that MSR will cause a #GP fault.
+
+Basically there is no way to declare a new microcode update suitable
+for late-loading. This is another one of the problems that caused late
+loading to be not enabled by default.
+
 Builtin microcode
 =================
 

[tip: x86/microcode] x86/microcode: Document the whole late loading problem

[tip-bot2 for Ashok Raj]

The following commit has been merged into the x86/microcode branch of tip:

Commit-ID:     3ecf671f1d354f40228e407ab350abd41034410b
Gitweb:        https://git.kernel.org/tip/3ecf671f1d354f40228e407ab350abd41034410b
Author:        Ashok Raj <ashok.raj@intel.com>
AuthorDate:    Sat, 13 Aug 2022 22:38:21 
Committer:     Borislav Petkov <bp@suse.de>
CommitterDate: Thu, 18 Aug 2022 15:57:53 +02:00

x86/microcode: Document the whole late loading problem

Commit

  d23d33ea0fcd ("x86/microcode: Taint and warn on late loading")

started tainting the kernel after microcode late loading.

There is some history behind why x86 microcode started doing the late
loading stop_machine() rendezvous. Document the whole situation.

No functional changes.

  [ bp: Fix typos, heavily massage. ]

Signed-off-by: Ashok Raj <ashok.raj@intel.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
Link: https://lore.kernel.org/r/20220813223825.3164861-2-ashok.raj@intel.com
---
 Documentation/admin-guide/tainted-kernels.rst |   6 +-
 Documentation/x86/microcode.rst               | 116 +++++++++++++++--
 2 files changed, 113 insertions(+), 9 deletions(-)

diff --git a/Documentation/admin-guide/tainted-kernels.rst b/Documentation/admin-guide/tainted-kernels.rst
index 7d80e8c..92a8a07 100644
--- a/Documentation/admin-guide/tainted-kernels.rst
+++ b/Documentation/admin-guide/tainted-kernels.rst
@@ -134,6 +134,12 @@ More detailed explanation for tainting
        scsi/snic on something else than x86_64, scsi/ips on non
        x86/x86_64/itanium, have broken firmware settings for the
        irqchip/irq-gic on arm64 ...).
+     - x86/x86_64: Microcode late loading is dangerous and will result in
+       tainting the kernel. It requires that all CPUs rendezvous to make sure
+       the update happens when the system is as quiescent as possible. However,
+       a higher priority MCE/SMI/NMI can move control flow away from that
+       rendezvous and interrupt the update, which can be detrimental to the
+       machine.
 
  3)  ``R`` if a module was force unloaded by ``rmmod -f``, ``' '`` if all
      modules were unloaded normally.
diff --git a/Documentation/x86/microcode.rst b/Documentation/x86/microcode.rst
index a320d37..b627c6f 100644
--- a/Documentation/x86/microcode.rst
+++ b/Documentation/x86/microcode.rst
@@ -6,6 +6,7 @@ The Linux Microcode Loader
 
 :Authors: - Fenghua Yu <fenghua.yu@intel.com>
           - Borislav Petkov <bp@suse.de>
+	  - Ashok Raj <ashok.raj@intel.com>
 
 The kernel has a x86 microcode loading facility which is supposed to
 provide microcode loading methods in the OS. Potential use cases are
@@ -92,15 +93,8 @@ vendor's site.
 Late loading
 ============
 
-There are two legacy user space interfaces to load microcode, either through
-/dev/cpu/microcode or through /sys/devices/system/cpu/microcode/reload file
-in sysfs.
-
-The /dev/cpu/microcode method is deprecated because it needs a special
-userspace tool for that.
-
-The easier method is simply installing the microcode packages your distro
-supplies and running::
+You simply install the microcode packages your distro supplies and
+run::
 
   # echo 1 > /sys/devices/system/cpu/microcode/reload
 
@@ -110,6 +104,110 @@ The loading mechanism looks for microcode blobs in
 /lib/firmware/{intel-ucode,amd-ucode}. The default distro installation
 packages already put them there.
 
+Since kernel 5.19, late loading is not enabled by default.
+
+The /dev/cpu/microcode method has been removed in 5.19.
+
+Why is late loading dangerous?
+==============================
+
+Synchronizing all CPUs
+----------------------
+
+The microcode engine which receives the microcode update is shared
+between the two logical threads in a SMT system. Therefore, when
+the update is executed on one SMT thread of the core, the sibling
+"automatically" gets the update.
+
+Since the microcode can "simulate" MSRs too, while the microcode update
+is in progress, those simulated MSRs transiently cease to exist. This
+can result in unpredictable results if the SMT sibling thread happens to
+be in the middle of an access to such an MSR. The usual observation is
+that such MSR accesses cause #GPs to be raised to signal that former are
+not present.
+
+The disappearing MSRs are just one common issue which is being observed.
+Any other instruction that's being patched and gets concurrently
+executed by the other SMT sibling, can also result in similar,
+unpredictable behavior.
+
+To eliminate this case, a stop_machine()-based CPU synchronization was
+introduced as a way to guarantee that all logical CPUs will not execute
+any code but just wait in a spin loop, polling an atomic variable.
+
+While this took care of device or external interrupts, IPIs including
+LVT ones, such as CMCI etc, it cannot address other special interrupts
+that can't be shut off. Those are Machine Check (#MC), System Management
+(#SMI) and Non-Maskable interrupts (#NMI).
+
+Machine Checks
+--------------
+
+Machine Checks (#MC) are non-maskable. There are two kinds of MCEs.
+Fatal un-recoverable MCEs and recoverable MCEs. While un-recoverable
+errors are fatal, recoverable errors can also happen in kernel context
+are also treated as fatal by the kernel.
+
+On certain Intel machines, MCEs are also broadcast to all threads in a
+system. If one thread is in the middle of executing WRMSR, a MCE will be
+taken at the end of the flow. Either way, they will wait for the thread
+performing the wrmsr(0x79) to rendezvous in the MCE handler and shutdown
+eventually if any of the threads in the system fail to check in to the
+MCE rendezvous.
+
+To be paranoid and get predictable behavior, the OS can choose to set
+MCG_STATUS.MCIP. Since MCEs can be at most one in a system, if an
+MCE was signaled, the above condition will promote to a system reset
+automatically. OS can turn off MCIP at the end of the update for that
+core.
+
+System Management Interrupt
+---------------------------
+
+SMIs are also broadcast to all CPUs in the platform. Microcode update
+requests exclusive access to the core before writing to MSR 0x79. So if
+it does happen such that, one thread is in WRMSR flow, and the 2nd got
+an SMI, that thread will be stopped in the first instruction in the SMI
+handler.
+
+Since the secondary thread is stopped in the first instruction in SMI,
+there is very little chance that it would be in the middle of executing
+an instruction being patched. Plus OS has no way to stop SMIs from
+happening.
+
+Non-Maskable Interrupts
+-----------------------
+
+When thread0 of a core is doing the microcode update, if thread1 is
+pulled into NMI, that can cause unpredictable behavior due to the
+reasons above.
+
+OS can choose a variety of methods to avoid running into this situation.
+
+
+Is the microcode suitable for late loading?
+-------------------------------------------
+
+Late loading is done when the system is fully operational and running
+real workloads. Late loading behavior depends on what the base patch on
+the CPU is before upgrading to the new patch.
+
+This is true for Intel CPUs.
+
+Consider, for example, a CPU has patch level 1 and the update is to
+patch level 3.
+
+Between patch1 and patch3, patch2 might have deprecated a software-visible
+feature.
+
+This is unacceptable if software is even potentially using that feature.
+For instance, say MSR_X is no longer available after an update,
+accessing that MSR will cause a #GP fault.
+
+Basically there is no way to declare a new microcode update suitable
+for late-loading. This is another one of the problems that caused late
+loading to be not enabled by default.
+
 Builtin microcode
 =================
 

[PATCH 2/5] x86/microcode/intel: Check against CPU signature before saving microcode

[Ashok Raj]

When save_microcode_patch() is looking to replace an existing microcode in
the cache, current code is *only* checks the CPU sig/pf in the main
header. Microcode can carry additional sig/pf combinations in the extended
signature table, which is completely missed today.

For e.g. Current patch is a multi-stepping patch and new incoming patch is
a specific patch just for this CPUs stepping.

patch1:
fms3 <--- header FMS
...
ext_sig:
fms1
fms2

patch2: new
fms2 <--- header FMS

Current code takes only fms3 and checks with patch2 fms2.

saved_patch.header.fms3 != new_patch.header.fms2, so save_microcode_patch
saves it to the end of list instead of replacing patch1 with patch2.

There is no functional user observable issue since find_patch() skips
patch versions that are <= current_patch and will land on patch2 properly.

Nevertheless this will just end up storing every patch that isn't required.
Kernel just needs to store the latest patch. Otherwise its a memory leak
that sits in kernel and never used.

Tested-by: William Xie <william.xie@intel.com>
Reported-by: William Xie <william.xie@intel.com>
Signed-off-by: Ashok Raj <ashok.raj@intel.com>
---
 arch/x86/kernel/cpu/microcode/intel.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 025c8f0cd948..c4b11e2fbe33 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -114,10 +114,18 @@ static void save_microcode_patch(struct ucode_cpu_info *uci, void *data, unsigne
 
 	list_for_each_entry_safe(iter, tmp, &microcode_cache, plist) {
 		mc_saved_hdr = (struct microcode_header_intel *)iter->data;
-		sig	     = mc_saved_hdr->sig;
-		pf	     = mc_saved_hdr->pf;
 
-		if (find_matching_signature(data, sig, pf)) {
+		sig = uci->cpu_sig.sig;
+		pf  = uci->cpu_sig.pf;
+
+		/*
+		 * Compare the current CPUs signature with the ones in the
+		 * cache to identify the right candidate to replace. At any
+		 * given time, we should have no more than one valid patch
+		 * file for a given CPU fms+pf in the cache list.
+		 */
+
+		if (find_matching_signature(iter->data, sig, pf)) {
 			prev_found = true;
 
 			if (mc_hdr->rev <= mc_saved_hdr->rev)
-- 
2.32.0

[PATCH 3/5] x86/microcode/intel: Allow a late-load only if a min rev is specified

[Ashok Raj]

In general users don't have the necessary information to determine
whether a late-load of a new microcode version has removed any feature
(MSR, CPUID etc) between what is currently loaded and this new microcode.
To address this issue, Intel has added a "minimum required version" field to
a previously reserved field in the file header. Microcode updates
should only be applied if the current microcode version is equal
to, or greater than this minimum required version.

https://lore.kernel.org/linux-kernel/alpine.DEB.2.21.1909062237580.1902@nanos.tec.linutronix.de/

Thomas made some suggestions on how meta-data in the microcode file could
provide Linux with information to decide if the new microcode is suitable
candidate for late-load. But even the "simpler" option#1 requires a lot of
metadata and corresponding kernel code to parse it.

The proposal here is an even simpler option. The criteria for a microcode to
be a viable late-load candidate is that no CPUID or OS visible MSR features
are removed with respect to an earlier version of the microcode.

Pseudocode for late-load is as follows:

if header.min_required_id == 0
	This is old format microcode, block late-load
else if current_ucode_version < header.min_required_id
	Current version is too old, block late-load of this microcode.
else
	OK to proceed with late-load.

Any microcode that removes a feature will set the min_version to itself.
This will enforce this microcode is not suitable for late-loading.

The enforcement is not in hardware and limited to kernel loader enforcing
the requirement. It is not required for early loading of microcode to
enforce this requirement, since the new features are only
evaluated after early loading in the boot process.


Test cases covered:

1. With new kernel, attempting to load an older format microcode with the
   min_rev=0 should be blocked by kernel.

   [  210.541802] microcode: Header MUST specify min version for late-load

2. New microcode with a non-zero min_rev in the header, but the specified
   min_rev is greater than what is currently loaded in the CPU should be
   blocked by kernel.

   245.139828] microcode: Current revision 0x8f685300 is too old to update,
must be at 0xaa000050 version or higher

3. New microcode with a min_rev < currently loaded should allow loading the
   microcode

4. Build initrd with microcode that has min_rev=0, or min_rev > currently
   loaded should permit early loading microcode from initrd.


Tested-by: William Xie <william.xie@intel.com>
Signed-off-by: Ashok Raj <ashok.raj@intel.com>
---
 arch/x86/include/asm/microcode_intel.h |  4 +++-
 arch/x86/kernel/cpu/microcode/intel.c  | 20 ++++++++++++++++++++
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/microcode_intel.h b/arch/x86/include/asm/microcode_intel.h
index 4c92cea7e4b5..16b8715e0984 100644
--- a/arch/x86/include/asm/microcode_intel.h
+++ b/arch/x86/include/asm/microcode_intel.h
@@ -14,7 +14,9 @@ struct microcode_header_intel {
 	unsigned int            pf;
 	unsigned int            datasize;
 	unsigned int            totalsize;
-	unsigned int            reserved[3];
+	unsigned int            reserved1;
+	unsigned int		min_req_id;
+	unsigned int            reserved3;
 };
 
 struct microcode_intel {
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index c4b11e2fbe33..1eb202ec2302 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -178,6 +178,7 @@ static int microcode_sanity_check(void *mc, int print_err)
 	struct extended_sigtable *ext_header = NULL;
 	u32 sum, orig_sum, ext_sigcount = 0, i;
 	struct extended_signature *ext_sig;
+	struct ucode_cpu_info uci;
 
 	total_size = get_totalsize(mc_header);
 	data_size = get_datasize(mc_header);
@@ -248,6 +249,25 @@ static int microcode_sanity_check(void *mc, int print_err)
 		return -EINVAL;
 	}
 
+	/*
+	 * Enforce for late-load that min_req_id is specified in the header.
+	 * Otherwise its an old format microcode, reject it.
+	 */
+	if (print_err) {
+		if (!mc_header->min_req_id) {
+			pr_warn("Header MUST specify min version for late-load\n");
+			return -EINVAL;
+		}
+
+		intel_cpu_collect_info(&uci);
+		if (uci.cpu_sig.rev < mc_header->min_req_id) {
+			pr_warn("Current revision 0x%x is too old to update,"
+				"must  be at 0x%x version or higher\n",
+				uci.cpu_sig.rev, mc_header->min_req_id);
+			return -EINVAL;
+		}
+	}
+
 	if (!ext_table_size)
 		return 0;
 
-- 
2.32.0

Re: [PATCH 3/5] x86/microcode/intel: Allow a late-load only if a min rev is specified

[Peter Zijlstra]

On Sat, Aug 13, 2022 at 10:38:23PM +0000, Ashok Raj wrote:

> The proposal here is an even simpler option. The criteria for a microcode to
> be a viable late-load candidate is that no CPUID or OS visible MSR features
> are removed with respect to an earlier version of the microcode.
> 
> Pseudocode for late-load is as follows:
> 
> if header.min_required_id == 0
> 	This is old format microcode, block late-load
> else if current_ucode_version < header.min_required_id
> 	Current version is too old, block late-load of this microcode.
> else
> 	OK to proceed with late-load.

What about ucode that adds CPUID bits? Since the kernel will not re-init
it will not pick up on those. But userspace might.

Should we at all time enable CPUID intercept to ensure user visible
CPUID doesn't change?

Re: [PATCH 3/5] x86/microcode/intel: Allow a late-load only if a min rev is specified

[Ashok Raj]

On Mon, Aug 15, 2022 at 09:43:00AM +0200, Peter Zijlstra wrote:
> On Sat, Aug 13, 2022 at 10:38:23PM +0000, Ashok Raj wrote:
> 
> > The proposal here is an even simpler option. The criteria for a microcode to
> > be a viable late-load candidate is that no CPUID or OS visible MSR features
> > are removed with respect to an earlier version of the microcode.
> > 
> > Pseudocode for late-load is as follows:
> > 
> > if header.min_required_id == 0
> > 	This is old format microcode, block late-load
> > else if current_ucode_version < header.min_required_id
> > 	Current version is too old, block late-load of this microcode.
> > else
> > 	OK to proceed with late-load.
> 
> What about ucode that adds CPUID bits? Since the kernel will not re-init
> it will not pick up on those. But userspace might.
> 
> Should we at all time enable CPUID intercept to ensure user visible
> CPUID doesn't change?

The protection is to make sure any existing users won't experience a
feature pulled under their feet.

Using new features aren't dangerous though, and should be permitted.

Re: [PATCH 3/5] x86/microcode/intel: Allow a late-load only if a min rev is specified

[Peter Zijlstra]

On Sat, Aug 13, 2022 at 10:38:23PM +0000, Ashok Raj wrote:

> The proposal here is an even simpler option. The criteria for a microcode to
> be a viable late-load candidate is that no CPUID or OS visible MSR features
> are removed with respect to an earlier version of the microcode.
> 
> Pseudocode for late-load is as follows:
> 
> if header.min_required_id == 0
> 	This is old format microcode, block late-load
> else if current_ucode_version < header.min_required_id
> 	Current version is too old, block late-load of this microcode.
> else
> 	OK to proceed with late-load.
> 
> Any microcode that removes a feature will set the min_version to itself.
> This will enforce this microcode is not suitable for late-loading.
> 
> The enforcement is not in hardware and limited to kernel loader enforcing
> the requirement. It is not required for early loading of microcode to
> enforce this requirement, since the new features are only
> evaluated after early loading in the boot process.
> 
> 
> Test cases covered:
> 
> 1. With new kernel, attempting to load an older format microcode with the
>    min_rev=0 should be blocked by kernel.
> 
>    [  210.541802] microcode: Header MUST specify min version for late-load
> 
> 2. New microcode with a non-zero min_rev in the header, but the specified
>    min_rev is greater than what is currently loaded in the CPU should be
>    blocked by kernel.
> 
>    245.139828] microcode: Current revision 0x8f685300 is too old to update,
> must be at 0xaa000050 version or higher
> 
> 3. New microcode with a min_rev < currently loaded should allow loading the
>    microcode
> 
> 4. Build initrd with microcode that has min_rev=0, or min_rev > currently
>    loaded should permit early loading microcode from initrd.

What if any validation do you have to ensure min_rev does as promised?
That is, ucode can very easily lie about the number and still remove an
MSR or CPUID enumerated feature.

Re: [PATCH 3/5] x86/microcode/intel: Allow a late-load only if a min rev is specified

[Ashok Raj]

On Mon, Aug 15, 2022 at 09:46:04AM +0200, Peter Zijlstra wrote:
> On Sat, Aug 13, 2022 at 10:38:23PM +0000, Ashok Raj wrote:
> 
> > The proposal here is an even simpler option. The criteria for a microcode to
> > be a viable late-load candidate is that no CPUID or OS visible MSR features
> > are removed with respect to an earlier version of the microcode.
> > 
> > Pseudocode for late-load is as follows:
> > 
> > if header.min_required_id == 0
> > 	This is old format microcode, block late-load
> > else if current_ucode_version < header.min_required_id
> > 	Current version is too old, block late-load of this microcode.
> > else
> > 	OK to proceed with late-load.
> > 
> > Any microcode that removes a feature will set the min_version to itself.
> > This will enforce this microcode is not suitable for late-loading.
> > 
> > The enforcement is not in hardware and limited to kernel loader enforcing
> > the requirement. It is not required for early loading of microcode to
> > enforce this requirement, since the new features are only
> > evaluated after early loading in the boot process.
> > 
> > 
> > Test cases covered:
> > 
> > 1. With new kernel, attempting to load an older format microcode with the
> >    min_rev=0 should be blocked by kernel.
> > 
> >    [  210.541802] microcode: Header MUST specify min version for late-load
> > 
> > 2. New microcode with a non-zero min_rev in the header, but the specified
> >    min_rev is greater than what is currently loaded in the CPU should be
> >    blocked by kernel.
> > 
> >    245.139828] microcode: Current revision 0x8f685300 is too old to update,
> > must be at 0xaa000050 version or higher
> > 
> > 3. New microcode with a min_rev < currently loaded should allow loading the
> >    microcode
> > 
> > 4. Build initrd with microcode that has min_rev=0, or min_rev > currently
> >    loaded should permit early loading microcode from initrd.
> 
> What if any validation do you have to ensure min_rev does as promised?

Today microcode release has a process by which these are packaged and
released. Qualifying a new update with a min version is a new step to their
process. This even limits their scope of validation to only revs >= min_rev
added in the header.

> That is, ucode can very easily lie about the number and still remove an
> MSR or CPUID enumerated feature.

Sorry I'm probably missing something. You mean someone maliciouly changes
the min_rev from what was released from Intel? 

OR

The release missed specifying a min-rev before release, accidently even
though its actually removing a feature? That would be in the bug category.

Release should have validation tests to cover all known feature bits and
such and check for any misses after an update as part of the qual process.

Cheers,
Ashok

Re: [PATCH 3/5] x86/microcode/intel: Allow a late-load only if a min rev is specified

[Peter Zijlstra]

On Mon, Aug 15, 2022 at 12:41:06PM +0000, Ashok Raj wrote:

> OR
> 
> The release missed specifying a min-rev before release, accidently even
> though its actually removing a feature? That would be in the bug category.
> 
> Release should have validation tests to cover all known feature bits and
> such and check for any misses after an update as part of the qual process.

This; it would be very unfortunate if this ever happened.

Re: [PATCH 3/5] x86/microcode/intel: Allow a late-load only if a min rev is specified

[Dave Hansen]

On 8/15/22 00:46, Peter Zijlstra wrote:
> What if any validation do you have to ensure min_rev does as promised?
> That is, ucode can very easily lie about the number and still remove an
> MSR or CPUID enumerated feature.

We can absolutely add sanity checks to this.  It would not be hard at
all to, for instance, dump out all the CPUID leaves we can get our hands
on and diff them before and after a ucode update.

That said, min_rev is *architectural*.  It includes an architectural
promise from Intel that the ucode won't lie.  If Intel is breaking
architectural promises, it has bigger problems on its hands.

Bugs happen, of course -- even bugs in architectural features.  If there
are enough bugs that we can't trust min_rev, late-loading will just get
disabled again, probably permanently.  Our Intel colleagues should have
all the incentive in the world to be very, very careful with min_rev.

[PATCH 4/5] x86/microcode: Avoid any chance of MCE's during microcode update

[Ashok Raj]

When a microcode update is in progress, several instructions and MSR's can
be patched by the update. During the update in progress, touching any of
the resources being patched could result in unpredictable results. If
thread0 is doing the update and thread1 happens to get a MCE, the handler
might read an MSR that's being patched.

In order to have predictable behavior, to avoid this scenario we set the MCIP in
all threads. Since MCE's can't be nested, HW will automatically promote to
shutdown condition.

After the update is completed, MCIP flag is cleared. The system is going to
shutdown anyway, since the MCE could be a fatal error, or even recoverable
errors in kernel space are treated as unrecoverable.

Signed-off-by: Ashok Raj <ashok.raj@intel.com>
---
 arch/x86/include/asm/mce.h           |  4 ++++
 arch/x86/kernel/cpu/mce/core.c       |  9 +++++++++
 arch/x86/kernel/cpu/microcode/core.c | 11 +++++++++++
 3 files changed, 24 insertions(+)

diff --git a/arch/x86/include/asm/mce.h b/arch/x86/include/asm/mce.h
index cc73061e7255..2aef6120e23f 100644
--- a/arch/x86/include/asm/mce.h
+++ b/arch/x86/include/asm/mce.h
@@ -207,12 +207,16 @@ void mcheck_cpu_init(struct cpuinfo_x86 *c);
 void mcheck_cpu_clear(struct cpuinfo_x86 *c);
 int apei_smca_report_x86_error(struct cper_ia_proc_ctx *ctx_info,
 			       u64 lapic_id);
+extern void mce_set_mcip(void);
+extern void mce_clear_mcip(void);
 #else
 static inline int mcheck_init(void) { return 0; }
 static inline void mcheck_cpu_init(struct cpuinfo_x86 *c) {}
 static inline void mcheck_cpu_clear(struct cpuinfo_x86 *c) {}
 static inline int apei_smca_report_x86_error(struct cper_ia_proc_ctx *ctx_info,
 					     u64 lapic_id) { return -EINVAL; }
+static inline void mce_set_mcip(void) {}
+static inline void mce_clear_mcip(void) {}
 #endif
 
 void mce_setup(struct mce *m);
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c
index 2c8ec5c71712..72b49d95bb3b 100644
--- a/arch/x86/kernel/cpu/mce/core.c
+++ b/arch/x86/kernel/cpu/mce/core.c
@@ -402,6 +402,15 @@ static noinstr void mce_wrmsrl(u32 msr, u64 v)
 		     : : "c" (msr), "a"(low), "d" (high) : "memory");
 }
 
+void mce_set_mcip(void)
+{
+	mce_wrmsrl(MSR_IA32_MCG_STATUS, 0x1);
+}
+
+void mce_clear_mcip(void)
+{
+	mce_wrmsrl(MSR_IA32_MCG_STATUS, 0x0);
+}
 /*
  * Collect all global (w.r.t. this processor) status about this machine
  * check into our "mce" struct so that we can use it later to assess
diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index ad57e0e4d674..d24e1c754c27 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -39,6 +39,7 @@
 #include <asm/processor.h>
 #include <asm/cmdline.h>
 #include <asm/setup.h>
+#include <asm/mce.h>
 
 #define DRIVER_VERSION	"2.2"
 
@@ -450,6 +451,14 @@ static int __reload_late(void *info)
 	if (__wait_for_cpus(&late_cpus_in, NSEC_PER_SEC))
 		return -1;
 
+	/*
+	 * Its dangerous to let MCE while microcode update is in progress.
+	 * Its extremely rare and even if happens they are fatal errors.
+	 * But reading patched areas before the update is complete can be
+	 * leading to unpredictable results. Setting MCIP will guarantee
+	 * the platform is taken to reset predictively.
+	 */
+	mce_set_mcip();
 	/*
 	 * On an SMT system, it suffices to load the microcode on one sibling of
 	 * the core because the microcode engine is shared between the threads.
@@ -457,6 +466,7 @@ static int __reload_late(void *info)
 	 * loading attempts happen on multiple threads of an SMT core. See
 	 * below.
 	 */
+
 	if (cpumask_first(topology_sibling_cpumask(cpu)) == cpu)
 		apply_microcode_local(&err);
 	else
@@ -473,6 +483,7 @@ static int __reload_late(void *info)
 	if (__wait_for_cpus(&late_cpus_out, NSEC_PER_SEC))
 		panic("Timeout during microcode update!\n");
 
+	mce_clear_mcip();
 	/*
 	 * At least one thread has completed update on each core.
 	 * For others, simply call the update to make sure the
-- 
2.32.0

[PATCH 5/5] x86/microcode: Handle NMI's during microcode update.

[Ashok Raj]

Microcode updates need a guarantee that the thread sibling that is waiting
for the update to finish on the primary core will not execute any
instructions until the update is complete. This is required to guarantee
any MSR or instruction that's being patched will be executed before the
update is complete.

After the stop_machine() rendezvous, an NMI handler is registered. If an
NMI were to happen while the microcode update is not complete, the
secondary thread will spin until the ucode update state is cleared.

Couple of choices discussed are:

1. Rendezvous inside the NMI handler, and also perform the update from
   within the handler. This seemed too risky and might cause instability
   with the races that we would need to solve. This would be a difficult
   choice.
2. Thomas (tglx) suggested that we could look into masking all the LVT
   originating NMI's. Such as LINT1, Perf control LVT entries and such.
   Since we are in the rendezvous loop, we don't need to worry about any
   NMI IPI's generated by the OS.

   The one we didn't have any control over is the ACPI mechanism of sending
   notifications to kernel for Firmware First Processing (FFM). Apparently
   it seems there is a PCH register that BIOS in SMI would write to
   generate such an interrupt (ACPI GHES).
3. This is a simpler option. OS registers an NMI handler and doesn't do any
   NMI rendezvous dance. But if an NMI were to happen, we check if any of
   the CPUs thread siblings have an update in progress. Only those CPUs
   would take an NMI. The thread performing the wrmsr() will only take an
   NMI after the completion of the wrmsr 0x79 flow.

Signed-off-by: Ashok Raj <ashok.raj@intel.com>
---
 arch/x86/kernel/cpu/microcode/core.c | 88 +++++++++++++++++++++++++++-
 1 file changed, 85 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/core.c b/arch/x86/kernel/cpu/microcode/core.c
index d24e1c754c27..ec10fa2db8b1 100644
--- a/arch/x86/kernel/cpu/microcode/core.c
+++ b/arch/x86/kernel/cpu/microcode/core.c
@@ -40,6 +40,7 @@
 #include <asm/cmdline.h>
 #include <asm/setup.h>
 #include <asm/mce.h>
+#include <asm/nmi.h>
 
 #define DRIVER_VERSION	"2.2"
 
@@ -411,6 +412,10 @@ static int check_online_cpus(void)
 
 static atomic_t late_cpus_in;
 static atomic_t late_cpus_out;
+static atomic_t nmi_cpus;
+static atomic_t nmi_timeouts;
+
+static struct cpumask cpus_in_wait;
 
 static int __wait_for_cpus(atomic_t *t, long long timeout)
 {
@@ -433,6 +438,53 @@ static int __wait_for_cpus(atomic_t *t, long long timeout)
 	return 0;
 }
 
+static int ucode_nmi_cb(unsigned int val, struct pt_regs *regs)
+{
+	int cpu = smp_processor_id();
+	int timeout = 100 * NSEC_PER_USEC;
+
+	atomic_inc(&nmi_cpus);
+	if (!cpumask_test_cpu(cpu, &cpus_in_wait))
+		return NMI_DONE;
+
+	while (timeout < NSEC_PER_USEC) {
+		if (timeout < NSEC_PER_USEC) {
+			atomic_inc(&nmi_timeouts);
+			break;
+		}
+		ndelay(SPINUNIT);
+		timeout -= SPINUNIT;
+		touch_nmi_watchdog();
+		if (!cpumask_test_cpu(cpu, &cpus_in_wait))
+			break;
+	}
+	return NMI_HANDLED;
+}
+
+static void set_nmi_cpus(struct cpumask *wait_mask)
+{
+	int first_cpu, wait_cpu, cpu = smp_processor_id();
+
+	first_cpu = cpumask_first(topology_sibling_cpumask(cpu));
+	for_each_cpu(wait_cpu, topology_sibling_cpumask(cpu)) {
+		if (wait_cpu == first_cpu)
+			continue;
+		cpumask_set_cpu(wait_cpu, wait_mask);
+	}
+}
+
+static void clear_nmi_cpus(struct cpumask *wait_mask)
+{
+	int first_cpu, wait_cpu, cpu = smp_processor_id();
+
+	first_cpu = cpumask_first(topology_sibling_cpumask(cpu));
+	for_each_cpu(wait_cpu, topology_sibling_cpumask(cpu)) {
+		if (wait_cpu == first_cpu)
+			continue;
+		cpumask_clear_cpu(wait_cpu, wait_mask);
+	}
+}
+
 /*
  * Returns:
  * < 0 - on error
@@ -440,7 +492,7 @@ static int __wait_for_cpus(atomic_t *t, long long timeout)
  */
 static int __reload_late(void *info)
 {
-	int cpu = smp_processor_id();
+	int first_cpu, cpu = smp_processor_id();
 	enum ucode_state err;
 	int ret = 0;
 
@@ -459,6 +511,7 @@ static int __reload_late(void *info)
 	 * the platform is taken to reset predictively.
 	 */
 	mce_set_mcip();
+
 	/*
 	 * On an SMT system, it suffices to load the microcode on one sibling of
 	 * the core because the microcode engine is shared between the threads.
@@ -466,9 +519,17 @@ static int __reload_late(void *info)
 	 * loading attempts happen on multiple threads of an SMT core. See
 	 * below.
 	 */
+	first_cpu = cpumask_first(topology_sibling_cpumask(cpu));
 
-	if (cpumask_first(topology_sibling_cpumask(cpu)) == cpu)
+	/*
+	 * Set the CPUs that we should hold in NMI until the primary has
+	 * completed the microcode update.
+	 */
+	if (first_cpu == cpu) {
+		set_nmi_cpus(&cpus_in_wait);
 		apply_microcode_local(&err);
+		clear_nmi_cpus(&cpus_in_wait);
+	}
 	else
 		goto wait_for_siblings;
 
@@ -502,20 +563,41 @@ static int __reload_late(void *info)
  */
 static int microcode_reload_late(void)
 {
-	int ret;
+	int ret = 0;
 
 	pr_err("Attempting late microcode loading - it is dangerous and taints the kernel.\n");
 	pr_err("You should switch to early loading, if possible.\n");
 
 	atomic_set(&late_cpus_in,  0);
 	atomic_set(&late_cpus_out, 0);
+	atomic_set(&nmi_cpus, 0);
+	atomic_set(&nmi_timeouts, 0);
+	cpumask_clear(&cpus_in_wait);
+
+	ret = register_nmi_handler(NMI_LOCAL, ucode_nmi_cb, NMI_FLAG_FIRST,
+				   "ucode_nmi");
+	if (ret) {
+		pr_err("Unable to register NMI handler\n");
+		goto done;
+	}
 
 	ret = stop_machine_cpuslocked(__reload_late, NULL, cpu_online_mask);
 	if (ret == 0)
 		microcode_check();
 
+	unregister_nmi_handler(NMI_LOCAL, "ucode_nmi");
+
+	if (atomic_read(&nmi_cpus))
+		pr_info("%d CPUs entered NMI while microcode update"
+			"in progress\n", atomic_read(&nmi_cpus));
+
+	if (atomic_read(&nmi_timeouts))
+		pr_err("Some CPUs [%d] entered NMI and timedout waiting for its"
+		       " mask to be cleared\n", atomic_read(&nmi_timeouts));
+
 	pr_info("Reload completed, microcode revision: 0x%x\n", boot_cpu_data.microcode);
 
+done:
 	return ret;
 }
 
-- 
2.32.0

Re: [PATCH 5/5] x86/microcode: Handle NMI's during microcode update.

[Andy Lutomirski]

On Sat, Aug 13, 2022, at 3:38 PM, Ashok Raj wrote:
> Microcode updates need a guarantee that the thread sibling that is waiting
> for the update to finish on the primary core will not execute any
> instructions until the update is complete. This is required to guarantee
> any MSR or instruction that's being patched will be executed before the
> update is complete.
>
> After the stop_machine() rendezvous, an NMI handler is registered. If an
> NMI were to happen while the microcode update is not complete, the
> secondary thread will spin until the ucode update state is cleared.
>
> Couple of choices discussed are:
>
> 1. Rendezvous inside the NMI handler, and also perform the update from
>    within the handler. This seemed too risky and might cause instability
>    with the races that we would need to solve. This would be a difficult
>    choice.

I prefer choice 1.  As I understand it, Xen has done this for a while to good effect.

If I were implementing this, I would rendezvous via stop_machine as usual.  Then I would set a flag or install a handler indicating that we are doing a microcode update, send NMI-to-self, and rendezvous in the NMI handler and do the update.

> 2. Thomas (tglx) suggested that we could look into masking all the LVT
>    originating NMI's. Such as LINT1, Perf control LVT entries and such.
>    Since we are in the rendezvous loop, we don't need to worry about any
>    NMI IPI's generated by the OS.
>
>    The one we didn't have any control over is the ACPI mechanism of sending
>    notifications to kernel for Firmware First Processing (FFM). Apparently
>    it seems there is a PCH register that BIOS in SMI would write to
>    generate such an interrupt (ACPI GHES).
> 3. This is a simpler option. OS registers an NMI handler and doesn't do any
>    NMI rendezvous dance. But if an NMI were to happen, we check if any of
>    the CPUs thread siblings have an update in progress. Only those CPUs
>    would take an NMI. The thread performing the wrmsr() will only take an
>    NMI after the completion of the wrmsr 0x79 flow.
>
> Signed-off-by: Ashok Raj <ashok.raj@intel.com>
> ---
>  arch/x86/kernel/cpu/microcode/core.c | 88 +++++++++++++++++++++++++++-
>  1 file changed, 85 insertions(+), 3 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/microcode/core.c 
> b/arch/x86/kernel/cpu/microcode/core.c
> index d24e1c754c27..ec10fa2db8b1 100644
> --- a/arch/x86/kernel/cpu/microcode/core.c
> +++ b/arch/x86/kernel/cpu/microcode/core.c
> @@ -40,6 +40,7 @@
>  #include <asm/cmdline.h>
>  #include <asm/setup.h>
>  #include <asm/mce.h>
> +#include <asm/nmi.h>
> 
>  #define DRIVER_VERSION	"2.2"
> 
> @@ -411,6 +412,10 @@ static int check_online_cpus(void)
> 
>  static atomic_t late_cpus_in;
>  static atomic_t late_cpus_out;
> +static atomic_t nmi_cpus;
> +static atomic_t nmi_timeouts;
> +
> +static struct cpumask cpus_in_wait;
> 
>  static int __wait_for_cpus(atomic_t *t, long long timeout)
>  {
> @@ -433,6 +438,53 @@ static int __wait_for_cpus(atomic_t *t, long long timeout)
>  	return 0;
>  }
> 
> +static int ucode_nmi_cb(unsigned int val, struct pt_regs *regs)
> +{
> +	int cpu = smp_processor_id();
> +	int timeout = 100 * NSEC_PER_USEC;
> +
> +	atomic_inc(&nmi_cpus);
> +	if (!cpumask_test_cpu(cpu, &cpus_in_wait))
> +		return NMI_DONE;
> +
> +	while (timeout < NSEC_PER_USEC) {
> +		if (timeout < NSEC_PER_USEC) {
> +			atomic_inc(&nmi_timeouts);
> +			break;
> +		}
> +		ndelay(SPINUNIT);
> +		timeout -= SPINUNIT;
> +		touch_nmi_watchdog();
> +		if (!cpumask_test_cpu(cpu, &cpus_in_wait))
> +			break;
> +	}
> +	return NMI_HANDLED;
> +}
> +
> +static void set_nmi_cpus(struct cpumask *wait_mask)
> +{
> +	int first_cpu, wait_cpu, cpu = smp_processor_id();
> +
> +	first_cpu = cpumask_first(topology_sibling_cpumask(cpu));
> +	for_each_cpu(wait_cpu, topology_sibling_cpumask(cpu)) {
> +		if (wait_cpu == first_cpu)
> +			continue;
> +		cpumask_set_cpu(wait_cpu, wait_mask);
> +	}
> +}
> +
> +static void clear_nmi_cpus(struct cpumask *wait_mask)
> +{
> +	int first_cpu, wait_cpu, cpu = smp_processor_id();
> +
> +	first_cpu = cpumask_first(topology_sibling_cpumask(cpu));
> +	for_each_cpu(wait_cpu, topology_sibling_cpumask(cpu)) {
> +		if (wait_cpu == first_cpu)
> +			continue;
> +		cpumask_clear_cpu(wait_cpu, wait_mask);
> +	}
> +}
> +
>  /*
>   * Returns:
>   * < 0 - on error
> @@ -440,7 +492,7 @@ static int __wait_for_cpus(atomic_t *t, long long timeout)
>   */
>  static int __reload_late(void *info)
>  {
> -	int cpu = smp_processor_id();
> +	int first_cpu, cpu = smp_processor_id();
>  	enum ucode_state err;
>  	int ret = 0;
> 
> @@ -459,6 +511,7 @@ static int __reload_late(void *info)
>  	 * the platform is taken to reset predictively.
>  	 */
>  	mce_set_mcip();
> +
>  	/*
>  	 * On an SMT system, it suffices to load the microcode on one sibling of
>  	 * the core because the microcode engine is shared between the threads.
> @@ -466,9 +519,17 @@ static int __reload_late(void *info)
>  	 * loading attempts happen on multiple threads of an SMT core. See
>  	 * below.
>  	 */
> +	first_cpu = cpumask_first(topology_sibling_cpumask(cpu));
> 
> -	if (cpumask_first(topology_sibling_cpumask(cpu)) == cpu)
> +	/*
> +	 * Set the CPUs that we should hold in NMI until the primary has
> +	 * completed the microcode update.
> +	 */
> +	if (first_cpu == cpu) {
> +		set_nmi_cpus(&cpus_in_wait);
>  		apply_microcode_local(&err);
> +		clear_nmi_cpus(&cpus_in_wait);
> +	}
>  	else
>  		goto wait_for_siblings;
> 
> @@ -502,20 +563,41 @@ static int __reload_late(void *info)
>   */
>  static int microcode_reload_late(void)
>  {
> -	int ret;
> +	int ret = 0;
> 
>  	pr_err("Attempting late microcode loading - it is dangerous and 
> taints the kernel.\n");
>  	pr_err("You should switch to early loading, if possible.\n");
> 
>  	atomic_set(&late_cpus_in,  0);
>  	atomic_set(&late_cpus_out, 0);
> +	atomic_set(&nmi_cpus, 0);
> +	atomic_set(&nmi_timeouts, 0);
> +	cpumask_clear(&cpus_in_wait);
> +
> +	ret = register_nmi_handler(NMI_LOCAL, ucode_nmi_cb, NMI_FLAG_FIRST,
> +				   "ucode_nmi");
> +	if (ret) {
> +		pr_err("Unable to register NMI handler\n");
> +		goto done;
> +	}
> 
>  	ret = stop_machine_cpuslocked(__reload_late, NULL, cpu_online_mask);
>  	if (ret == 0)
>  		microcode_check();
> 
> +	unregister_nmi_handler(NMI_LOCAL, "ucode_nmi");
> +
> +	if (atomic_read(&nmi_cpus))
> +		pr_info("%d CPUs entered NMI while microcode update"
> +			"in progress\n", atomic_read(&nmi_cpus));
> +
> +	if (atomic_read(&nmi_timeouts))
> +		pr_err("Some CPUs [%d] entered NMI and timedout waiting for its"
> +		       " mask to be cleared\n", atomic_read(&nmi_timeouts));
> +
>  	pr_info("Reload completed, microcode revision: 0x%x\n", 
> boot_cpu_data.microcode);
> 
> +done:
>  	return ret;
>  }
> 
> -- 
> 2.32.0

Re: [PATCH 5/5] x86/microcode: Handle NMI's during microcode update.

[Andy Lutomirski]

On Sat, Aug 13, 2022, at 5:13 PM, Andy Lutomirski wrote:
> On Sat, Aug 13, 2022, at 3:38 PM, Ashok Raj wrote:
>> Microcode updates need a guarantee that the thread sibling that is waiting
>> for the update to finish on the primary core will not execute any
>> instructions until the update is complete. This is required to guarantee
>> any MSR or instruction that's being patched will be executed before the
>> update is complete.
>>
>> After the stop_machine() rendezvous, an NMI handler is registered. If an
>> NMI were to happen while the microcode update is not complete, the
>> secondary thread will spin until the ucode update state is cleared.
>>
>> Couple of choices discussed are:
>>
>> 1. Rendezvous inside the NMI handler, and also perform the update from
>>    within the handler. This seemed too risky and might cause instability
>>    with the races that we would need to solve. This would be a difficult
>>    choice.
>
> I prefer choice 1.  As I understand it, Xen has done this for a while 
> to good effect.
>
> If I were implementing this, I would rendezvous via stop_machine as 
> usual.  Then I would set a flag or install a handler indicating that we 
> are doing a microcode update, send NMI-to-self, and rendezvous in the 
> NMI handler and do the update.
>

So I thought about this a bit more.

What's the actual goal?  Are we trying to execute no instructions at all on the sibling or are we trying to avoid executing nasty instructions like RDMSR and WRMSR?

If it's the former, we don't have a whole lot of choices.  We need the sibling to be in HLT or MWAIT, and we need NMIs masked or we need all likely NMI sources shut down.  If it's the latter, then we would ideally like to avoid a trip through the entry or exit code -- that code has nasty instructions (RDMSR in the paranoid path if !FSGSBASE, WRMSRs for mitigations, etc).  And we need to be extra careful: there are cases where NMIs are not actually masked but we just simulate NMIs being masked through the delightful logic in the entry code.  Off the top of my head, the NMI-entry-pretend-masked path probably doesn't execute nasty instructions other than IRET, but still, this might be fragile.

So here's my half-backed suggestion.  Add a new feature to the NMI entry code: at this point:

        /* Everything up to here is safe from nested NMIs */

At that point, NMIs are actually masked.  So check a flag indicating that we're trying to do the NMI-protected ucode patch dance.  If so, call a special noinstr C function (this part is distinctly nontrivial) that does the ucode work.  Now the stop_machine handler does NMI-to-self in a loop until it actually hits the special code path.

Alternatively, stop_machine, then change the IDT to a special one with a special non-IST NMI handler that does the dirty work.  NMI-to-self, do the ucode work, set the IDT back *inside the handler* so a latched NMI will do the right thing, and return.  This may be much, much simpler.

Or we stop messing around and do this for real.  Soft-offline the sibling, send it INIT, do the ucode patch, then SIPI, SIPI and resume the world.  What could possibly go wrong?

I have to say: Intel, can you please get your act together?  There is an entity in the system that is *actually* capable of doing this right: the microcode.

Re: [PATCH 5/5] x86/microcode: Handle NMI's during microcode update.

[Ashok Raj]

Hi Andy

On Sat, Aug 13, 2022 at 06:19:04PM -0700, Andy Lutomirski wrote:
> On Sat, Aug 13, 2022, at 5:13 PM, Andy Lutomirski wrote:
> > On Sat, Aug 13, 2022, at 3:38 PM, Ashok Raj wrote:
> >> Microcode updates need a guarantee that the thread sibling that is waiting
> >> for the update to finish on the primary core will not execute any
> >> instructions until the update is complete. This is required to guarantee
> >> any MSR or instruction that's being patched will be executed before the
> >> update is complete.
> >>
> >> After the stop_machine() rendezvous, an NMI handler is registered. If an
> >> NMI were to happen while the microcode update is not complete, the
> >> secondary thread will spin until the ucode update state is cleared.
> >>
> >> Couple of choices discussed are:
> >>
> >> 1. Rendezvous inside the NMI handler, and also perform the update from
> >>    within the handler. This seemed too risky and might cause instability
> >>    with the races that we would need to solve. This would be a difficult
> >>    choice.
> >
> > I prefer choice 1.  As I understand it, Xen has done this for a while 
> > to good effect.
> >
> > If I were implementing this, I would rendezvous via stop_machine as 
> > usual.  Then I would set a flag or install a handler indicating that we 
> > are doing a microcode update, send NMI-to-self, and rendezvous in the 
> > NMI handler and do the update.
> >
> 
> So I thought about this a bit more.
> 
> What's the actual goal?  Are we trying to execute no instructions at all on the sibling or are we trying to avoid executing nasty instructions like RDMSR and WRMSR?

Basically when the thread running wrmsr 0x79 asks for exclusive access to
the core, the second CPU is pulled into some ucode context, then the
primary thread updates the ucode. Secondary is released. But if the
secondary was in the middle of an instruction that is being patched, when
it resumes execution, it may go to some non-existent context and cause
weird things to happen. 

I'm not sure if the interrupt entry code does any fancy stuff, which case
dropping in the NMI handler early might be a better option.

I tried to do apic->sendIPIall(), then just wait for the 2 threads of a
core to rendezvous. Maybe instead I should have done the
selfIPI might be better. I'll do some more experiments with what I sent in
this patchset. 
> 
> If it's the former, we don't have a whole lot of choices.  We need the sibling to be in HLT or MWAIT, and we need NMIs masked or we need all likely NMI sources shut down.  If it's the latter, then we would ideally like to avoid a trip through the entry or exit code -- that code has nasty instructions (RDMSR in the paranoid path if !FSGSBASE, WRMSRs for mitigations, etc).  And we need to be extra careful: there are cases where NMIs are not actually masked but we just simulate NMIs being masked through the delightful logic in the entry code.  Off the top of my head, the NMI-entry-pretend-masked path probably doesn't execute nasty instructions other than IRET, but still, this might be fragile.

Remember, mwait() was patched that caused pain.. I remember Thomas
mentioned this a while back.

> 
> Or we stop messing around and do this for real.  Soft-offline the sibling, send it INIT, do the ucode patch, then SIPI, SIPI and resume the world.  What could possibly go wrong?


All these are looking more complicated, and might add some significant
latency. We might as well invoke SMI and let BIOS SMM handler to the update
:-)

> 
> I have to say: Intel, can you please get your act together?  There is an entity in the system that is *actually* capable of doing this right: the microcode.

So we have it.. this dirtyness is for all the current gen products.. Much
improved microcode loading is on the way. 

Re: [PATCH 5/5] x86/microcode: Handle NMI's during microcode update.

[Ashok Raj]

On Sat, Aug 13, 2022 at 05:13:13PM -0700, Andy Lutomirski wrote:
> 
> 
> On Sat, Aug 13, 2022, at 3:38 PM, Ashok Raj wrote:
> > Microcode updates need a guarantee that the thread sibling that is waiting
> > for the update to finish on the primary core will not execute any
> > instructions until the update is complete. This is required to guarantee
> > any MSR or instruction that's being patched will be executed before the
> > update is complete.
> >
> > After the stop_machine() rendezvous, an NMI handler is registered. If an
> > NMI were to happen while the microcode update is not complete, the
> > secondary thread will spin until the ucode update state is cleared.
> >
> > Couple of choices discussed are:
> >
> > 1. Rendezvous inside the NMI handler, and also perform the update from
> >    within the handler. This seemed too risky and might cause instability
> >    with the races that we would need to solve. This would be a difficult
> >    choice.
> 
> I prefer choice 1.  As I understand it, Xen has done this for a while to good effect.
> 
> If I were implementing this, I would rendezvous via stop_machine as usual.  Then I would set a flag or install a handler indicating that we are doing a microcode update, send NMI-to-self, and rendezvous in the NMI handler and do the update.

Well, that is exactly what I did for the first attempt. The code looked so
beautiful in the eyes of the creator :-) but somehow I couldn't get it to
not lock up. But the new implementation seemed to be more efficient. We do
nothing until the secondary drops in the NMI handler, and then hold them
hostage right there.

I thought this was slightly improved, in the sense we don't take extra hit
for sending and receiving interrupts. 

In my first attempt I didn't only rendezvous between threads of the core.
Everything looked great but just didn't work for me in time.

Cheers,
Ashok

Re: [PATCH 5/5] x86/microcode: Handle NMI's during microcode update.

[Andrew Cooper]

On 14/08/2022 03:54, Ashok Raj wrote:
> On Sat, Aug 13, 2022 at 05:13:13PM -0700, Andy Lutomirski wrote:
>>
>> On Sat, Aug 13, 2022, at 3:38 PM, Ashok Raj wrote:
>>> Microcode updates need a guarantee that the thread sibling that is waiting
>>> for the update to finish on the primary core will not execute any
>>> instructions until the update is complete. This is required to guarantee
>>> any MSR or instruction that's being patched will be executed before the
>>> update is complete.
>>>
>>> After the stop_machine() rendezvous, an NMI handler is registered. If an
>>> NMI were to happen while the microcode update is not complete, the
>>> secondary thread will spin until the ucode update state is cleared.
>>>
>>> Couple of choices discussed are:
>>>
>>> 1. Rendezvous inside the NMI handler, and also perform the update from
>>>    within the handler. This seemed too risky and might cause instability
>>>    with the races that we would need to solve. This would be a difficult
>>>    choice.
>> I prefer choice 1.  As I understand it, Xen has done this for a while to good effect.
>>
>> If I were implementing this, I would rendezvous via stop_machine as usual.  Then I would set a flag or install a handler indicating that we are doing a microcode update, send NMI-to-self, and rendezvous in the NMI handler and do the update.
> Well, that is exactly what I did for the first attempt. The code looked so
> beautiful in the eyes of the creator :-) but somehow I couldn't get it to
> not lock up.

So the way we do this in Xen is to rendezvous in stop machine, then have
only the siblings self-NMI.  The primary threads don't need to be in NMI
context, because the WRMSR to trigger the update *is* atomic with NMIs.

However, you do need to make sure that the NMI wait loop knows not to
wait for primary threads, otherwise you can deadlock when taking an NMI
on a primary thread between setting up the NMI handler and actually
issuing the update.

~Andrew

Re: [PATCH 5/5] x86/microcode: Handle NMI's during microcode update.

[Ashok Raj]

Hi Andrew,

On Sun, Aug 14, 2022 at 11:58:17AM +0000, Andrew Cooper wrote:
> >> If I were implementing this, I would rendezvous via stop_machine as usual.  Then I would set a flag or install a handler indicating that we are doing a microcode update, send NMI-to-self, and rendezvous in the NMI handler and do the update.
> > Well, that is exactly what I did for the first attempt. The code looked so
> > beautiful in the eyes of the creator :-) but somehow I couldn't get it to
> > not lock up.
> 
> So the way we do this in Xen is to rendezvous in stop machine, then have
> only the siblings self-NMI.  The primary threads don't need to be in NMI
> context, because the WRMSR to trigger the update *is* atomic with NMIs.
> 
> However, you do need to make sure that the NMI wait loop knows not to
> wait for primary threads, otherwise you can deadlock when taking an NMI
> on a primary thread between setting up the NMI handler and actually
> issuing the update.
> 

I'm almost sure that was the deadlock I ran into. You are correct, the
primary thread doesn't need to be in NMI, since once the wrmsr starts,  it
can't be interrupted.

But the primary needs to wait until its own siblings have dropped into NMI.
Before proceeding to perform wrmsr.

in stop_machine() handler, primary thread waits for its thread siblings to
enter NMI and report itself. Siblings will simply self IPI and then proceed
to wait for exit_sync

then primary does the wrmsr flow
clears the wait_cpus mask so that secondary inside NMI hander can release
itself

resync at exit rendezvous.

I have this coded, will test and repost.

Cheers,
Ashok