[PATCH] x86, amd_ucode: Safeguard against #GP

[PATCH] x86, amd_ucode: Safeguard against #GP

[Aravind Gopalakrishnan]

When HW tries to load a corrupted patch, it generates #GP
and hangs the system. Use wrmsr_safe instead so that we
fail to load microcode gracefully.

Example on a Fam15h system-
(XEN) microcode: CPU0 collect_cpu_info: patch_id=0x6000626
(XEN) microcode: CPU0 size 7870, block size 2586 offset 76 equivID
0x6012 rev 0x6000637
(XEN) microcode: CPU0 found a matching microcode update with version
0x6000637 (current=0x6000626)
(XEN) traps.c:3073: GPF (0000): ffff82d08016f682 -> ffff82d08022d9f8
(XEN) microcode: CPU0 update from revision 0x6000637 to 0x6000626 failed

Signed-off-by: Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>
---
 xen/arch/x86/microcode_amd.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
index e83f4b6..23637e2 100644
--- a/xen/arch/x86/microcode_amd.c
+++ b/xen/arch/x86/microcode_amd.c
@@ -191,7 +191,7 @@ static int apply_microcode(int cpu)
 
     spin_lock_irqsave(&microcode_update_lock, flags);
 
-    wrmsrl(MSR_AMD_PATCHLOADER, (unsigned long)hdr);
+    wrmsr_safe(MSR_AMD_PATCHLOADER, (unsigned long)hdr);
 
     /* get patch id after patching */
     rdmsrl(MSR_AMD_PATCHLEVEL, rev);
-- 
1.7.9.5

Re: [PATCH] x86, amd_ucode: Safeguard against #GP

[Boris Ostrovsky]

On 05/21/2014 05:28 PM, Aravind Gopalakrishnan wrote:
> When HW tries to load a corrupted patch, it generates #GP
> and hangs the system. Use wrmsr_safe instead so that we
> fail to load microcode gracefully.
>
> Example on a Fam15h system-
> (XEN) microcode: CPU0 collect_cpu_info: patch_id=0x6000626
> (XEN) microcode: CPU0 size 7870, block size 2586 offset 76 equivID
> 0x6012 rev 0x6000637
> (XEN) microcode: CPU0 found a matching microcode update with version
> 0x6000637 (current=0x6000626)
> (XEN) traps.c:3073: GPF (0000): ffff82d08016f682 -> ffff82d08022d9f8
> (XEN) microcode: CPU0 update from revision 0x6000637 to 0x6000626 failed
>
> Signed-off-by: Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

> ---
>   xen/arch/x86/microcode_amd.c |    2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
> index e83f4b6..23637e2 100644
> --- a/xen/arch/x86/microcode_amd.c
> +++ b/xen/arch/x86/microcode_amd.c
> @@ -191,7 +191,7 @@ static int apply_microcode(int cpu)
>   
>       spin_lock_irqsave(&microcode_update_lock, flags);
>   
> -    wrmsrl(MSR_AMD_PATCHLOADER, (unsigned long)hdr);
> +    wrmsr_safe(MSR_AMD_PATCHLOADER, (unsigned long)hdr);
>   
>       /* get patch id after patching */
>       rdmsrl(MSR_AMD_PATCHLEVEL, rev);

Re: [PATCH] x86, amd_ucode: Safeguard against #GP

[Andrew Cooper]

On 21/05/2014 22:28, Aravind Gopalakrishnan wrote:
> When HW tries to load a corrupted patch, it generates #GP
> and hangs the system. Use wrmsr_safe instead so that we
> fail to load microcode gracefully.
>
> Example on a Fam15h system-
> (XEN) microcode: CPU0 collect_cpu_info: patch_id=0x6000626
> (XEN) microcode: CPU0 size 7870, block size 2586 offset 76 equivID
> 0x6012 rev 0x6000637
> (XEN) microcode: CPU0 found a matching microcode update with version
> 0x6000637 (current=0x6000626)
> (XEN) traps.c:3073: GPF (0000): ffff82d08016f682 -> ffff82d08022d9f8
> (XEN) microcode: CPU0 update from revision 0x6000637 to 0x6000626 failed
>
> Signed-off-by: Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>

