2013-09-26 21:55:09

by Suthikulpanit, Suravee

[permalink] [raw]
Subject: [PATCH] x86, microcode, AMD: Fix patch level reporting for family15h

From: Suravee Suthikulpanit <[email protected]>

On AMD family15h, applying microcode patch on the a core (core0)
would also affect the other core (core1) in the same compute unit.
The driver would skip applying the patch on core1, but it still
need to update kernel structures to reflect the proper patch level.

The current logic is not updating the struct ucode_cpu_info.cpu_sig.rev
of the skipped core. This causes the /sys/devices/system/cpu/cpu1/microcode/version
to report incorrect patch level as shown below:

[ 10.708841] microcode: CPU0: new patch_level=0x0600063d
[ 10.714256] microcode: CPU1: patch_level=0x06000626
[ 10.719345] microcode: CPU2: patch_level=0x06000626
[ 10.748095] microcode: CPU2: new patch_level=0x0600063d
[ 10.753365] microcode: CPU3: patch_level=0x06000626
[ 10.758264] microcode: CPU4: patch_level=0x06000626
[ 10.786999] microcode: CPU4: new patch_level=0x0600063d

Signed-off-by: Suravee Suthikulpanit <[email protected]>
---
arch/x86/kernel/microcode_amd.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/arch/x86/kernel/microcode_amd.c b/arch/x86/kernel/microcode_amd.c
index 7123b5d..af99f71 100644
--- a/arch/x86/kernel/microcode_amd.c
+++ b/arch/x86/kernel/microcode_amd.c
@@ -216,6 +216,7 @@ int apply_microcode_amd(int cpu)
/* need to apply patch? */
if (rev >= mc_amd->hdr.patch_id) {
c->microcode = rev;
+ uci->cpu_sig.rev = rev;
return 0;
}

--
1.8.1.2


2013-09-26 22:13:39

by Borislav Petkov

[permalink] [raw]
Subject: Re: [PATCH] x86, microcode, AMD: Fix patch level reporting for family15h

On Thu, Sep 26, 2013 at 04:54:32PM -0500, [email protected] wrote:
> From: Suravee Suthikulpanit <[email protected]>
>
> On AMD family15h, applying microcode patch on the a core (core0)
> would also affect the other core (core1) in the same compute unit.
> The driver would skip applying the patch on core1, but it still
> need to update kernel structures to reflect the proper patch level.
>
> The current logic is not updating the struct ucode_cpu_info.cpu_sig.rev
> of the skipped core. This causes the /sys/devices/system/cpu/cpu1/microcode/version
> to report incorrect patch level as shown below:
>
> [ 10.708841] microcode: CPU0: new patch_level=0x0600063d
> [ 10.714256] microcode: CPU1: patch_level=0x06000626
> [ 10.719345] microcode: CPU2: patch_level=0x06000626
> [ 10.748095] microcode: CPU2: new patch_level=0x0600063d
> [ 10.753365] microcode: CPU3: patch_level=0x06000626
> [ 10.758264] microcode: CPU4: patch_level=0x06000626
> [ 10.786999] microcode: CPU4: new patch_level=0x0600063d

Actually, this is collect_cpu_info_amd()'s normal operation and shows
that there's no need to apply a microcode patch on the odd core since
the even core's ucode has been updated.

Actually you need something like that:

$ grep . cpu?/microcode/version
cpu0/microcode/version:0x6000822
cpu1/microcode/version:0x600081f
cpu2/microcode/version:0x6000822
cpu3/microcode/version:0x600081f
cpu4/microcode/version:0x6000822
cpu5/microcode/version:0x600081f
cpu6/microcode/version:0x6000822
cpu7/microcode/version:0x600081f

which shows the bug. Other than that, the patch is correct so please fix
the commit message and add [email protected] to CC on the next submission
so that it gets picked by x86 people.

Thanks.

--
Regards/Gruss,
Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

2013-09-26 23:06:30

by Andreas Herrmann

[permalink] [raw]
Subject: Re: [PATCH] x86, microcode, AMD: Fix patch level reporting for family15h

