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

[Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>]

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.