From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH] x86/boot: Fix BIOS memory corruption on certain IBM systems Date: Wed, 4 Dec 2013 10:35:34 +0000 Message-ID: <529F0576.8050606@citrix.com> References: <1386102859-14477-1-git-send-email-andrew.cooper3@citrix.com> <529F0BF60200007800109E0C@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1Vo9nu-0006p7-8s for xen-devel@lists.xenproject.org; Wed, 04 Dec 2013 10:35:38 +0000 In-Reply-To: <529F0BF60200007800109E0C@nat28.tlf.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: George Dunlap , xen-devel , Keir Fraser List-Id: xen-devel@lists.xenproject.org On 04/12/13 10:03, Jan Beulich wrote: >>>> On 03.12.13 at 21:34, Andrew Cooper wrote: >> --- a/xen/arch/x86/boot/trampoline.S >> +++ b/xen/arch/x86/boot/trampoline.S >> @@ -140,10 +140,12 @@ trampoline_boot_cpu_entry: >> 1: mov %cs,%ax >> mov %ax,%ds >> mov %ax,%es >> + mov %ax,%fs >> + mov %ax,%gs >> mov %ax,%ss >> >> /* Initialise stack pointer and IDT, and enable irqs. */ >> - xor %sp,%sp >> + xor %esp,%esp > According to your findings this one line change is really all that's > needed. I believe this to be the case, yes. > While I may be willing to accept the setting of %fs and > %gs, despite them being set to BOOT_PSEUDORM_DS right > before leaving protected mode (albeit I think it would be better > to clear them than to make them match %cs), ... The set to BOOT_PSEUDORM_DS in 32bit mode is quite pointless, as they are never used and reloaded moments later in 16bit mode. I have already queued it up in my Xen-4.5 improvements series to the early boot code which I have been collecting while debugging this issue. > >> @@ -151,6 +153,11 @@ trampoline_boot_cpu_entry: >> * Declare that our target operating mode is long mode. >> * Initialise 32-bit registers since some buggy BIOSes depend on it. >> */ >> + xor %ecx,%ecx >> + xor %edx,%edx >> + xor %esi,%esi >> + xor %edi,%edi >> + xor %ebp,%ebp >> movl $0xec00,%eax # declare target operating mode >> movl $0x0002,%ebx # long mode >> int $0x15 > ... I can't really see the value of the change here: If we're to > work around theoretical BIOS bugs, we'd need to do this prior to > each BIOS call. That's surely overkill. Therefore let's focus on > what is needed to work around _known_ BIOS bugs. > > Jan > I admit that I was leaning on the cautious side with these changes. I can take them out if you think that would be better, but given this int was already flagged as buggy in some BIOSes, and we have found another case, I think covering all GPRs is the safer option. ~Andrew