[PATCH 0/1] x86/microcode: Revert cache flush on Intel microcode loading

[PATCH 0/1] x86/microcode: Revert cache flush on Intel microcode loading

[Chang S. Bae]

Hi All,

During the process of aligning our internal code with the upstream code,
Yan reported significantly increased late loading times. William
identified the cause as the cache writeback and invalidation in the
microcode update path. This patch [1] appears to have been around our
platform-specific branches before.

The changelog [2] explains that the flush can guarantee a successful
microcode update and mentions Broadwell parts as an example, without
specifying any erratum. After discussing with some related folks, the
erratum [3] was clarified as the reason for the flush.

However, the affected revisions on the relevant Broadwell models have
already been blacklisted, making this flush obsolete. Initially, I was
quite confused that the two approaches ([4,5,6] and [7]) were dealing
with the same issue. Unfortunately, that is the case, as I double-checked
with the author.

This cache flush does not come without a cost. In older parts like
Broadwell and Skylake systems, for example, it takes about 3.5x more time
than WRMSR for the microcode update.

I’d like to ensure the patch description is clear enough to describe the
whole story for the record, as I am posting this revert.

Thanks,
Chang

[1] “Drop wbinvd() from microcode loading”:
    https://lore.kernel.org/lkml/20230130213955.6046-9-ashok.raj@intel.com/
[2] Commit 91df9fdf5149 (“x86/microcode/intel: Writeback and invalidate
    caches before updating microcode”)
[3] BDX90 - Loading Microcode Updates or Executing an Authenticated Code
    Module May Result in a System Hang. Details can be found in Intel
    Xeon E7-8800/4800 v4 Processor Product Family, Specification Update,
    August 2020:
    https://www.intel.com/content/dam/www/public/us/en/documents/specification-updates/xeon-e7-v4-spec-update.pdf
[4] Commit 723f2828a98c (“x86/microcode/intel: Disable late loading on
    model 79”)
[5] Commit b94b73733171 (“x86/microcode/intel: Extend BDW late-loading
    with a revision check”)
[6] Commit 7e702d17ed13 (“x86/microcode/intel: Extend BDW late-loading
    further with LLC size check”)
[7] Commit 91df9fdf5149 (“x86/microcode/intel: Writeback and invalidate
    caches before updating microcode”)

Chang S. Bae (1):
  arch/x86/microcode/intel: Remove unnecessary cache writeback and
    invalidation

 arch/x86/kernel/cpu/microcode/intel.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

-- 
2.34.1

[PATCH 1/1] arch/x86/microcode/intel: Remove unnecessary cache writeback and invalidation

[Chang S. Bae]

Currently, an unconditional cache flush is conducted during every
microcode update. Although its changelog did not specify any erratum,
this was primarily intended to address a specific microcode bug, a load
of which has already been blocked by is_blacklisted(). Therefore, this
flush is unnecessary.

Additionally, the side effects of doing this have been overlooked. It
extends the CPU rendezvous time for late loading. The cache flush takes
about 1x to 3.5x more time than needed for updating the microcode.

Remove native_wbinvd() and update the erratum name to align with the
latest errata documentation.

Fixes: 91df9fdf5149 ("x86/microcode/intel: Writeback and invalidate caches before updating microcode")
Reported-by: Yan Hua Wu <yanhua1.wu@intel.com>
Reported-by: William Xie <william.xie@intel.com>
Signed-off-by: Chang S. Bae <chang.seok.bae@intel.com>
Tested-by: Yan Hua Wu <yanhua1.wu@intel.com>
---
Note: Errata names have been updated from BDF to BDX according to the
latest documentation:
https://www.intel.com/content/dam/www/public/us/en/documents/specification-updates/xeon-e7-v4-spec-update.pdf
---
 arch/x86/kernel/cpu/microcode/intel.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 815fa67356a2..f3d534807d91 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -319,12 +319,6 @@ static enum ucode_state __apply_microcode(struct ucode_cpu_info *uci,
 		return UCODE_OK;
 	}
 
-	/*
-	 * Writeback and invalidate caches before updating microcode to avoid
-	 * internal issues depending on what the microcode is updating.
-	 */
-	native_wbinvd();
-
 	/* write microcode via MSR 0x79 */
 	native_wrmsrl(MSR_IA32_UCODE_WRITE, (unsigned long)mc->bits);
 
@@ -574,14 +568,14 @@ static bool is_blacklisted(unsigned int cpu)
 	/*
 	 * Late loading on model 79 with microcode revision less than 0x0b000021
 	 * and LLC size per core bigger than 2.5MB may result in a system hang.
-	 * This behavior is documented in item BDF90, #334165 (Intel Xeon
+	 * This behavior is documented in item BDX90, #334165 (Intel Xeon
 	 * Processor E7-8800/4800 v4 Product Family).
 	 */
 	if (c->x86_vfm == INTEL_BROADWELL_X &&
 	    c->x86_stepping == 0x01 &&
 	    llc_size_per_core > 2621440 &&
 	    c->microcode < 0x0b000021) {
-		pr_err_once("Erratum BDF90: late loading with revision < 0x0b000021 (0x%x) disabled.\n", c->microcode);
+		pr_err_once("Erratum BDX90: late loading with revision < 0x0b000021 (0x%x) disabled.\n", c->microcode);
 		pr_err_once("Please consider either early loading through initrd/built-in or a potential BIOS update.\n");
 		return true;
 	}
