Re: [PATCH v2] arch/x86: Fix boot_cpu_data.microcode version output

Re: [PATCH v2] arch/x86: Fix boot_cpu_data.microcode version output

[Oleksandr Natalenko]

Hi.

> I tested this on AMD Ryzen & Intel Broadwell system and dumped the
> boot_cpu_data before and after a microcode update.  On the Intel
> system I also did a fatal MCE using mce-inject to confirm the output
> from the mce handling code.
> 
> P.
> 
> ---8<---
> 
> On systems where a runtime microcode update has occurred the microcode
> version output in a MCE log record is wrong because
> boot_cpu_data.microcode is not updated during runtime.
> 
> Update boot_cpu_data.microcode when the BSP's microcode is updated.
> 
> Fixes: fa94d0c6e0f3 ("x86/MCE: Save microcode revision in machine check 
> records")
> Suggested-by: Borislav Petkov <bp@alien8.com>
> Signed-off-by: Prarit Bhargava <prarit@redhat.com>
> Cc: stable@vger.kernel.org
> Cc: sironi@amazon.de
> Cc: tony.luck@intel.com
> ---
> Changes in v2: Use mc_amd->hdr.patch_id on AMD
> 
>  arch/x86/kernel/cpu/microcode/amd.c   | 4 ++++
>  arch/x86/kernel/cpu/microcode/intel.c | 4 ++++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/microcode/amd.c 
> b/arch/x86/kernel/cpu/microcode/amd.c
> index 0624957aa068..63b072377ba4 100644
> --- a/arch/x86/kernel/cpu/microcode/amd.c
> +++ b/arch/x86/kernel/cpu/microcode/amd.c
> @@ -537,6 +537,10 @@ static enum ucode_state apply_microcode_amd(int 
> cpu)
>  	uci->cpu_sig.rev = mc_amd->hdr.patch_id;
>  	c->microcode = mc_amd->hdr.patch_id;
> 
> +	/* Update boot_cpu_data's revision too, if we're on the BSP: */
> +	if (c->cpu_index == boot_cpu_data.cpu_index)
> +		boot_cpu_data.microcode =  mc_amd->hdr.patch_id;
> +
>  	return UCODE_UPDATED;
>  }
> 
> diff --git a/arch/x86/kernel/cpu/microcode/intel.c 
> b/arch/x86/kernel/cpu/microcode/intel.c
> index 97ccf4c3b45b..256d336cbc04 100644
> --- a/arch/x86/kernel/cpu/microcode/intel.c
> +++ b/arch/x86/kernel/cpu/microcode/intel.c
> @@ -851,6 +851,10 @@ static enum ucode_state apply_microcode_intel(int 
> cpu)
>  	uci->cpu_sig.rev = rev;
>  	c->microcode = rev;
> 
> +	/* Update boot_cpu_data's revision too, if we're on the BSP: */
> +	if (c->cpu_index == boot_cpu_data.cpu_index)
> +		boot_cpu_data.microcode = rev;
> +
>  	return UCODE_UPDATED;
>  }
> 
> --
> 2.17.0

After this patch, do we preserve an original microcode version 
somewhere? If no, why? Sometimes it is useful while debugging another 
crash because of faulty microcode.

Thanks.

-- 
   Oleksandr Natalenko (post-factum)

Re: [PATCH v2] arch/x86: Fix boot_cpu_data.microcode version output

[Prarit Bhargava]

