From mboxrd@z Thu Jan 1 00:00:00 1970 From: Aravind Gopalakrishnan Subject: Re: [PATCH] x86, amd_ucode: Safeguard against #GP Date: Thu, 22 May 2014 10:59:09 -0500 Message-ID: <537E1ECD.2000209@amd.com> References: <1400707718-3826-1-git-send-email-aravind.gopalakrishnan@amd.com> <537DD4F30200007800014D0D@mail.emea.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <537DD4F30200007800014D0D@mail.emea.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: boris.ostrovsky@oracle.com, keir@xen.org, xen-devel@lists.xen.org List-Id: xen-devel@lists.xenproject.org On 5/22/2014 3:44 AM, Jan Beulich wrote: >>>> On 21.05.14 at 23:28, 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 >> --- >> 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(µcode_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.