-- 
2.34.1

Re: [PATCH 1/1] arch/x86/microcode/intel: Remove unnecessary cache writeback and invalidation

[Dave Hansen]

On 7/1/24 14:20, Chang S. Bae wrote:
...> Remove native_wbinvd() and update the erratum name to align with the
> latest errata documentation.

I'm all for simplifying this code and also for removing any WBINVDs that
we possibly can.  But it also makes me a wee bit nervous that this could
have been hiding any _new_ issues (like the Broadwell one) had they
crept in.

I'm tentatively in favor of this, but it's definitely the kind of thing
we want to apply early and get maximum testing on.

I'd also appreciate an ack from Ashok on this one.

Re: [PATCH 1/1] arch/x86/microcode/intel: Remove unnecessary cache writeback and invalidation

[Chang S. Bae]

Sorry, I just noticed a silly typo in the subject. The prefix should be 
'x86/microcode/intel:'.

Thanks,
Chang

Re: [PATCH 1/1] arch/x86/microcode/intel: Remove unnecessary cache writeback and invalidation

[Ashok Raj]

On Mon, Jul 01, 2024 at 03:56:20PM -0700, Dave Hansen wrote:
> On 7/1/24 14:20, Chang S. Bae wrote:
> ...> Remove native_wbinvd() and update the erratum name to align with the
> > latest errata documentation.
> 
> I'm all for simplifying this code and also for removing any WBINVDs that
> we possibly can.  But it also makes me a wee bit nervous that this could
> have been hiding any _new_ issues (like the Broadwell one) had they
> crept in.
> 
> I'm tentatively in favor of this, but it's definitely the kind of thing
> we want to apply early and get maximum testing on.
> 
> I'd also appreciate an ack from Ashok on this one.

Thanks Dave. At the time when wbinvd() was added, there was no
confirmation that we didn't have any other products that slipped. This was
also during the meltdown timeframe, so big hammer was choosen for safety.

We attempted to remove them during the minrev rework.

https://lore.kernel.org/lkml/20230130213955.6046-9-ashok.raj@intel.com/

Agree that we must get wider testing. Only caveat is that you should find a
newer microcode to apply, which might be difficult for all products. Unless
there is a debug option to reload force the same rev in case you don't have
a newer ucode to test. Its good to get this in to reduce the big hammer
effect.

Acked by: Ashok Raj <ashok.raj@intel.com>

-- 
Cheers,
Ashok

Re: [PATCH 1/1] arch/x86/microcode/intel: Remove unnecessary cache writeback and invalidation

[Dave Hansen]

On 7/3/24 13:50, Ashok Raj wrote:
> Agree that we must get wider testing. Only caveat is that you should find a
> newer microcode to apply, which might be difficult for all products. Unless
> there is a debug option to reload force the same rev in case you don't have
> a newer ucode to test. Its good to get this in to reduce the big hammer
> effect.

Why is it hard to find a newer microcode to apply?  Just because the
BIOS-provided one is more likely to be the last update the other the CPU?

Re: [PATCH 1/1] arch/x86/microcode/intel: Remove unnecessary cache writeback and invalidation

[Ashok Raj]

On Wed, Jul 03, 2024 at 01:55:19PM -0700, Dave Hansen wrote:
> On 7/3/24 13:50, Ashok Raj wrote:
> > Agree that we must get wider testing. Only caveat is that you should find a
> > newer microcode to apply, which might be difficult for all products. Unless
> > there is a debug option to reload force the same rev in case you don't have
> > a newer ucode to test. Its good to get this in to reduce the big hammer
> > effect.
> 
> Why is it hard to find a newer microcode to apply?  Just because the
> BIOS-provided one is more likely to be the last update the other the CPU?

Yes, sometimes that, or an earlier update has already been applied via
early loading (which seems most of the case). Someone needs to do some
extra work to remove it from initramfs copy, reboot and redo the test.

Some manual assemply required. It would have been nice to just avoid
doing early loading, but that cmdline (dis_ucode_ldr) disables both early
and late-loading. I had some patches a while back to separate the two to 
help with this, but I never got a chance to send that out. 

dis_ucode_ldr: Disables both early and late-loading.

-- 
Cheers,
Ashok

Re: [PATCH 1/1] arch/x86/microcode/intel: Remove unnecessary cache writeback and invalidation

[Dave Hansen]