On 08/01/2018 02:38 AM, Oleksandr Natalenko wrote:
> Hi.
> 
>> I tested this on AMD Ryzen & Intel Broadwell system and dumped the
>> boot_cpu_data before and after a microcode update.  On the Intel
>> system I also did a fatal MCE using mce-inject to confirm the output
>> from the mce handling code.
>>
>> P.
>>
>> ---8<---
>>
>> On systems where a runtime microcode update has occurred the microcode
>> version output in a MCE log record is wrong because
>> boot_cpu_data.microcode is not updated during runtime.
>>
>> Update boot_cpu_data.microcode when the BSP's microcode is updated.
>>
>> Fixes: fa94d0c6e0f3 ("x86/MCE: Save microcode revision in machine check records")
>> Suggested-by: Borislav Petkov <bp@alien8.com>
>> Signed-off-by: Prarit Bhargava <prarit@redhat.com>
>> Cc: stable@vger.kernel.org
>> Cc: sironi@amazon.de
>> Cc: tony.luck@intel.com
>> ---
>> Changes in v2: Use mc_amd->hdr.patch_id on AMD
>>
>>  arch/x86/kernel/cpu/microcode/amd.c   | 4 ++++
>>  arch/x86/kernel/cpu/microcode/intel.c | 4 ++++
>>  2 files changed, 8 insertions(+)
>>
>> diff --git a/arch/x86/kernel/cpu/microcode/amd.c
>> b/arch/x86/kernel/cpu/microcode/amd.c
>> index 0624957aa068..63b072377ba4 100644
>> --- a/arch/x86/kernel/cpu/microcode/amd.c
>> +++ b/arch/x86/kernel/cpu/microcode/amd.c
>> @@ -537,6 +537,10 @@ static enum ucode_state apply_microcode_amd(int cpu)
>>      uci->cpu_sig.rev = mc_amd->hdr.patch_id;
>>      c->microcode = mc_amd->hdr.patch_id;
>>
>> +    /* Update boot_cpu_data's revision too, if we're on the BSP: */
>> +    if (c->cpu_index == boot_cpu_data.cpu_index)
>> +        boot_cpu_data.microcode =  mc_amd->hdr.patch_id;
>> +
>>      return UCODE_UPDATED;
>>  }
>>
>> diff --git a/arch/x86/kernel/cpu/microcode/intel.c
>> b/arch/x86/kernel/cpu/microcode/intel.c
>> index 97ccf4c3b45b..256d336cbc04 100644
>> --- a/arch/x86/kernel/cpu/microcode/intel.c
>> +++ b/arch/x86/kernel/cpu/microcode/intel.c
>> @@ -851,6 +851,10 @@ static enum ucode_state apply_microcode_intel(int cpu)
>>      uci->cpu_sig.rev = rev;
>>      c->microcode = rev;
>>
>> +    /* Update boot_cpu_data's revision too, if we're on the BSP: */
>> +    if (c->cpu_index == boot_cpu_data.cpu_index)
>> +        boot_cpu_data.microcode = rev;
>> +
>>      return UCODE_UPDATED;
>>  }
>>
>> -- 
>> 2.17.0
> 
> After this patch, do we preserve an original microcode version somewhere? If no,
> why? Sometimes it is useful while debugging another crash because of faulty
> microcode.
> 

Interesting, and thanks for bringing this up.  Oleksandr, under what
circumstances would you want to know what the old version was.  AFAICS it is no
longer running and should not have an impact on the system?

P.

> Thanks.
> 

Re: [PATCH v2] arch/x86: Fix boot_cpu_data.microcode version output

[Oleksandr Natalenko]

Hi.

On 01.08.2018 13:38, Prarit Bhargava wrote:
>> After this patch, do we preserve an original microcode version 
>> somewhere? If no,
>> why? Sometimes it is useful while debugging another crash because of 
>> faulty
>> microcode.
>> 
> Interesting, and thanks for bringing this up.  Oleksandr, under what
> circumstances would you want to know what the old version was.  AFAICS 
> it is no
> longer running and should not have an impact on the system?

Once the kernel log does not contain a printout regarding updated 
microcode anymore (because the log buffer is limited in size and can be 
overwritten) and once you have a vmcore, it is handy to use 
boot_cpu_data to compare the microcode version with the per-CPU value to 
find out that is was updated at all. Or, maybe, that can be inspected in 
another way now?

-- 
   Oleksandr Natalenko (post-factum)

Re: [PATCH v2] arch/x86: Fix boot_cpu_data.microcode version output

