seabios 0.6.1 regression

seabios 0.6.1 regression

[Avi Kivity]

Installing Windows XP with seabios 0.6.1, immediately after the first 
reboot, Windows hangs in protected mode instead of proceeding with 
installation.

I'm bisecting this, but if anyone can point to a likely culprit, I can 
try it first.

-- 
error compiling committee.c: too many arguments to function

Re: seabios 0.6.1 regression

[Avi Kivity]

On 11/15/2010 03:39 PM, Avi Kivity wrote:
> Installing Windows XP with seabios 0.6.1, immediately after the first 
> reboot, Windows hangs in protected mode instead of proceeding with 
> installation.
>
> I'm bisecting this, but if anyone can point to a likely culprit, I can 
> try it first.
>

Bisect says:

commit 9a01a9c3eb336eca37c17fd74c79806ee0bda05b
Author: Kevin O'Connor <kevin@koconnor.net>
Date:   Wed Aug 25 21:07:48 2010 -0400

     Only show bootsplash during boot menu.

     When the bootsplash picture is shown, it's not possible to see text.
     So, only display the picture while prompting the user for the boot
     menu.

git bisect start 'rel-0.6.1' '17d3e46511aeedc9f09a8216d194d749187b80aa'
# good: [b4525a0ec176426788f293cce92160e6573e86b6] Handle unaligned 
sizes in iomemcpy().
git bisect good b4525a0ec176426788f293cce92160e6573e86b6
# good: [e2074bf6ec2956e1d803e62dcb052b7c88c214f0] Add ACPI SSDT/DSDT 
support for CPU hotplug.
git bisect good e2074bf6ec2956e1d803e62dcb052b7c88c214f0
# bad: [7ce09ae6542f0f4187024ae3267b61a0cf6ebd39] Make 
tools/transdump.py more resilient to unknown input.
git bisect bad 7ce09ae6542f0f4187024ae3267b61a0cf6ebd39
# good: [5feb83c8a55744397b4dd208fb4016a5c051222e] add write support to 
virtio-blk
git bisect good 5feb83c8a55744397b4dd208fb4016a5c051222e
# bad: [6039fc55274deb7202060d08e0f23b9f3dcface4] Update qemu_cfg_read 
to use "rep insb".
git bisect bad 6039fc55274deb7202060d08e0f23b9f3dcface4


-- 
error compiling committee.c: too many arguments to function

Re: seabios 0.6.1 regression

[Avi Kivity]

On 11/15/2010 05:04 PM, Avi Kivity wrote:
> On 11/15/2010 03:39 PM, Avi Kivity wrote:
>> Installing Windows XP with seabios 0.6.1, immediately after the first 
>> reboot, Windows hangs in protected mode instead of proceeding with 
>> installation.
>>
>> I'm bisecting this, but if anyone can point to a likely culprit, I 
>> can try it first.
>>
>
> Bisect says:
>
> commit 9a01a9c3eb336eca37c17fd74c79806ee0bda05b
> Author: Kevin O'Connor <kevin@koconnor.net>
> Date:   Wed Aug 25 21:07:48 2010 -0400
>
>     Only show bootsplash during boot menu.
>
>     When the bootsplash picture is shown, it's not possible to see text.
>     So, only display the picture while prompting the user for the boot
>     menu.
>
> git bisect start 'rel-0.6.1' '17d3e46511aeedc9f09a8216d194d749187b80aa'
> # good: [b4525a0ec176426788f293cce92160e6573e86b6] Handle unaligned 
> sizes in iomemcpy().
> git bisect good b4525a0ec176426788f293cce92160e6573e86b6
> # good: [e2074bf6ec2956e1d803e62dcb052b7c88c214f0] Add ACPI SSDT/DSDT 
> support for CPU hotplug.
> git bisect good e2074bf6ec2956e1d803e62dcb052b7c88c214f0
> # bad: [7ce09ae6542f0f4187024ae3267b61a0cf6ebd39] Make 
> tools/transdump.py more resilient to unknown input.
> git bisect bad 7ce09ae6542f0f4187024ae3267b61a0cf6ebd39
> # good: [5feb83c8a55744397b4dd208fb4016a5c051222e] add write support 
> to virtio-blk
> git bisect good 5feb83c8a55744397b4dd208fb4016a5c051222e
> # bad: [6039fc55274deb7202060d08e0f23b9f3dcface4] Update qemu_cfg_read 
> to use "rep insb".
> git bisect bad 6039fc55274deb7202060d08e0f23b9f3dcface4
>
>