On 7/3/24 14:03, Ashok Raj wrote:
> On Wed, Jul 03, 2024 at 01:55:19PM -0700, Dave Hansen wrote:
>> On 7/3/24 13:50, Ashok Raj wrote:
>>> Agree that we must get wider testing. Only caveat is that you should find a
>>> newer microcode to apply, which might be difficult for all products. Unless
>>> there is a debug option to reload force the same rev in case you don't have
>>> a newer ucode to test. Its good to get this in to reduce the big hammer
>>> effect.
>>
>> Why is it hard to find a newer microcode to apply?  Just because the
>> BIOS-provided one is more likely to be the last update the other the CPU?
> 
> Yes, sometimes that, or an earlier update has already been applied via
> early loading (which seems most of the case). Someone needs to do some
> extra work to remove it from initramfs copy, reboot and redo the test.

This patch touches __apply_microcode(), which looks like it's used in
both early and late loading.

But it sounds like you're thinking that the WBINVD is (or was) primarily
useful during late loading.  Why is that?

Or am I totally misreading the code again? :)

Re: [PATCH 1/1] arch/x86/microcode/intel: Remove unnecessary cache writeback and invalidation

[Ashok Raj]

On Wed, Jul 03, 2024 at 02:11:34PM -0700, Dave Hansen wrote:
> On 7/3/24 14:03, Ashok Raj wrote:
> > On Wed, Jul 03, 2024 at 01:55:19PM -0700, Dave Hansen wrote:
> >> On 7/3/24 13:50, Ashok Raj wrote:
> >>> Agree that we must get wider testing. Only caveat is that you should find a
> >>> newer microcode to apply, which might be difficult for all products. Unless
> >>> there is a debug option to reload force the same rev in case you don't have
> >>> a newer ucode to test. Its good to get this in to reduce the big hammer
> >>> effect.
> >>
> >> Why is it hard to find a newer microcode to apply?  Just because the
> >> BIOS-provided one is more likely to be the last update the other the CPU?
> > 
> > Yes, sometimes that, or an earlier update has already been applied via
> > early loading (which seems most of the case). Someone needs to do some
> > extra work to remove it from initramfs copy, reboot and redo the test.
> 
> This patch touches __apply_microcode(), which looks like it's used in
> both early and late loading.

In the old days we had a separate function for early and separate for late
loading. tglx consolidated them, so they all look pretty now. 

When wbinvd() was introduced I do believe we added to both early and late.
Although I don't recall entirely.

> 
> But it sounds like you're thinking that the WBINVD is (or was) primarily
> useful during late loading.  Why is that?
> 
> Or am I totally misreading the code again? :)

-- 
Cheers,
Ashok

Re: [PATCH 1/1] arch/x86/microcode/intel: Remove unnecessary cache writeback and invalidation

[Chang S. Bae]

On 7/3/2024 2:33 PM, Ashok Raj wrote:
> 
> When wbinvd() was introduced I do believe we added to both early and late.
> Although I don't recall entirely.

Yes, it was added for both:

$ git show --source 91df9fdf5149
...
diff --git a/arch/x86/kernel/cpu/microcode/intel.c 
b/arch/x86/kernel/cpu/microcode/intel.c
index 87bd6dc94081..e2864bc2d575 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -600,6 +600,12 @@ static int apply_microcode_early(struct 
ucode_cpu_info *uci, bool early)
                 return UCODE_OK;
         }

+       /*
+        * Writeback and invalidate caches before updating microcode to 
avoid
+        * internal issues depending on what the microcode is updating.
+        */
+       native_wbinvd();
+
         /* write microcode via MSR 0x79 */
         native_wrmsrl(MSR_IA32_UCODE_WRITE, (unsigned long)mc->bits);

@@ -816,6 +822,12 @@ static enum ucode_state apply_microcode_intel(int cpu)
                 return UCODE_OK;
         }

+       /*
+        * Writeback and invalidate caches before updating microcode to 
avoid
+        * internal issues depending on what the microcode is updating.
+        */
+       native_wbinvd();
+
         /* write microcode via MSR 0x79 */
         wrmsrl(MSR_IA32_UCODE_WRITE, (unsigned long)mc->bits);

Thanks,
Chang

Re: [PATCH 1/1] arch/x86/microcode/intel: Remove unnecessary cache writeback and invalidation

[Chang S. Bae]

On 7/1/2024 2:20 PM, Chang S. Bae wrote:
> 
> Additionally, the side effects of doing this have been overlooked. It
> extends the CPU rendezvous time for late loading. The cache flush takes
> about 1x to 3.5x more time than needed for updating the microcode.

To provide more context, the latency impact was found to be more adverse 
when late loading was staged using the new loading feature [1]. Its 
enabling patch set will be posted once the specification is updated, 
likely after the upcoming merge window. I will include this fix (v2) as 
part of the series.

Thanks,
Chang

[1]: https://cdrdv2.intel.com/v1/dl/getContent/782715