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

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

[Aravind Gopalakrishnan]

Some of older[Fam10h] systems require that the microcode versions
that it comes up with should not be updated by the microcode driver.
Otherwise, system hangs are known to occur.

In this patch, we check for those microcode versions and abort the
update process if existing microcode level is already 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>
---
 xen/arch/x86/microcode_amd.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
index f79b397..c958a47 100644
--- a/xen/arch/x86/microcode_amd.c
+++ b/xen/arch/x86/microcode_amd.c
@@ -347,6 +347,30 @@ static int container_fast_forward(const void *data, size_t size_left, size_t *of
     return 0;
 }
 
+static unsigned int final_levels[] = {
+    0x01000098,
+    0x0100009f,
+    0x010000af,
+    0
+};
+
+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);
+    int i;
+
+    for ( i = 0; final_levels[i]; 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 +393,13 @@ static int cpu_request_microcode(int cpu, const void *buf, size_t bufsize)
         goto out;
     }
 
+    if ( check_final_patch_levels(cpu) )
+    {
+        pr_debug("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] x86, amd_ucode: Skip microcode updates for final levels

[Boris Ostrovsky]

On 07/30/2015 12:23 PM, Aravind Gopalakrishnan wrote:
> Some of older[Fam10h] systems require that the microcode versions
> that it comes up with should not be updated by the microcode driver.
> Otherwise, system hangs are known to occur.
>
> In this patch, we check for those microcode versions and abort the
> update process if existing microcode level is already 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>
> ---
>   xen/arch/x86/microcode_amd.c | 31 +++++++++++++++++++++++++++++++
>   1 file changed, 31 insertions(+)
>
> diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
> index f79b397..c958a47 100644
> --- a/xen/arch/x86/microcode_amd.c
> +++ b/xen/arch/x86/microcode_amd.c
> @@ -347,6 +347,30 @@ static int container_fast_forward(const void *data, size_t size_left, size_t *of
>       return 0;
>   }
>   
> +static unsigned int final_levels[] = {
> +    0x01000098,
> +    0x0100009f,
> +    0x010000af,
> +    0
> +};

Are these values documented somewhere?

-boris


> +
> +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);
> +    int i;
> +
> +    for ( i = 0; final_levels[i]; 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 +393,13 @@ static int cpu_request_microcode(int cpu, const void *buf, size_t bufsize)
>           goto out;
>       }
>   
> +    if ( check_final_patch_levels(cpu) )
> +    {
> +        pr_debug("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] x86, amd_ucode: Skip microcode updates for final levels

[Andrew Cooper]

On 30/07/15 17:23, Aravind Gopalakrishnan wrote:
> Some of older[Fam10h] systems require that the microcode versions
> that it comes up with should not be updated by the microcode driver.
> Otherwise, system hangs are known to occur.
>
> In this patch, we check for those microcode versions and abort the
> update process if existing microcode level is already 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>
> ---
>  xen/arch/x86/microcode_amd.c | 31 +++++++++++++++++++++++++++++++
>  1 file changed, 31 insertions(+)
>
> diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
> index f79b397..c958a47 100644
> --- a/xen/arch/x86/microcode_amd.c
> +++ b/xen/arch/x86/microcode_amd.c
> @@ -347,6 +347,30 @@ static int container_fast_forward(const void *data, size_t size_left, size_t *of
>      return 0;
>  }
>  

Please include the same comment as the Linux patch, explaining that
these microcode versions can't be updated from.

I would also like to see some documentation from AMD concerning this.

> +static unsigned int final_levels[] = {
> +    0x01000098,
> +    0x0100009f,
> +    0x010000af,
> +    0
> +};
> +
> +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);
> +    int i;

unsigned

> +
> +    for ( i = 0; final_levels[i]; i++ )

ARRAY_SIZE(), and drop the 0 on the end of the list.

> +        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 +393,13 @@ static int cpu_request_microcode(int cpu, const void *buf, size_t bufsize)
>          goto out;
>      }
>  
> +    if ( check_final_patch_levels(cpu) )
> +    {
> +        pr_debug("microcode: Cannot update microcode patch on the cpu as we hit a final level\n");

pr_debug() is compiled out completely.  I would suggest
printk(XENLOG_INFO instead.

~Andrew

> +        error = -EPERM;
> +        goto out;
> +    }
> +
>      mc_amd = xmalloc(struct microcode_amd);
>      if ( !mc_amd )
>      {

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

[Aravind Gopalakrishnan]

On 7/30/2015 12:01 PM, Andrew Cooper wrote:
> On 30/07/15 17:23, Aravind Gopalakrishnan wrote:
>> Some of older[Fam10h] systems require that the microcode versions
>> that it comes up with should not be updated by the microcode driver.
>> Otherwise, system hangs are known to occur.
>>
>> In this patch, we check for those microcode versions and abort the
>> update process if existing microcode level is already 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>
>> ---
>>   xen/arch/x86/microcode_amd.c | 31 +++++++++++++++++++++++++++++++
>>   1 file changed, 31 insertions(+)
>>
>> diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
>> index f79b397..c958a47 100644
>> --- a/xen/arch/x86/microcode_amd.c
>> +++ b/xen/arch/x86/microcode_amd.c
>> @@ -347,6 +347,30 @@ static int container_fast_forward(const void *data, size_t size_left, size_t *of
>>       return 0;
>>   }
>>   
> Please include the same comment as the Linux patch, explaining that
> these microcode versions can't be updated from.

Ok, will do that.

> I would also like to see some documentation from AMD concerning this.

(hopefully) answering Boris' question too here-

So, the patch id values have only been obtained empirically.
The Linux patch provides the bug reference for 
this:https://bugzilla.suse.com/show_bug.cgi?id=913996
(It's a fairly long thread but the gist of it is that people 
predominantly seem to be experiencing system hang issues
when they try to update microcode levels from these patch ids: 
0x01000098,  0x0100009f, 0x010000af)

 From discussing about it internally, we gathered that OS/hypervisor 
cannot reliably perform microcode updates beyond these specified levels
due to HW issues.

>> +static unsigned int final_levels[] = {
>> +    0x01000098,
>> +    0x0100009f,
>> +    0x010000af,
>> +    0
>> +};
>> +
>> +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);
>> +    int i;
> unsigned

Will fix this.

>> +
>> +    for ( i = 0; final_levels[i]; i++ )
> ARRAY_SIZE(), and drop the 0 on the end of the list.


Ok, will fix.

>> +        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 +393,13 @@ static int cpu_request_microcode(int cpu, const void *buf, size_t bufsize)
>>           goto out;
>>       }
>>   
>> +    if ( check_final_patch_levels(cpu) )
>> +    {
>> +        pr_debug("microcode: Cannot update microcode patch on the cpu as we hit a final level\n");
> pr_debug() is compiled out completely.  I would suggest
> printk(XENLOG_INFO instead.

Ok, Will fix this and send out a V2 with the changes.

Thanks,
-Aravind.

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

[Andrew Cooper]

On 31/07/2015 21:45, Aravind Gopalakrishnan wrote:
> On 7/30/2015 12:01 PM, Andrew Cooper wrote:
>> On 30/07/15 17:23, Aravind Gopalakrishnan wrote:
>>> Some of older[Fam10h] systems require that the microcode versions
>>> that it comes up with should not be updated by the microcode driver.
>>> Otherwise, system hangs are known to occur.
>>>
>>> In this patch, we check for those microcode versions and abort the
>>> update process if existing microcode level is already 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>
>>> ---
>>>   xen/arch/x86/microcode_amd.c | 31 +++++++++++++++++++++++++++++++
>>>   1 file changed, 31 insertions(+)
>>>
>>> diff --git a/xen/arch/x86/microcode_amd.c
>>> b/xen/arch/x86/microcode_amd.c
>>> index f79b397..c958a47 100644
>>> --- a/xen/arch/x86/microcode_amd.c
>>> +++ b/xen/arch/x86/microcode_amd.c
>>> @@ -347,6 +347,30 @@ static int container_fast_forward(const void
>>> *data, size_t size_left, size_t *of
>>>       return 0;
>>>   }
>>>   
>> Please include the same comment as the Linux patch, explaining that
>> these microcode versions can't be updated from.
>
> Ok, will do that.
>
>> I would also like to see some documentation from AMD concerning this.
>
> (hopefully) answering Boris' question too here-
>
> So, the patch id values have only been obtained empirically.
> The Linux patch provides the bug reference for
> this:https://bugzilla.suse.com/show_bug.cgi?id=913996
> (It's a fairly long thread but the gist of it is that people
> predominantly seem to be experiencing system hang issues
> when they try to update microcode levels from these patch ids:
> 0x01000098,  0x0100009f, 0x010000af)
>
> From discussing about it internally, we gathered that OS/hypervisor
> cannot reliably perform microcode updates beyond these specified levels
> due to HW issues.

Ok - please include this information in the comment as well please.

~Andrew

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

[Aravind Gopalakrishnan]

On 7/31/2015 3:48 PM, Andrew Cooper wrote:
>
>> So, the patch id values have only been obtained empirically.
>> The Linux patch provides the bug reference for
>> this:https://bugzilla.suse.com/show_bug.cgi?id=913996
>> (It's a fairly long thread but the gist of it is that people
>> predominantly seem to be experiencing system hang issues
>> when they try to update microcode levels from these patch ids:
>> 0x01000098,  0x0100009f, 0x010000af)
>>
>>  From discussing about it internally, we gathered that OS/hypervisor
>> cannot reliably perform microcode updates beyond these specified levels
>> due to HW issues.
> Ok - please include this information in the comment as well please.
>

Will do that.

Thanks,
-Aravind.