On Fri, Sep 27, 2013 at 12:13:22AM +0200, Borislav Petkov wrote:
> On Thu, Sep 26, 2013 at 04:54:32PM -0500, [email protected] wrote:
> > From: Suravee Suthikulpanit <[email protected]>
> >
> > On AMD family15h, applying microcode patch on the a core (core0)
> > would also affect the other core (core1) in the same compute unit.
> > The driver would skip applying the patch on core1, but it still
> > need to update kernel structures to reflect the proper patch level.
> >
> > The current logic is not updating the struct ucode_cpu_info.cpu_sig.rev
> > of the skipped core. This causes the /sys/devices/system/cpu/cpu1/microcode/version
> > to report incorrect patch level as shown below:
> >
> > [ 10.708841] microcode: CPU0: new patch_level=0x0600063d
> > [ 10.714256] microcode: CPU1: patch_level=0x06000626
> > [ 10.719345] microcode: CPU2: patch_level=0x06000626
> > [ 10.748095] microcode: CPU2: new patch_level=0x0600063d
> > [ 10.753365] microcode: CPU3: patch_level=0x06000626
> > [ 10.758264] microcode: CPU4: patch_level=0x06000626
> > [ 10.786999] microcode: CPU4: new patch_level=0x0600063d
>
> Actually, this is collect_cpu_info_amd()'s normal operation and shows
> that there's no need to apply a microcode patch on the odd core since
> the even core's ucode has been updated.

Hmm, I think Boris is right, above messages are just logging what
happened during µcode update. I think the patch_level in "CPU1:
patch_level=0x06000626" is based on c->microcode which is updated
shortly after this message was printed.

I assume with your patch, above message won't look different but just
the contents in /sys/devices/system/cpu/cpu1/microcode/version will
show the correct version, right?


Andreas

2013-09-26 23:18:49

by Suthikulpanit, Suravee

[permalink] [raw]
Subject: Re: [PATCH] x86, microcode, AMD: Fix patch level reporting for family15h

On 9/26/2013 6:06 PM, Andreas Herrmann wrote:
> On Fri, Sep 27, 2013 at 12:13:22AM +0200, Borislav Petkov wrote:
>> On Thu, Sep 26, 2013 at 04:54:32PM -0500, [email protected] wrote:
>>> From: Suravee Suthikulpanit <[email protected]>
>>>
>>> On AMD family15h, applying microcode patch on the a core (core0)
>>> would also affect the other core (core1) in the same compute unit.
>>> The driver would skip applying the patch on core1, but it still
>>> need to update kernel structures to reflect the proper patch level.
>>>
>>> The current logic is not updating the struct ucode_cpu_info.cpu_sig.rev
>>> of the skipped core. This causes the /sys/devices/system/cpu/cpu1/microcode/version
>>> to report incorrect patch level as shown below:
>>>
>>> [ 10.708841] microcode: CPU0: new patch_level=0x0600063d
>>> [ 10.714256] microcode: CPU1: patch_level=0x06000626
>>> [ 10.719345] microcode: CPU2: patch_level=0x06000626
>>> [ 10.748095] microcode: CPU2: new patch_level=0x0600063d
>>> [ 10.753365] microcode: CPU3: patch_level=0x06000626
>>> [ 10.758264] microcode: CPU4: patch_level=0x06000626
>>> [ 10.786999] microcode: CPU4: new patch_level=0x0600063d
>> Actually, this is collect_cpu_info_amd()'s normal operation and shows
>> that there's no need to apply a microcode patch on the odd core since
>> the even core's ucode has been updated.
> Hmm, I think Boris is right, above messages are just logging what
> happened during µcode update. I think the patch_level in "CPU1:
> patch_level=0x06000626" is based on c->microcode which is updated
> shortly after this message was printed.
>
> I assume with your patch, above message won't look different but just
> the contents in /sys/devices/system/cpu/cpu1/microcode/version will
> show the correct version, right?
>
>
> Andreas
>
Yes, the message in dmesg is still showing the same. Only the sysfs...
version is now fixed.

Suravee