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

[Andrew Cooper <andrew.cooper3@citrix.com>]

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