[PATCH V2] x86, amd_ucode: Skip microcode updates for final levels

[PATCH V2] x86, amd_ucode: Skip microcode updates for final levels

[Aravind Gopalakrishnan]

Some of older[Fam10h] systems require that certain number of
applied microcode patch levels should not be overwritten by
the microcode loader. Otherwise, system hangs are known to occur.

The 'final_levels' of patch ids have been obtained empirically.
Refer bug https://bugzilla.suse.com/show_bug.cgi?id=913996
for details of the issue.

The short version is that people have predominantly noticed
system hang issues when trying to update microcode levels
beyond the patch IDs below.
[0x01000098, 0x0100009f, 0x010000af]

>From internal discussions, we gathered that OS/hypervisor
cannot reliably perform microcode updates beyond these levels
due to hardware issues. Therefore, we need to abort microcode
update process if we hit any of these levels.

In this patch, we check for those microcode versions and abort
if the current core has one of those final patch levels applied
by the BIOS

A linux version of the patch has already made it into tip-
http://marc.info/?l=linux-kernel&m=143703405627170

Signed-off-by: Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>
---
Changes from V1 (per Andrew)
  - use commit text from linux patch
  - include details about how 'final_levels' are obtaines in comments
    (I have also copied it into commit message. But shall remove if you
     feel it's redundant)
  - use ARRAY_SIZE() and kill NULL terminator
  - use XENLOG_INFO in place of pr_debug()
  - correct unsigned int usage

 xen/arch/x86/microcode_amd.c | 42 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
index f79b397..a9761dc 100644
--- a/xen/arch/x86/microcode_amd.c
+++ b/xen/arch/x86/microcode_amd.c
@@ -347,6 +347,40 @@ static int container_fast_forward(const void *data, size_t size_left, size_t *of
     return 0;
 }
 
+/*
+ * The 'final_levels' of patch ids have been obtained empirically.
+ * Refer bug https://bugzilla.suse.com/show_bug.cgi?id=913996
+ * for details of the issue. The short version is that people
+ * have predominantly noticed system hang issues when trying to
+ * update microcode levels beyond the patch IDs below.
+ * From internal discussions, we gathered that OS/hypervisor
+ * cannot reliably perform microcode updates beyond these levels
+ * due to hardware issues. Therefore, we need to abort microcode
+ * update process if we hit any of these levels.
+ */
+static unsigned int final_levels[] = {
+    0x01000098,
+    0x0100009f,
+    0x010000af
+};
+
+static bool_t check_final_patch_levels(int cpu)
+{
+    /*
+     * Check the current patch levels on the cpu. If they are equal to
+     * any of the 'final_levels', then we should not update the microcode
+     * patch on the cpu as system will hang otherwise.
+     */
+    struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
+    unsigned int i;
+
+    for ( i = 0; i < ARRAY_SIZE(final_levels); i++ )
+        if ( uci->cpu_sig.rev == final_levels[i] )
+            return 1;
+
+    return 0;
+}
+
 static int cpu_request_microcode(int cpu, const void *buf, size_t bufsize)
 {
     struct microcode_amd *mc_amd, *mc_old;
@@ -369,6 +403,14 @@ static int cpu_request_microcode(int cpu, const void *buf, size_t bufsize)
         goto out;
     }
 
+    if ( check_final_patch_levels(cpu) )
+    {
+        printk(XENLOG_INFO
+               "microcode: Cannot update microcode patch on the cpu as we hit a final level\n");
+        error = -EPERM;
+        goto out;
+    }
+
     mc_amd = xmalloc(struct microcode_amd);
     if ( !mc_amd )
     {
-- 
1.9.1

Re: [PATCH V2] x86, amd_ucode: Skip microcode updates for final levels

[Andrew Cooper]

On 03/08/15 17:35, Aravind Gopalakrishnan wrote:
> Some of older[Fam10h] systems require that certain number of
> applied microcode patch levels should not be overwritten by
> the microcode loader. Otherwise, system hangs are known to occur.
>
> The 'final_levels' of patch ids have been obtained empirically.
> Refer bug https://bugzilla.suse.com/show_bug.cgi?id=913996
> for details of the issue.
>
> The short version is that people have predominantly noticed
> system hang issues when trying to update microcode levels
> beyond the patch IDs below.
> [0x01000098, 0x0100009f, 0x010000af]
>
> From internal discussions, we gathered that OS/hypervisor
> cannot reliably perform microcode updates beyond these levels
> due to hardware issues. Therefore, we need to abort microcode
> update process if we hit any of these levels.
>
> In this patch, we check for those microcode versions and abort
> if the current core has one of those final patch levels applied
> by the BIOS
>
> A linux version of the patch has already made it into tip-
> http://marc.info/?l=linux-kernel&m=143703405627170
>
> Signed-off-by: Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

Re: [PATCH V2] x86, amd_ucode: Skip microcode updates for final levels

[Boris Ostrovsky]

On 08/03/2015 12:35 PM, Aravind Gopalakrishnan wrote:
> Some of older[Fam10h] systems require that certain number of
> applied microcode patch levels should not be overwritten by
> the microcode loader. Otherwise, system hangs are known to occur.
>
> The 'final_levels' of patch ids have been obtained empirically.
> Refer bug https://bugzilla.suse.com/show_bug.cgi?id=913996
> for details of the issue.
>
> The short version is that people have predominantly noticed
> system hang issues when trying to update microcode levels
> beyond the patch IDs below.
> [0x01000098, 0x0100009f, 0x010000af]
>
>  From internal discussions, we gathered that OS/hypervisor
> cannot reliably perform microcode updates beyond these levels
> due to hardware issues. Therefore, we need to abort microcode
> update process if we hit any of these levels.
>
> In this patch, we check for those microcode versions and abort
> if the current core has one of those final patch levels applied
> by the BIOS
>
> A linux version of the patch has already made it into tip-
> http://marc.info/?l=linux-kernel&m=143703405627170
>
> Signed-off-by: Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>
> ---
> Changes from V1 (per Andrew)
>    - use commit text from linux patch
>    - include details about how 'final_levels' are obtaines in comments
>      (I have also copied it into commit message. But shall remove if you
>       feel it's redundant)
>    - use ARRAY_SIZE() and kill NULL terminator
>    - use XENLOG_INFO in place of pr_debug()
>    - correct unsigned int usage
>
>   xen/arch/x86/microcode_amd.c | 42 ++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 42 insertions(+)
>
> diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
> index f79b397..a9761dc 100644
> --- a/xen/arch/x86/microcode_amd.c
> +++ b/xen/arch/x86/microcode_amd.c
> @@ -347,6 +347,40 @@ static int container_fast_forward(const void *data, size_t size_left, size_t *of
>       return 0;
>   }
>   
> +/*
> + * The 'final_levels' of patch ids have been obtained empirically.
> + * Refer bug https://bugzilla.suse.com/show_bug.cgi?id=913996
> + * for details of the issue. The short version is that people
> + * have predominantly noticed system hang issues when trying to
> + * update microcode levels beyond the patch IDs below.
> + * From internal discussions, we gathered that OS/hypervisor
> + * cannot reliably perform microcode updates beyond these levels
> + * due to hardware issues. Therefore, we need to abort microcode
> + * update process if we hit any of these levels.
> + */
> +static unsigned int final_levels[] = {
> +    0x01000098,
> +    0x0100009f,
> +    0x010000af
> +};
> +
> +static bool_t check_final_patch_levels(int cpu)
> +{
> +    /*
> +     * Check the current patch levels on the cpu. If they are equal to
> +     * any of the 'final_levels', then we should not update the microcode
> +     * patch on the cpu as system will hang otherwise.
> +     */
> +    struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
> +    unsigned int i;
> +
> +    for ( i = 0; i < ARRAY_SIZE(final_levels); i++ )
> +        if ( uci->cpu_sig.rev == final_levels[i] )
> +            return 1;

(I should have asked this when you posted v1, sorry)

Are these final_levels[] fam10h-specific? Are we guaranteed that other 
families won't have them? I think family check would be good here. Or at 
least a comment stating that these levels can only be observed on family 
10h (the only mention of family right now is in the commit message)


-borsi


> +
> +    return 0;
> +}
> +
>   static int cpu_request_microcode(int cpu, const void *buf, size_t bufsize)
>   {
>       struct microcode_amd *mc_amd, *mc_old;
> @@ -369,6 +403,14 @@ static int cpu_request_microcode(int cpu, const void *buf, size_t bufsize)
>           goto out;
>       }
>   
> +    if ( check_final_patch_levels(cpu) )
> +    {
> +        printk(XENLOG_INFO
> +               "microcode: Cannot update microcode patch on the cpu as we hit a final level\n");
> +        error = -EPERM;
> +        goto out;
> +    }
> +
>       mc_amd = xmalloc(struct microcode_amd);
>       if ( !mc_amd )
>       {

Re: [PATCH V2] x86, amd_ucode: Skip microcode updates for final levels

[Aravind Gopalakrishnan]

On 8/3/2015 12:22 PM, Boris Ostrovsky wrote:
> On 08/03/2015 12:35 PM, Aravind Gopalakrishnan wrote:
>> Some of older[Fam10h] systems require that certain number of
>> applied microcode patch levels should not be overwritten by
>> the microcode loader. Otherwise, system hangs are known to occur.
>>
>> The 'final_levels' of patch ids have been obtained empirically.
>> Refer bug https://bugzilla.suse.com/show_bug.cgi?id=913996
>> for details of the issue.
>>
>> The short version is that people have predominantly noticed
>> system hang issues when trying to update microcode levels
>> beyond the patch IDs below.
>> [0x01000098, 0x0100009f, 0x010000af]
>>
>>
>>
>>   +/*
>> + * The 'final_levels' of patch ids have been obtained empirically.
>> + * Refer bug https://bugzilla.suse.com/show_bug.cgi?id=913996
>> + * for details of the issue. The short version is that people
>> + * have predominantly noticed system hang issues when trying to
>> + * update microcode levels beyond the patch IDs below.
>> + * From internal discussions, we gathered that OS/hypervisor
>> + * cannot reliably perform microcode updates beyond these levels
>> + * due to hardware issues. Therefore, we need to abort microcode
>> + * update process if we hit any of these levels.
>> + */
>> +static unsigned int final_levels[] = {
>> +    0x01000098,
>> +    0x0100009f,
>> +    0x010000af
>> +};
>> +
>> +static bool_t check_final_patch_levels(int cpu)
>> +{
>> +    /*
>> +     * Check the current patch levels on the cpu. If they are equal to
>> +     * any of the 'final_levels', then we should not update the 
>> microcode
>> +     * patch on the cpu as system will hang otherwise.
>> +     */
>> +    struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
>> +    unsigned int i;
>> +
>> +    for ( i = 0; i < ARRAY_SIZE(final_levels); i++ )
>> +        if ( uci->cpu_sig.rev == final_levels[i] )
>> +            return 1;
>
> (I should have asked this when you posted v1, sorry)
>
> Are these final_levels[] fam10h-specific? Are we guaranteed that other 
> families won't have them? I think family check would be good here. Or 
> at least a comment stating that these levels can only be observed on 
> family 10h (the only mention of family right now is in the commit 
> message)
>
>


Yep. These are Fam10h specific microcode patches. And yeah, the HW 
problem seems to exist only on these Fam10h specific processor variants. 
So, it's pretty isolated.

And why need a family check? The microcode patches are tied to a 
specific family/model anyway right?
It's only ever going to apply successfully if the equivalent id's match..

And sure, I can massage the comment like so-
" ... The short version is that people who are using certain Fam10h 
systems are noticing system hang issues when tryingupdate microcode..."

Would that be ok?

Thanks,
-Aravind.

Re: [PATCH V2] x86, amd_ucode: Skip microcode updates for final levels

[Boris Ostrovsky]

On 08/03/2015 01:52 PM, Aravind Gopalakrishnan wrote:
> On 8/3/2015 12:22 PM, Boris Ostrovsky wrote:
>> On 08/03/2015 12:35 PM, Aravind Gopalakrishnan wrote:
>>> Some of older[Fam10h] systems require that certain number of
>>> applied microcode patch levels should not be overwritten by
>>> the microcode loader. Otherwise, system hangs are known to occur.
>>>
>>> The 'final_levels' of patch ids have been obtained empirically.
>>> Refer bug https://bugzilla.suse.com/show_bug.cgi?id=913996
>>> for details of the issue.
>>>
>>> The short version is that people have predominantly noticed
>>> system hang issues when trying to update microcode levels
>>> beyond the patch IDs below.
>>> [0x01000098, 0x0100009f, 0x010000af]
>>>
>>>
>>>
>>>   +/*
>>> + * The 'final_levels' of patch ids have been obtained empirically.
>>> + * Refer bug https://bugzilla.suse.com/show_bug.cgi?id=913996
>>> + * for details of the issue. The short version is that people
>>> + * have predominantly noticed system hang issues when trying to
>>> + * update microcode levels beyond the patch IDs below.
>>> + * From internal discussions, we gathered that OS/hypervisor
>>> + * cannot reliably perform microcode updates beyond these levels
>>> + * due to hardware issues. Therefore, we need to abort microcode
>>> + * update process if we hit any of these levels.
>>> + */
>>> +static unsigned int final_levels[] = {
>>> +    0x01000098,
>>> +    0x0100009f,
>>> +    0x010000af
>>> +};
>>> +
>>> +static bool_t check_final_patch_levels(int cpu)
>>> +{
>>> +    /*
>>> +     * Check the current patch levels on the cpu. If they are equal to
>>> +     * any of the 'final_levels', then we should not update the 
>>> microcode
>>> +     * patch on the cpu as system will hang otherwise.
>>> +     */
>>> +    struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
>>> +    unsigned int i;
>>> +
>>> +    for ( i = 0; i < ARRAY_SIZE(final_levels); i++ )
>>> +        if ( uci->cpu_sig.rev == final_levels[i] )
>>> +            return 1;
>>
>> (I should have asked this when you posted v1, sorry)
>>
>> Are these final_levels[] fam10h-specific? Are we guaranteed that 
>> other families won't have them? I think family check would be good 
>> here. Or at least a comment stating that these levels can only be 
>> observed on family 10h (the only mention of family right now is in 
>> the commit message)
>>
>>
>
>
> Yep. These are Fam10h specific microcode patches. And yeah, the HW 
> problem seems to exist only on these Fam10h specific processor 
> variants. So, it's pretty isolated.
>
> And why need a family check? The microcode patches are tied to a 
> specific family/model anyway right?

Patches --- yes. I wasn't sure about levels (I hope we will never see 
the same level for different families but AFAIK it's not strictly 
prohibited since level is just a cookie).

> It's only ever going to apply successfully if the equivalent id's match..
>
> And sure, I can massage the comment like so-
> " ... The short version is that people who are using certain Fam10h 
> systems are noticing system hang issues when tryingupdate microcode..."
>
> Would that be ok?

Better than nothing (although TBH I'd rather see both the comment and 
the check).

-boris

>
> Thanks,
> -Aravind.
>

Re: [PATCH V2] x86, amd_ucode: Skip microcode updates for final levels

[Aravind Gopalakrishnan]

On 8/3/2015 1:18 PM, Boris Ostrovsky wrote:
> On 08/03/2015 01:52 PM, Aravind Gopalakrishnan wrote:
>> On 8/3/2015 12:22 PM, Boris Ostrovsky wrote:
>>> On 08/03/2015 12:35 PM, Aravind Gopalakrishnan wrote:
>>>> Some of older[Fam10h] systems require that certain number of
>>>> applied microcode patch levels should not be overwritten by
>>>> the microcode loader. Otherwise, system hangs are known to occur.
>>>>
>>>> The 'final_levels' of patch ids have been obtained empirically.
>>>> Refer bug https://bugzilla.suse.com/show_bug.cgi?id=913996
>>>> for details of the issue.
>>>>
>>>> The short version is that people have predominantly noticed
>>>> system hang issues when trying to update microcode levels
>>>> beyond the patch IDs below.
>>>> [0x01000098, 0x0100009f, 0x010000af]
>>>>
>>>>
>>>>
>>>>   +/*
>>>> + * The 'final_levels' of patch ids have been obtained empirically.
>>>> + * Refer bug https://bugzilla.suse.com/show_bug.cgi?id=913996
>>>> + * for details of the issue. The short version is that people
>>>> + * have predominantly noticed system hang issues when trying to
>>>> + * update microcode levels beyond the patch IDs below.
>>>> + * From internal discussions, we gathered that OS/hypervisor
>>>> + * cannot reliably perform microcode updates beyond these levels
>>>> + * due to hardware issues. Therefore, we need to abort microcode
>>>> + * update process if we hit any of these levels.
>>>> + */
>>>> +static unsigned int final_levels[] = {
>>>> +    0x01000098,
>>>> +    0x0100009f,
>>>> +    0x010000af
>>>> +};
>>>> +
>>>> +static bool_t check_final_patch_levels(int cpu)
>>>> +{
>>>> +    /*
>>>> +     * Check the current patch levels on the cpu. If they are 
>>>> equal to
>>>> +     * any of the 'final_levels', then we should not update the 
>>>> microcode
>>>> +     * patch on the cpu as system will hang otherwise.
>>>> +     */
>>>> +    struct ucode_cpu_info *uci = &per_cpu(ucode_cpu_info, cpu);
>>>> +    unsigned int i;
>>>> +
>>>> +    for ( i = 0; i < ARRAY_SIZE(final_levels); i++ )
>>>> +        if ( uci->cpu_sig.rev == final_levels[i] )
>>>> +            return 1;
>>>
>>> (I should have asked this when you posted v1, sorry)
>>>
>>> Are these final_levels[] fam10h-specific? Are we guaranteed that 
>>> other families won't have them? I think family check would be good 
>>> here. Or at least a comment stating that these levels can only be 
>>> observed on family 10h (the only mention of family right now is in 
>>> the commit message)
>>>
>>>
>>
>>
>> Yep. These are Fam10h specific microcode patches. And yeah, the HW 
>> problem seems to exist only on these Fam10h specific processor 
>> variants. So, it's pretty isolated.
>>
>> And why need a family check? The microcode patches are tied to a 
>> specific family/model anyway right?
>
> Patches --- yes. I wasn't sure about levels (I hope we will never see 
> the same level for different families but AFAIK it's not strictly 
> prohibited since level is just a cookie).
>

Ah. I see what you mean.
I can think of two ways around this-
a. I can move the check_final_patch_levels() call to apply_microcode(). 
That way, our initial checks in microcode_fits() would have already 
checked equivalent cpu IDs so we know we are on the right family.
b. Introduce a family check in this patch as you suggested (It's a 
simple change, should work fine too and I think can retain Andrew's 
'Reviewed-by' as the logic doesn't change too much..)

Do let me know if you have a preference.


>> It's only ever going to apply successfully if the equivalent id's 
>> match..
>>
>> And sure, I can massage the comment like so-
>> " ... The short version is that people who are using certain Fam10h 
>> systems are noticing system hang issues when tryingupdate microcode..."
>>
>> Would that be ok?
>
> Better than nothing (although TBH I'd rather see both the comment and 
> the check).
>

And sure, I'll include a comment as well.

Thanks,
-Aravind.

Re: [PATCH V2] x86, amd_ucode: Skip microcode updates for final levels

[Boris Ostrovsky]

On 08/03/2015 02:42 PM, Aravind Gopalakrishnan wrote:
>
> Ah. I see what you mean.
> I can think of two ways around this-
> a. I can move the check_final_patch_levels() call to 
> apply_microcode(). That way, our initial checks in microcode_fits() 
> would have already checked equivalent cpu IDs so we know we are on the 
> right family.
> b. Introduce a family check in this patch as you suggested (It's a 
> simple change, should work fine too and I think can retain Andrew's 
> 'Reviewed-by' as the logic doesn't change too much..)
>
> Do let me know if you have a preference.

I'd leave it where it is right now since there is no reason to do all 
allocations/checks/stuff and then get to apply_microcode() and find out 
that it was all for nothing. So just testing boot_cpu_data.x86 in 
check_final_patch_levels() and returning if it's not 10h should be 
sufficient.

Thanks.
-boris

Re: [PATCH V2] x86, amd_ucode: Skip microcode updates for final levels

[Aravind Gopalakrishnan]

On 8/3/2015 2:00 PM, Boris Ostrovsky wrote:
> On 08/03/2015 02:42 PM, Aravind Gopalakrishnan wrote:
>>
>> Ah. I see what you mean.
>> I can think of two ways around this-
>> a. I can move the check_final_patch_levels() call to 
>> apply_microcode(). That way, our initial checks in microcode_fits() 
>> would have already checked equivalent cpu IDs so we know we are on 
>> the right family.
>> b. Introduce a family check in this patch as you suggested (It's a 
>> simple change, should work fine too and I think can retain Andrew's 
>> 'Reviewed-by' as the logic doesn't change too much..)
>>
>> Do let me know if you have a preference.
>
> I'd leave it where it is right now since there is no reason to do all 
> allocations/checks/stuff and then get to apply_microcode() and find 
> out that it was all for nothing. So just testing boot_cpu_data.x86 in 
> check_final_patch_levels() and returning if it's not 10h should be 
> sufficient.
>

Okay, will do it this way and resend.

Thanks,
-Aravind.