Re: [Xen-devel] [PATCH] x86/boot: Fix BIOS memory corruption on certain IBM systems

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 04.12.13 at 11:35, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:
> On 04/12/13 10:03, Jan Beulich wrote:
>>>>> On 03.12.13 at 21:34, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> 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.

This isn't pointless at all - it sets the segment attributes to real
mode compatible values (which the reloading in real mode does
_not_ do - this changes only the segment base).

Since we don't care about the particular values (read: base
addresses) of %fs and %gs (but we do care about those in
the other four selectors), not reloading them after entering
real mode is actually quite sensible.

>>> @@ -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.
> 
> 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.

As said - I doubt this would help much. I'd really prefer at least this
part of the patch to be taken out again. Unless Keir is specifically of
the opposite opinion...

Jan


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