That was premature, the real culprit (if I didn't mess up the bisect) is:

commit 6039fc55274deb7202060d08e0f23b9f3dcface4
Author: Kevin O'Connor <kevin@koconnor.net>
Date:   Wed Aug 25 21:43:19 2010 -0400

     Update qemu_cfg_read to use "rep insb".

     Use rep insb instead of manual loop - the host may be able to optimize
     the rep insb instruction.

Which doesn't make any sense - will do more testing.

-- 
error compiling committee.c: too many arguments to function

Re: seabios 0.6.1 regression

[Avi Kivity]

On 11/15/2010 05:12 PM, Avi Kivity wrote:
>
> That was premature, the real culprit (if I didn't mess up the bisect) is:
>
> commit 6039fc55274deb7202060d08e0f23b9f3dcface4
> Author: Kevin O'Connor <kevin@koconnor.net>
> Date:   Wed Aug 25 21:43:19 2010 -0400
>
>     Update qemu_cfg_read to use "rep insb".
>
>     Use rep insb instead of manual loop - the host may be able to 
> optimize
>     the rep insb instruction.
>
> Which doesn't make any sense - will do more testing.
>

Confirmed with multiple tests.

-- 
error compiling committee.c: too many arguments to function

Re: seabios 0.6.1 regression

[Avi Kivity]

On 11/15/2010 05:34 PM, Avi Kivity wrote:
> On 11/15/2010 05:12 PM, Avi Kivity wrote:
>>
>> That was premature, the real culprit (if I didn't mess up the bisect) 
>> is:
>>
>> commit 6039fc55274deb7202060d08e0f23b9f3dcface4
>> Author: Kevin O'Connor <kevin@koconnor.net>
>> Date:   Wed Aug 25 21:43:19 2010 -0400
>>
>>     Update qemu_cfg_read to use "rep insb".
>>
>>     Use rep insb instead of manual loop - the host may be able to 
>> optimize
>>     the rep insb instruction.
>>
>> Which doesn't make any sense - will do more testing.
>>
>
> Confirmed with multiple tests.
>

I think it's a miscompile.

out/code16.o:
  1a4:   3e                      ds
  1a5:   6c                      insb   (%dx),%es:(%edi)

Note no 66 prefix.

-- 
error compiling committee.c: too many arguments to function

Re: seabios 0.6.1 regression

[Avi Kivity]

On 11/15/2010 05:41 PM, Avi Kivity wrote:
>
> I think it's a miscompile.
>
> out/code16.o:
>  1a4:   3e                      ds
>  1a5:   6c                      insb   (%dx),%es:(%edi)
>
> Note no 66 prefix.
>

It isn't, that was random crap.  All the insb() code is 32-bit.

-- 
error compiling committee.c: too many arguments to function

Re: seabios 0.6.1 regression

[Avi Kivity]

On 11/15/2010 05:49 PM, Avi Kivity wrote:
> On 11/15/2010 05:41 PM, Avi Kivity wrote:
>>
>> I think it's a miscompile.
>>
>> out/code16.o:
>>  1a4:   3e                      ds
>>  1a5:   6c                      insb   (%dx),%es:(%edi)
>>
>> Note no 66 prefix.
>>
>
> It isn't, that was random crap.  All the insb() code is 32-bit.
>

Rewriting it to use inb / stos works (jecxz ; insb; loop doesn't) so it 
looks like a kernel bug in insb emulation.

-- 
error compiling committee.c: too many arguments to function

Re: seabios 0.6.1 regression

[Kevin O'Connor]

On Mon, Nov 15, 2010 at 06:09:45PM +0200, Avi Kivity wrote:
> On 11/15/2010 05:49 PM, Avi Kivity wrote:
> >On 11/15/2010 05:41 PM, Avi Kivity wrote:
> >>
> >>I think it's a miscompile.
> >>
> >>out/code16.o:
> >> 1a4:   3e                      ds
> >> 1a5:   6c                      insb   (%dx),%es:(%edi)
> >>
> >>Note no 66 prefix.
> >>
> >
> >It isn't, that was random crap.  All the insb() code is 32-bit.
> >
> 
> Rewriting it to use inb / stos works (jecxz ; insb; loop doesn't) so
> it looks like a kernel bug in insb emulation.

Ughh.  I can revert that change on stable-0.6.1 if needed.  It sounds
like we really do want this on the main branch though for the speed
benefit it provides.

-Kevin

Re: seabios 0.6.1 regression

[Avi Kivity]

On 11/16/2010 02:18 AM, Kevin O'Connor wrote:
> >
> >  Rewriting it to use inb / stos works (jecxz ; insb; loop doesn't) so
> >  it looks like a kernel bug in insb emulation.
>
> Ughh.  I can revert that change on stable-0.6.1 if needed.

kernel bug -> kernel fix.  SeaBIOS should feel free to use any 
documented hardware capability, esp. something as basic as rep insb 
which dates back to the good old 8086 days.

As an exception I ask that you stay away from big real mode and SMM.

> It sounds
> like we really do want this on the main branch though for the speed
> benefit it provides.

Yes.

-- 
error compiling committee.c: too many arguments to function

Re: seabios 0.6.1 regression

[Avi Kivity]

On 11/15/2010 06:09 PM, Avi Kivity wrote:
> On 11/15/2010 05:49 PM, Avi Kivity wrote:
>> On 11/15/2010 05:41 PM, Avi Kivity wrote:
>>>
>>> I think it's a miscompile.
>>>
>>> out/code16.o:
>>>  1a4:   3e                      ds
>>>  1a5:   6c                      insb   (%dx),%es:(%edi)
>>>
>>> Note no 66 prefix.
>>>
>>
>> It isn't, that was random crap.  All the insb() code is 32-bit.
>>
>
> Rewriting it to use inb / stos works (jecxz ; insb; loop doesn't) so 
> it looks like a kernel bug in insb emulation.
>

Turns out is was a subtle bug in the tpr optimization we do for Windows 
XP.  The problem happens when we load the vapic option rom from the 
firmware config interface.  With inb / movb, writing the vapic area 
happens in guest context, which the kernel is prepared to handle.  With 
insb, the write happens from kvm, which is then undone on the next 
entry, leading to the tpr being set to a high value.

I'll fix it temporarily by clearing the vapic area from the option rom.  
Later I'd like to add a proper reset to kvm tpr optimization within 
qemu-kvm (helps when we dual boot Windows XP and some other OS).

-- 
error compiling committee.c: too many arguments to function

Re: seabios 0.6.1 regression

[Alexander Graf]

On 16.11.2010, at 11:47, Avi Kivity wrote:

> On 11/15/2010 06:09 PM, Avi Kivity wrote:
>> On 11/15/2010 05:49 PM, Avi Kivity wrote:
>>> On 11/15/2010 05:41 PM, Avi Kivity wrote:
>>>> 
>>>> I think it's a miscompile.
>>>> 
>>>> out/code16.o:
>>>> 1a4:   3e                      ds
>>>> 1a5:   6c                      insb   (%dx),%es:(%edi)
>>>> 
>>>> Note no 66 prefix.
>>>> 
>>> 
>>> It isn't, that was random crap.  All the insb() code is 32-bit.
>>> 
>> 
>> Rewriting it to use inb / stos works (jecxz ; insb; loop doesn't) so it looks like a kernel bug in insb emulation.
>> 
> 
> Turns out is was a subtle bug in the tpr optimization we do for Windows XP.  The problem happens when we load the vapic option rom from the firmware config interface.  With inb / movb, writing the vapic area happens in guest context, which the kernel is prepared to handle.  With insb, the write happens from kvm, which is then undone on the next entry, leading to the tpr being set to a high value.

Shouldn't the vapic area be mapped in on demand? Then we could map it on option rom init time and everyone's happy.

Alex

Re: seabios 0.6.1 regression

[Avi Kivity]

On 11/16/2010 03:19 PM, Alexander Graf wrote:
> >>>
> >>
> >>  Rewriting it to use inb / stos works (jecxz ; insb; loop doesn't) so it looks like a kernel bug in insb emulation.
> >>
> >
> >  Turns out is was a subtle bug in the tpr optimization we do for Windows XP.  The problem happens when we load the vapic option rom from the firmware config interface.  With inb / movb, writing the vapic area happens in guest context, which the kernel is prepared to handle.  With insb, the write happens from kvm, which is then undone on the next entry, leading to the tpr being set to a high value.
>
> Shouldn't the vapic area be mapped in on demand? Then we could map it on option rom init time and everyone's happy.

Mapped in? It's an option rom.

-- 
error compiling committee.c: too many arguments to function

Re: seabios 0.6.1 regression

[Alexander Graf]

On 16.11.2010, at 15:15, Avi Kivity wrote:

> On 11/16/2010 03:19 PM, Alexander Graf wrote:
>> >>>
>> >>
>> >>  Rewriting it to use inb / stos works (jecxz ; insb; loop doesn't) so it looks like a kernel bug in insb emulation.
>> >>
>> >
>> >  Turns out is was a subtle bug in the tpr optimization we do for Windows XP.  The problem happens when we load the vapic option rom from the firmware config interface.  With inb / movb, writing the vapic area happens in guest context, which the kernel is prepared to handle.  

^^

>> With insb, the write happens from kvm, which is then undone on the next entry, leading to the tpr being set to a high value.
>> 
>> Shouldn't the vapic area be mapped in on demand? Then we could map it on option rom init time and everyone's happy.
> 
> Mapped in? It's an option rom.

According to your wording, the kernel handles writes to the vapic area. If the vapic area is only made special after initialization of the option rom, we wouldn't have this issue.


Alex

Re: seabios 0.6.1 regression

[Avi Kivity]

On 11/16/2010 04:17 PM, Alexander Graf wrote:
> On 16.11.2010, at 15:15, Avi Kivity wrote:
>
> >  On 11/16/2010 03:19 PM, Alexander Graf wrote:
> >>  >>>
> >>  >>
> >>  >>   Rewriting it to use inb / stos works (jecxz ; insb; loop doesn't) so it looks like a kernel bug in insb emulation.
> >>  >>
> >>  >
> >>  >   Turns out is was a subtle bug in the tpr optimization we do for Windows XP.  The problem happens when we load the vapic option rom from the firmware config interface.  With inb / movb, writing the vapic area happens in guest context, which the kernel is prepared to handle.
>
> ^^
>
> >>  With insb, the write happens from kvm, which is then undone on the next entry, leading to the tpr being set to a high value.
> >>
> >>  Shouldn't the vapic area be mapped in on demand? Then we could map it on option rom init time and everyone's happy.
> >
> >  Mapped in? It's an option rom.
>
> According to your wording, the kernel handles writes to the vapic area. If the vapic area is only made special after initialization of the option rom, we wouldn't have this issue.
>

Ah, yes, and I'm even now writing a reset handler to make this happen.

-- 
error compiling committee.c: too many arguments to function