[Borislav Petkov]

(drop stable@ from CC)

On Wed, Aug 01, 2018 at 04:16:42PM +0200, Oleksandr Natalenko wrote:
> Once the kernel log does not contain a printout regarding updated microcode
> anymore (because the log buffer is limited in size and can be overwritten)

There's no reliable way to get the old microcode revision which was
overwritten during the upgrade. If dmesg gets overwritten you lose, like
in all the other gazillion cases where you lose information due to that.
Sorry.

This becomes an even bigger problem if you have a long-running system
which upgrades microcode a couple of times before being rebooted again.
In that case, your only log which contains the microcode revisions being
upgraded is dmesg.

> once you have a vmcore, it is handy to use boot_cpu_data to compare
> the microcode version with the per-CPU value to find out that is was
> updated at all.

boot_cpu_data.microcode was never meant to contain the *previous*
microcode revision AFAICT - it just didn't get updated, which we're
fixing now.

> Or, maybe, that can be inspected in another way now?

Dunno, does kdump collect /proc/cpuinfo? If so:

$ grep microcode /proc/cpuinfo

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

RE: [PATCH v2] arch/x86: Fix boot_cpu_data.microcode version output

[Luck, Tony]

> There's no reliable way to get the old microcode revision which was
> overwritten during the upgrade. If dmesg gets overwritten you lose, like
> in all the other gazillion cases where you lose information due to that.

The primary requirement here is that we report the version of the microcode
in use at the time of a crash. Keeping history of all updates seems to me to
beyond the scope of the kernel's responsibilities.

It's not like these updates appear out of the ether. You have to go out and
grab a new package and install it.  User land can keep track of this much
more easily than the kernel.

-Tony

Re: [PATCH v2] arch/x86: Fix boot_cpu_data.microcode version output

[Oleksandr Natalenko]

Hi.

On 01.08.2018 17:59, Luck, Tony wrote:
>> There's no reliable way to get the old microcode revision which was
>> overwritten during the upgrade. If dmesg gets overwritten you lose, 
>> like
>> in all the other gazillion cases where you lose information due to 
>> that.
> 
> The primary requirement here is that we report the version of the 
> microcode
> in use at the time of a crash. Keeping history of all updates seems to 
> me to
> beyond the scope of the kernel's responsibilities.
> 
> It's not like these updates appear out of the ether. You have to go out 
> and
> grab a new package and install it.  User land can keep track of this 
> much
> more easily than the kernel.

I don't mind doing the right thing at all. It is just to inform you that 
it was found to be useful.

Also, [1].

Thanks ☺.

[1] https://xkcd.com/1172/

-- 
   Oleksandr Natalenko (post-factum)

Re: [PATCH v2] arch/x86: Fix boot_cpu_data.microcode version output

[Boris Petkov]

On August 1, 2018 7:01:50 PM GMT+03:00, Oleksandr Natalenko <oleksandr@natalenko.name> wrote:
>I don't mind doing the right thing at all. It is just to inform you
>that 
>it was found to be useful.

I don't think it would've worked if you did a second microcode upgrade on the system.
 
-- 
Sent from a small device: formatting sux and brevity is inevitable. 

Re: [PATCH v2] arch/x86: Fix boot_cpu_data.microcode version output

[Prarit Bhargava]

On 08/01/2018 11:43 AM, Borislav Petkov wrote:
> (dropping stable@)
> 
> On Tue, Jul 31, 2018 at 07:27:39AM -0400, Prarit Bhargava wrote:
>> I tested this on AMD Ryzen & Intel Broadwell system and dumped the
>> boot_cpu_data before and after a microcode update.  On the Intel
>> system I also did a fatal MCE using mce-inject to confirm the output
>> from the mce handling code.
> 
> Ok, sending the two patches I've massaged into submission as a reply to
> this message - please run them on your boxes. You can also inject MCEs
> on the AMD box to verify - simply enable CONFIG_X86_MCE_INJECT, go to
> debugfs and do, for example:
> 
> cd /sys/kernel/debug/mce-inject/
> echo 10 > /sys/devices/system/machinecheck/machinecheck0/check_interval
> echo 0x9c7d410092080813 > status; echo 0x000000006d3d483b > addr; echo 2 > cpu; echo 4 > bank; echo hw > flags
> echo 0xbc00200000010015 > status; echo 0x00000000942e4d98 > addr; echo 6 > cpu; echo 2 > bank; echo hw > flags
> echo 0xfe000010000b0c0f > status; echo 0xe1101add1e550012 > addr; echo 0xcafebeef > misc; echo 2 > cpu; echo hw > flags; echo 4 > bank
> 

