[v7,07/10] microcode/intel: Writeback and invalidate caches before updating microcode

Commit Message

Chao Gao May 27, 2019, 8:31 a.m. UTC

Updating microcode is less error prone when caches have been flushed and
depending on what exactly the microcode is updating. For example, some
of the issues around certain Broadwell parts can be addressed by doing a
full cache flush.

With parallel microcode update, the cost of this patch is hardly
noticable. Although only BDX with an old microcode needs this fix, we
would like to avoid future issues in case they come by later due to
other reasons.

[linux commit: 91df9fdf51492aec9fed6b4cbd33160886740f47]
Signed-off-by: Chao Gao <[e-mail]>
Cc: Ashok Raj <[e-mail]>
---
Changes in v7:
 - explain why we do 'wbinvd' unconditionally rather than only for BDX
 in commit message

Changes in v6:
 - new
---
 xen/arch/x86/microcode_intel.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Jan Beulich June 5, 2019, 1:20 p.m. UTC | #1

>>> On 27.05.19 at 10:31, <[e-mail]> wrote:
> Updating microcode is less error prone when caches have been flushed and
> depending on what exactly the microcode is updating.

Up to the "and" I understand this sentence, but the rest doesn't really
seem to fit. Taking out the good part it seems to me you're saying
"Updating microcode is less error prone depending on what exactly the
 microcode is updating," which - to me at least - doesn't make a hole lot
of sense. Should it perhaps be

"Updating microcode, depending on what exactly the microcode is
 updating, may be less error prone when caches have been flushed."
(The same could perhaps also be achieved by replacing the "and" by
a comma.)

> For example, some
> of the issues around certain Broadwell parts can be addressed by doing a
> full cache flush.
> 
> With parallel microcode update, the cost of this patch is hardly
> noticable. Although only BDX with an old microcode needs this fix, we
> would like to avoid future issues in case they come by later due to
> other reasons.

I doubt the "hardly noticable" part, and I'm sure you're also aware of
the patch (going on top of your series) to make selecting between
serial or parallel application a runtime option. But I'm not going to
stand in the way if everyone else thinks this is the way to go; it's
just that from previous discussions I didn't get such an impression.

Jan

Patch

diff --git a/xen/arch/x86/microcode_intel.c b/xen/arch/x86/microcode_intel.c
index 650495d..bfb48ce 100644
--- a/xen/arch/x86/microcode_intel.c
+++ b/xen/arch/x86/microcode_intel.c
@@ -310,6 +310,12 @@  static int apply_microcode(const struct microcode_patch *patch)
     /* serialize access to the physical write to MSR 0x79 */
     spin_lock_irqsave(&microcode_update_lock, flags);
 
+    /*
+     * Writeback and invalidate caches before updating microcode to avoid
+     * internal issues depending on what the microcode is updating.
+     */
+    wbinvd();
+
     /* write microcode via MSR 0x79 */
     wrmsrl(MSR_IA32_UCODE_WRITE, (unsigned long)mc_intel->bits);
     wrmsrl(MSR_IA32_UCODE_REV, 0x0ULL);