I suspect that you will find with a *very* up-to-date master tree, an
early #GP should result in panic() instead of a hang, following
279840d5ea646  (If it doesn't I would be interested to know)

As for the patch itself, is it worth using the failure to purge this
patch from the cache? Nothing good can come of attempting to load the
same patch on a different cpu.

~Andrew

> ---
>  xen/arch/x86/microcode_amd.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
> index e83f4b6..23637e2 100644
> --- a/xen/arch/x86/microcode_amd.c
> +++ b/xen/arch/x86/microcode_amd.c
> @@ -191,7 +191,7 @@ static int apply_microcode(int cpu)
>  
>      spin_lock_irqsave(&microcode_update_lock, flags);
>  
> -    wrmsrl(MSR_AMD_PATCHLOADER, (unsigned long)hdr);
> +    wrmsr_safe(MSR_AMD_PATCHLOADER, (unsigned long)hdr);
>  
>      /* get patch id after patching */
>      rdmsrl(MSR_AMD_PATCHLEVEL, rev);

Re: [PATCH] x86, amd_ucode: Safeguard against #GP

[Aravind Gopalakrishnan]

[-- Attachment #1: Type: text/plain, Size: 1375 bytes --]

On 5/21/2014 5:08 PM, Andrew Cooper wrote:
> On 21/05/2014 22:28, Aravind Gopalakrishnan wrote:
>> When HW tries to load a corrupted patch, it generates #GP
>> and hangs the system. Use wrmsr_safe instead so that we
>> fail to load microcode gracefully.
>>
>> Example on a Fam15h system-
>> (XEN) microcode: CPU0 collect_cpu_info: patch_id=0x6000626
>> (XEN) microcode: CPU0 size 7870, block size 2586 offset 76 equivID
>> 0x6012 rev 0x6000637
>> (XEN) microcode: CPU0 found a matching microcode update with version
>> 0x6000637 (current=0x6000626)
>> (XEN) traps.c:3073: GPF (0000): ffff82d08016f682 -> ffff82d08022d9f8
>> (XEN) microcode: CPU0 update from revision 0x6000637 to 0x6000626 failed
>>
>> Signed-off-by: Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>
> I suspect that you will find with a *very* up-to-date master tree, an
> early #GP should result in panic() instead of a hang, following
> 279840d5ea646  (If it doesn't I would be interested to know)

The console log states 'Panic on CPU0'. But system is basically hung 
after that.
(attached snapshot)
> As for the patch itself, is it worth using the failure to purge this
> patch from the cache? Nothing good can come of attempting to load the
> same patch on a different cpu.
>
>

I don't think we do..
We only save the patch for apply on resume if 'applied_offset' has a 
value right?

-Aravind.

[-- Attachment #2: xen-hang-latest-tree.jpg --]
[-- Type: image/jpeg, Size: 1660671 bytes --]

[-- Attachment #3: Type: text/plain, Size: 126 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [PATCH] x86, amd_ucode: Safeguard against #GP

[Andrew Cooper]

On 22/05/14 16:53, Aravind Gopalakrishnan wrote:
> On 5/21/2014 5:08 PM, Andrew Cooper wrote:
>> On 21/05/2014 22:28, Aravind Gopalakrishnan wrote:
>>> When HW tries to load a corrupted patch, it generates #GP
>>> and hangs the system. Use wrmsr_safe instead so that we
>>> fail to load microcode gracefully.
>>>
>>> Example on a Fam15h system-
>>> (XEN) microcode: CPU0 collect_cpu_info: patch_id=0x6000626
>>> (XEN) microcode: CPU0 size 7870, block size 2586 offset 76 equivID
>>> 0x6012 rev 0x6000637
>>> (XEN) microcode: CPU0 found a matching microcode update with version
>>> 0x6000637 (current=0x6000626)
>>> (XEN) traps.c:3073: GPF (0000): ffff82d08016f682 -> ffff82d08022d9f8
>>> (XEN) microcode: CPU0 update from revision 0x6000637 to 0x6000626
>>> failed
>>>
>>> Signed-off-by: Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>
>> I suspect that you will find with a *very* up-to-date master tree, an
>> early #GP should result in panic() instead of a hang, following
>> 279840d5ea646  (If it doesn't I would be interested to know)
>
> The console log states 'Panic on CPU0'. But system is basically hung
> after that.
> (attached snapshot)

I refer you to the last line :)

You have "noreboot" on your command line

~Andrew

Re: [PATCH] x86, amd_ucode: Safeguard against #GP

[Aravind Gopalakrishnan]

On 5/22/2014 10:56 AM, Andrew Cooper wrote:
> On 22/05/14 16:53, Aravind Gopalakrishnan wrote:
>> On 5/21/2014 5:08 PM, Andrew Cooper wrote:
>>> On 21/05/2014 22:28, Aravind Gopalakrishnan wrote:
>>>> When HW tries to load a corrupted patch, it generates #GP
>>>> and hangs the system. Use wrmsr_safe instead so that we
>>>> fail to load microcode gracefully.
>>>>
>>>> Example on a Fam15h system-
>>>> (XEN) microcode: CPU0 collect_cpu_info: patch_id=0x6000626
>>>> (XEN) microcode: CPU0 size 7870, block size 2586 offset 76 equivID
>>>> 0x6012 rev 0x6000637
>>>> (XEN) microcode: CPU0 found a matching microcode update with version
>>>> 0x6000637 (current=0x6000626)
>>>> (XEN) traps.c:3073: GPF (0000): ffff82d08016f682 -> ffff82d08022d9f8
>>>> (XEN) microcode: CPU0 update from revision 0x6000637 to 0x6000626
>>>> failed
>>>>
>>>> Signed-off-by: Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>
>>> I suspect that you will find with a *very* up-to-date master tree, an
>>> early #GP should result in panic() instead of a hang, following
>>> 279840d5ea646  (If it doesn't I would be interested to know)
>> The console log states 'Panic on CPU0'. But system is basically hung
>> after that.
>> (attached snapshot)
> I refer you to the last line :)
>
> You have "noreboot" on your command line
>

Ah. Okay, Will try again.

Thanks,
-Aravind.

Re: [PATCH] x86, amd_ucode: Safeguard against #GP

[Jan Beulich]

>>> On 21.05.14 at 23:28, <aravind.gopalakrishnan@amd.com> wrote:
> When HW tries to load a corrupted patch, it generates #GP
> and hangs the system. Use wrmsr_safe instead so that we
> fail to load microcode gracefully.
> 
> Example on a Fam15h system-
> (XEN) microcode: CPU0 collect_cpu_info: patch_id=0x6000626
> (XEN) microcode: CPU0 size 7870, block size 2586 offset 76 equivID
> 0x6012 rev 0x6000637
> (XEN) microcode: CPU0 found a matching microcode update with version
> 0x6000637 (current=0x6000626)
> (XEN) traps.c:3073: GPF (0000): ffff82d08016f682 -> ffff82d08022d9f8
> (XEN) microcode: CPU0 update from revision 0x6000637 to 0x6000626 failed
> 
> Signed-off-by: Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>
> ---
>  xen/arch/x86/microcode_amd.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
> index e83f4b6..23637e2 100644
> --- a/xen/arch/x86/microcode_amd.c
> +++ b/xen/arch/x86/microcode_amd.c
> @@ -191,7 +191,7 @@ static int apply_microcode(int cpu)
>  
>      spin_lock_irqsave(&microcode_update_lock, flags);
>  
> -    wrmsrl(MSR_AMD_PATCHLOADER, (unsigned long)hdr);
> +    wrmsr_safe(MSR_AMD_PATCHLOADER, (unsigned long)hdr);

I think you shouldn't ignore the "return" value here.

Also the last quoted log message above has the two revisions
reversed - mind fixing this at the same time?

Jan

Re: [PATCH] x86, amd_ucode: Safeguard against #GP

[Aravind Gopalakrishnan]

On 5/22/2014 3:44 AM, Jan Beulich wrote:
>>>> On 21.05.14 at 23:28, <aravind.gopalakrishnan@amd.com> wrote:
>> When HW tries to load a corrupted patch, it generates #GP
>> and hangs the system. Use wrmsr_safe instead so that we
>> fail to load microcode gracefully.
>>
>> Example on a Fam15h system-
>> (XEN) microcode: CPU0 collect_cpu_info: patch_id=0x6000626
>> (XEN) microcode: CPU0 size 7870, block size 2586 offset 76 equivID
>> 0x6012 rev 0x6000637
>> (XEN) microcode: CPU0 found a matching microcode update with version
>> 0x6000637 (current=0x6000626)
>> (XEN) traps.c:3073: GPF (0000): ffff82d08016f682 -> ffff82d08022d9f8
>> (XEN) microcode: CPU0 update from revision 0x6000637 to 0x6000626 failed
>>
>> Signed-off-by: Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>
>> ---
>>   xen/arch/x86/microcode_amd.c |    2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
>> index e83f4b6..23637e2 100644
>> --- a/xen/arch/x86/microcode_amd.c
>> +++ b/xen/arch/x86/microcode_amd.c
>> @@ -191,7 +191,7 @@ static int apply_microcode(int cpu)
>>   
>>       spin_lock_irqsave(&microcode_update_lock, flags);
>>   
>> -    wrmsrl(MSR_AMD_PATCHLOADER, (unsigned long)hdr);
>> +    wrmsr_safe(MSR_AMD_PATCHLOADER, (unsigned long)hdr);
> I think you shouldn't ignore the "return" value here.
Not sure I understand..
Do you mean you want to capture return value and bubble it up? maybe 
print a debug message too?
Since we check if patch application succeeded using -
if ( rev != hdr->patch_id )
and return a error val here, would this not be sufficient?

> Also the last quoted log message above has the two revisions
> reversed - mind fixing this at the same time?
>
>
Will fix this.

Thanks,
-Aravind.

Re: [PATCH] x86, amd_ucode: Safeguard against #GP

[Jan Beulich]

>>> On 22.05.14 at 17:59, <aravind.gopalakrishnan@amd.com> wrote:
> On 5/22/2014 3:44 AM, Jan Beulich wrote:
>>>>> On 21.05.14 at 23:28, <aravind.gopalakrishnan@amd.com> wrote:
>>> When HW tries to load a corrupted patch, it generates #GP
>>> and hangs the system. Use wrmsr_safe instead so that we
>>> fail to load microcode gracefully.
>>>
>>> Example on a Fam15h system-
>>> (XEN) microcode: CPU0 collect_cpu_info: patch_id=0x6000626
>>> (XEN) microcode: CPU0 size 7870, block size 2586 offset 76 equivID
>>> 0x6012 rev 0x6000637
>>> (XEN) microcode: CPU0 found a matching microcode update with version
>>> 0x6000637 (current=0x6000626)
>>> (XEN) traps.c:3073: GPF (0000): ffff82d08016f682 -> ffff82d08022d9f8
>>> (XEN) microcode: CPU0 update from revision 0x6000637 to 0x6000626 failed
>>>
>>> Signed-off-by: Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>
>>> ---
>>>   xen/arch/x86/microcode_amd.c |    2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
>>> index e83f4b6..23637e2 100644
>>> --- a/xen/arch/x86/microcode_amd.c
>>> +++ b/xen/arch/x86/microcode_amd.c
>>> @@ -191,7 +191,7 @@ static int apply_microcode(int cpu)
>>>   
>>>       spin_lock_irqsave(&microcode_update_lock, flags);
>>>   
>>> -    wrmsrl(MSR_AMD_PATCHLOADER, (unsigned long)hdr);
>>> +    wrmsr_safe(MSR_AMD_PATCHLOADER, (unsigned long)hdr);
>> I think you shouldn't ignore the "return" value here.
> Not sure I understand..
> Do you mean you want to capture return value and bubble it up? maybe 
> print a debug message too?

Yes.

> Since we check if patch application succeeded using -
> if ( rev != hdr->patch_id )
> and return a error val here, would this not be sufficient?

It might, but I'm of the opinion that errors should never be ignored
_and_ be detected as early as possible.

Jan

Re: [PATCH] x86, amd_ucode: Safeguard against #GP

[Aravind Gopalakrishnan]

On 5/23/2014 1:05 AM, Jan Beulich wrote:
>>>> On 22.05.14 at 17:59, <aravind.gopalakrishnan@amd.com> wrote:
>> On 5/22/2014 3:44 AM, Jan Beulich wrote:
>>>>>> On 21.05.14 at 23:28, <aravind.gopalakrishnan@amd.com> wrote:
>>>> When HW tries to load a corrupted patch, it generates #GP
>>>> and hangs the system. Use wrmsr_safe instead so that we
>>>> fail to load microcode gracefully.
>>>>
>>>> Example on a Fam15h system-
>>>> (XEN) microcode: CPU0 collect_cpu_info: patch_id=0x6000626
>>>> (XEN) microcode: CPU0 size 7870, block size 2586 offset 76 equivID
>>>> 0x6012 rev 0x6000637
>>>> (XEN) microcode: CPU0 found a matching microcode update with version
>>>> 0x6000637 (current=0x6000626)
>>>> (XEN) traps.c:3073: GPF (0000): ffff82d08016f682 -> ffff82d08022d9f8
>>>> (XEN) microcode: CPU0 update from revision 0x6000637 to 0x6000626 failed
>>>>
>>>> Signed-off-by: Aravind Gopalakrishnan <aravind.gopalakrishnan@amd.com>
>>>> ---
>>>>    xen/arch/x86/microcode_amd.c |    2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/xen/arch/x86/microcode_amd.c b/xen/arch/x86/microcode_amd.c
>>>> index e83f4b6..23637e2 100644
>>>> --- a/xen/arch/x86/microcode_amd.c
>>>> +++ b/xen/arch/x86/microcode_amd.c
>>>> @@ -191,7 +191,7 @@ static int apply_microcode(int cpu)
>>>>    
>>>>        spin_lock_irqsave(&microcode_update_lock, flags);
>>>>    
>>>> -    wrmsrl(MSR_AMD_PATCHLOADER, (unsigned long)hdr);
>>>> +    wrmsr_safe(MSR_AMD_PATCHLOADER, (unsigned long)hdr);
>>> I think you shouldn't ignore the "return" value here.
>> Not sure I understand..
>> Do you mean you want to capture return value and bubble it up? maybe
>> print a debug message too?
> Yes.
>
>> Since we check if patch application succeeded using -
>> if ( rev != hdr->patch_id )
>> and return a error val here, would this not be sufficient?
> It might, but I'm of the opinion that errors should never be ignored
> _and_ be detected as early as possible.
>
>
Have addressed above issues and sending out V2 with Boris' Reviewed-by tag.

Thanks,
-Aravind.