Borislav, I'm not sure if you saw his comments but Oleksandr, cc'd, has an
objection to overwriting the stored microcode version in boot_cpu_data.  He has
found that information useful in debugging microcode issues.

P.

> Thanks!
> 

Re: [PATCH v2] arch/x86: Fix boot_cpu_data.microcode version output

[Borislav Petkov]

(dropping stable@)

On Tue, Jul 31, 2018 at 07:27:39AM -0400, Prarit Bhargava wrote:
> I tested this on AMD Ryzen & Intel Broadwell system and dumped the
> boot_cpu_data before and after a microcode update.  On the Intel
> system I also did a fatal MCE using mce-inject to confirm the output
> from the mce handling code.

Ok, sending the two patches I've massaged into submission as a reply to
this message - please run them on your boxes. You can also inject MCEs
on the AMD box to verify - simply enable CONFIG_X86_MCE_INJECT, go to
debugfs and do, for example:

cd /sys/kernel/debug/mce-inject/
echo 10 > /sys/devices/system/machinecheck/machinecheck0/check_interval
echo 0x9c7d410092080813 > status; echo 0x000000006d3d483b > addr; echo 2 > cpu; echo 4 > bank; echo hw > flags
echo 0xbc00200000010015 > status; echo 0x00000000942e4d98 > addr; echo 6 > cpu; echo 2 > bank; echo hw > flags
echo 0xfe000010000b0c0f > status; echo 0xe1101add1e550012 > addr; echo 0xcafebeef > misc; echo 2 > cpu; echo hw > flags; echo 4 > bank

Thanks!

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH v2] arch/x86: Fix boot_cpu_data.microcode version output

[Sironi, Filippo]

> On 31. Jul 2018, at 15:00, Borislav Petkov <bp@alien8.de> wrote:
> 
> On Tue, Jul 31, 2018 at 11:46:09AM +0000, Sironi, Filippo wrote:
>> There may be a chance of skipping this code, I think.
>> 
>> If the microcode is loaded on the hyperthread sibling of the boot cpu
>> before being loaded on the boot cpu, the boot cpu will exit earlier
>> from apply_microcode_intel() - in if (rev >= mc->hdr.rev) { ... }.
>> 
>> (This seems to be possible in apply_microcode_amd() as well.)
>> 
>> In my tree with the aforementioned change - Intel only - I also had
>> the following patch:
> 
> Yap, good catch.
> 
> -- 
> Regards/Gruss,
>    Boris.
> 
> ECO tip #101: Trim your mails when you reply.
> --
> 

Boris, Prarit,

I've sent the patch the I had in my tree and extended it to cover
apply_microcode_amd(). I don't have an AMD host available for testing,
can one of you give that a spin?

Thanks,
Filippo


Amazon Development Center Germany GmbH
Berlin - Dresden - Aachen
main office: Krausenstr. 38, 10117 Berlin
Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
Ust-ID: DE289237879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B

Re: [PATCH v2] arch/x86: Fix boot_cpu_data.microcode version output

[Borislav Petkov]

On Tue, Jul 31, 2018 at 11:46:09AM +0000, Sironi, Filippo wrote:
> There may be a chance of skipping this code, I think.
> 
> If the microcode is loaded on the hyperthread sibling of the boot cpu
> before being loaded on the boot cpu, the boot cpu will exit earlier
> from apply_microcode_intel() - in if (rev >= mc->hdr.rev) { ... }.
> 
> (This seems to be possible in apply_microcode_amd() as well.)
> 
> In my tree with the aforementioned change - Intel only - I also had
> the following patch:

Yap, good catch.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [PATCH v2] arch/x86: Fix boot_cpu_data.microcode version output

[Sironi, Filippo]

> On 31. Jul 2018, at 13:27, Prarit Bhargava <prarit@redhat.com> wrote:
> 
> I tested this on AMD Ryzen & Intel Broadwell system and dumped the
> boot_cpu_data before and after a microcode update.  On the Intel
> system I also did a fatal MCE using mce-inject to confirm the output
> from the mce handling code.
> 
> P.
> 
> ---8<---
> 
> On systems where a runtime microcode update has occurred the microcode
> version output in a MCE log record is wrong because
> boot_cpu_data.microcode is not updated during runtime.
> 
> Update boot_cpu_data.microcode when the BSP's microcode is updated.
> 
> Fixes: fa94d0c6e0f3 ("x86/MCE: Save microcode revision in machine check records")
> Suggested-by: Borislav Petkov <bp@alien8.com>
> Signed-off-by: Prarit Bhargava <prarit@redhat.com>
> Cc: stable@vger.kernel.org
> Cc: sironi@amazon.de
> Cc: tony.luck@intel.com
> ---
> Changes in v2: Use mc_amd->hdr.patch_id on AMD
> 
> arch/x86/kernel/cpu/microcode/amd.c   | 4 ++++
> arch/x86/kernel/cpu/microcode/intel.c | 4 ++++
> 2 files changed, 8 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
> index 0624957aa068..63b072377ba4 100644
> --- a/arch/x86/kernel/cpu/microcode/amd.c
> +++ b/arch/x86/kernel/cpu/microcode/amd.c
> @@ -537,6 +537,10 @@ static enum ucode_state apply_microcode_amd(int cpu)
> 	uci->cpu_sig.rev = mc_amd->hdr.patch_id;
> 	c->microcode = mc_amd->hdr.patch_id;
> 
> +	/* Update boot_cpu_data's revision too, if we're on the BSP: */
> +	if (c->cpu_index == boot_cpu_data.cpu_index)
> +		boot_cpu_data.microcode =  mc_amd->hdr.patch_id;
> +
> 	return UCODE_UPDATED;
> }
> 
> diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
> index 97ccf4c3b45b..256d336cbc04 100644
> --- a/arch/x86/kernel/cpu/microcode/intel.c
> +++ b/arch/x86/kernel/cpu/microcode/intel.c
> @@ -851,6 +851,10 @@ static enum ucode_state apply_microcode_intel(int cpu)
> 	uci->cpu_sig.rev = rev;
> 	c->microcode = rev;
> 
> +	/* Update boot_cpu_data's revision too, if we're on the BSP: */
> +	if (c->cpu_index == boot_cpu_data.cpu_index)
> +		boot_cpu_data.microcode = rev;
> +
> 	return UCODE_UPDATED;
> }

There may be a chance of skipping this code, I think.

If the microcode is loaded on the hyperthread sibling of the boot cpu
before being loaded on the boot cpu, the boot cpu will exit earlier
from apply_microcode_intel() - in if (rev >= mc->hdr.rev) { ... }.

(This seems to be possible in apply_microcode_amd() as well.)

In my tree with the aforementioned change - Intel only - I also had
the following patch:

diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 97ccf4c3b45b..4bc869e829eb 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -797,6 +797,7 @@ static enum ucode_state apply_microcode_intel(int cpu)
        struct microcode_intel *mc;
        static int prev_rev;
        u32 rev;
+       enum ucode_state ret;
 
        /* We should bind the task to the CPU */
        if (WARN_ON(raw_smp_processor_id() != cpu))
@@ -817,9 +818,8 @@ static enum ucode_state apply_microcode_intel(int cpu)
         */
        rev = intel_get_microcode_revision();
        if (rev >= mc->hdr.rev) {
-               uci->cpu_sig.rev = rev;
-               c->microcode = rev;
-               return UCODE_OK;
+               ret = UCODE_OK;
+               goto out;
        }
 
        /*
@@ -848,10 +848,12 @@ static enum ucode_state apply_microcode_intel(int cpu)
                prev_rev = rev;
        }
 
+       ret = UCODE_UPDATED;
+out:
        uci->cpu_sig.rev = rev;
        c->microcode = rev;
 
-       return UCODE_UPDATED;
+       return ret;
 }
 
 static enum ucode_state generic_load_microcode(int cpu, void *data, size_t size,

which prevents the issue.

> -- 
> 2.17.0
> 
> 

Amazon Development Center Germany GmbH
Berlin - Dresden - Aachen
main office: Krausenstr. 38, 10117 Berlin
Geschaeftsfuehrer: Dr. Ralf Herbrich, Christian Schlaeger
Ust-ID: DE289237879
Eingetragen am Amtsgericht Charlottenburg HRB 149173 B

[PATCH v2] arch/x86: Fix boot_cpu_data.microcode version output

[Prarit Bhargava]

I tested this on AMD Ryzen & Intel Broadwell system and dumped the
boot_cpu_data before and after a microcode update.  On the Intel
system I also did a fatal MCE using mce-inject to confirm the output
from the mce handling code.

P.

---8<---

On systems where a runtime microcode update has occurred the microcode
version output in a MCE log record is wrong because
boot_cpu_data.microcode is not updated during runtime.

Update boot_cpu_data.microcode when the BSP's microcode is updated.

Fixes: fa94d0c6e0f3 ("x86/MCE: Save microcode revision in machine check records")
Suggested-by: Borislav Petkov <bp@alien8.com>
Signed-off-by: Prarit Bhargava <prarit@redhat.com>
Cc: stable@vger.kernel.org
Cc: sironi@amazon.de
Cc: tony.luck@intel.com
---
Changes in v2: Use mc_amd->hdr.patch_id on AMD

 arch/x86/kernel/cpu/microcode/amd.c   | 4 ++++
 arch/x86/kernel/cpu/microcode/intel.c | 4 ++++
 2 files changed, 8 insertions(+)

diff --git a/arch/x86/kernel/cpu/microcode/amd.c b/arch/x86/kernel/cpu/microcode/amd.c
index 0624957aa068..63b072377ba4 100644
--- a/arch/x86/kernel/cpu/microcode/amd.c
+++ b/arch/x86/kernel/cpu/microcode/amd.c
@@ -537,6 +537,10 @@ static enum ucode_state apply_microcode_amd(int cpu)
 	uci->cpu_sig.rev = mc_amd->hdr.patch_id;
 	c->microcode = mc_amd->hdr.patch_id;
 
+	/* Update boot_cpu_data's revision too, if we're on the BSP: */
+	if (c->cpu_index == boot_cpu_data.cpu_index)
+		boot_cpu_data.microcode =  mc_amd->hdr.patch_id;
+
 	return UCODE_UPDATED;
 }
 
diff --git a/arch/x86/kernel/cpu/microcode/intel.c b/arch/x86/kernel/cpu/microcode/intel.c
index 97ccf4c3b45b..256d336cbc04 100644
--- a/arch/x86/kernel/cpu/microcode/intel.c
+++ b/arch/x86/kernel/cpu/microcode/intel.c
@@ -851,6 +851,10 @@ static enum ucode_state apply_microcode_intel(int cpu)
 	uci->cpu_sig.rev = rev;
 	c->microcode = rev;
 
+	/* Update boot_cpu_data's revision too, if we're on the BSP: */
+	if (c->cpu_index == boot_cpu_data.cpu_index)
+		boot_cpu_data.microcode = rev;
+
 	return UCODE_UPDATED;
 }
 
-- 
2.17.0