[PATCH master, stable-0.12] linuxboot: fix gdt address calculation

[PATCH master, stable-0.12] linuxboot: fix gdt address calculation

[Avi Kivity]

The gdt address calculation in linuxboot.bin is broken in two ways: first
it loads %cs into %eax, but that instruction leaves the high bits of %eax
undefined and we did not clear them.  Secondly, we completely ignore the
incorrect %eax, and use the undefined %ebx instead.

With these issues fixed, linuxboot works again.

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 pc-bios/optionrom/linuxboot.S |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/pc-bios/optionrom/linuxboot.S b/pc-bios/optionrom/linuxboot.S
index c4c9109..8aebe51 100644
--- a/pc-bios/optionrom/linuxboot.S
+++ b/pc-bios/optionrom/linuxboot.S
@@ -86,9 +86,10 @@ copy_kernel:
 	/* Now create the GDT descriptor */
 	movw		$((3 * 8) - 1), -16(%bp)
 	mov		%cs, %eax
+	movzwl		%ax, %eax
 	shl		$4, %eax
-	addl		$gdt, %ebx
-	movl		%ebx, -14(%bp)
+	addl		$gdt, %eax
+	movl		%eax, -14(%bp)
 
 	/* And load the GDT */
 	data32 lgdt	-16(%bp)
-- 
1.6.5.3

Re: [PATCH master, stable-0.12] linuxboot: fix gdt address calculation

[Paolo Bonzini]

On 12/24/2009 02:38 PM, Avi Kivity wrote:
> The gdt address calculation in linuxboot.bin is broken in two ways: first
> it loads %cs into %eax, but that instruction leaves the high bits of %eax
> undefined and we did not clear them.  Secondly, we completely ignore the
> incorrect %eax, and use the undefined %ebx instead.
>
> With these issues fixed, linuxboot works again.

Wow, I wonder how it worked for me...

Paolo

Re: [Qemu-devel] [PATCH] write option roms in pc-bios/

[Anthony Liguori]

On 12/24/2009 08:27 AM, Paolo Bonzini wrote:
>>> The gdt address calculation in linuxboot.bin is broken in two ways: first
>>> it loads %cs into %eax, but that instruction leaves the high bits of %eax
>>> undefined and we did not clear them. Secondly, we completely ignore the
>>> incorrect %eax, and use the undefined %ebx instead.
>>>
>>> With these issues fixed, linuxboot works again.
>>>        
>> Wow, I wonder how it worked for me...
>>      
> Got it.  The option roms are built in pc-bios/optionrom, but QEMU loads
> the ones in pc-bios.  So actually I was always testing Alexander's code
> even though I had done a "make -B" to feel safer. :-(
>
> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com>
>    

This will wreak havoc on the tree since these are built by default and 
they are replacing files in revision control.

Regards,

Anthony Liguori

[Qemu-devel] Re: [PATCH] write option roms in pc-bios/

[Paolo Bonzini]

On 01/07/2010 09:03 PM, Anthony Liguori wrote:
>
> This will wreak havoc on the tree since these are built by default and
> they are replacing files in revision control.

... the next question is why are they (linuxboot.bin and multiboot.bin) 
in revision control since they do not require any strange tool to be built.

Paolo

Re: [Qemu-devel] Re: [PATCH] write option roms in pc-bios/

[Aurelien Jarno]

Paolo Bonzini a écrit :
> On 01/07/2010 09:03 PM, Anthony Liguori wrote:
>> This will wreak havoc on the tree since these are built by default and
>> they are replacing files in revision control.
> 
> ... the next question is why are they (linuxboot.bin and multiboot.bin) 
> in revision control since they do not require any strange tool to be built.
> 

They require an i386 cross compiler, which is not available on all
installations.

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] Re: [PATCH] write option roms in pc-bios/

[Paolo Bonzini]

On 01/08/2010 08:57 AM, Aurelien Jarno wrote:
> Paolo Bonzini a écrit :
>> On 01/07/2010 09:03 PM, Anthony Liguori wrote:
>>> This will wreak havoc on the tree since these are built by default and
>>> they are replacing files in revision control.
>>
>> ... the next question is why are they (linuxboot.bin and multiboot.bin)
>> in revision control since they do not require any strange tool to be built.
>
> They require an i386 cross compiler, which is not available on all
> installations.

They are being built always anyway now, since their target 
pc-bios/optionrom/linuxboot.bin does not exist on a checked out tree or 
even a release tree.  But no one has complained.

Paolo

Re: [Qemu-devel] Re: [PATCH] write option roms in pc-bios/

[Aurelien Jarno]

Paolo Bonzini a écrit :
> On 01/08/2010 08:57 AM, Aurelien Jarno wrote:
>> Paolo Bonzini a écrit :
>>> On 01/07/2010 09:03 PM, Anthony Liguori wrote:
>>>> This will wreak havoc on the tree since these are built by default and
>>>> they are replacing files in revision control.
>>> ... the next question is why are they (linuxboot.bin and multiboot.bin)
>>> in revision control since they do not require any strange tool to be built.
>> They require an i386 cross compiler, which is not available on all
>> installations.
> 
> They are being built always anyway now, since their target 
> pc-bios/optionrom/linuxboot.bin does not exist on a checked out tree or 
> even a release tree.  But no one has complained.

They are only built on i386 and x86_64 hosts, look at the configure
script. We should continue providing all the ROM binaries, even the x86
ones. We can't require that every installation has a (cross-)compiler
for x86, the same way that we don't require a sparc or powerpc cross
compiler for x86 installations.

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

[Qemu-devel] Re: [PATCH] write option roms in pc-bios/

[Anthony Liguori]

On 01/08/2010 01:31 AM, Paolo Bonzini wrote:
> On 01/07/2010 09:03 PM, Anthony Liguori wrote:
>>
>> This will wreak havoc on the tree since these are built by default and
>> they are replacing files in revision control.
>
> ... the next question is why are they (linuxboot.bin and 
> multiboot.bin) in revision control since they do not require any 
> strange tool to be built.

If you're just thinking about x86 and kvm, sure, but when you consider a 
default build creates tcg targets for arm, sparc, mips, etc, and that 
they all need firmware, it's extremely unlikely that any given user has 
all of the cross compilers available to build all of the roms.

Yes, we could and should be smarter about building roms when we can and 
making sure we install and use those whenever possible.  We still need 
to ship roms though for the foreseeable future.

Regards,

Anthony Liguori



> Paolo

Re: [Qemu-devel] [PATCH master, stable-0.12] linuxboot: fix gdt address calculation

[Anthony Liguori]

On 12/24/2009 07:38 AM, Avi Kivity wrote:
> The gdt address calculation in linuxboot.bin is broken in two ways: first
> it loads %cs into %eax, but that instruction leaves the high bits of %eax
> undefined and we did not clear them.  Secondly, we completely ignore the
> incorrect %eax, and use the undefined %ebx instead.
>
> With these issues fixed, linuxboot works again.
>
> Signed-off-by: Avi Kivity<avi@redhat.com>
>    

Applied.  Thanks.

Regards,

Anthony Liguori
> ---
>   pc-bios/optionrom/linuxboot.S |    5 +++--
>   1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/pc-bios/optionrom/linuxboot.S b/pc-bios/optionrom/linuxboot.S
> index c4c9109..8aebe51 100644
> --- a/pc-bios/optionrom/linuxboot.S
> +++ b/pc-bios/optionrom/linuxboot.S
> @@ -86,9 +86,10 @@ copy_kernel:
>   	/* Now create the GDT descriptor */
>   	movw		$((3 * 8) - 1), -16(%bp)
>   	mov		%cs, %eax
> +	movzwl		%ax, %eax
>   	shl		$4, %eax
> -	addl		$gdt, %ebx
> -	movl		%ebx, -14(%bp)
> +	addl		$gdt, %eax
> +	movl		%eax, -14(%bp)
>
>   	/* And load the GDT */
>   	data32 lgdt	-16(%bp)
>    

Re: [Qemu-devel] [PATCH master, stable-0.12] linuxboot: fix gdt address calculation

[Anthony Liguori]

On 12/24/2009 07:38 AM, Avi Kivity wrote:
> The gdt address calculation in linuxboot.bin is broken in two ways: first
> it loads %cs into %eax, but that instruction leaves the high bits of %eax
> undefined and we did not clear them.  Secondly, we completely ignore the
> incorrect %eax, and use the undefined %ebx instead.
>
> With these issues fixed, linuxboot works again.
>
> Signed-off-by: Avi Kivity<avi@redhat.com>
>    

Applied.  Thanks.

Regards,

Anthony Liguori
> ---
>   pc-bios/optionrom/linuxboot.S |    5 +++--
>   1 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/pc-bios/optionrom/linuxboot.S b/pc-bios/optionrom/linuxboot.S
> index c4c9109..8aebe51 100644
> --- a/pc-bios/optionrom/linuxboot.S
> +++ b/pc-bios/optionrom/linuxboot.S
> @@ -86,9 +86,10 @@ copy_kernel:
>   	/* Now create the GDT descriptor */
>   	movw		$((3 * 8) - 1), -16(%bp)
>   	mov		%cs, %eax
> +	movzwl		%ax, %eax
>   	shl		$4, %eax
> -	addl		$gdt, %ebx
> -	movl		%ebx, -14(%bp)
> +	addl		$gdt, %eax
> +	movl		%eax, -14(%bp)
>
>   	/* And load the GDT */
>   	data32 lgdt	-16(%bp)
>    

Re: [Qemu-devel] Re: [PATCH] write option roms in pc-bios/

[Paolo Bonzini]

[-- Attachment #1: Type: text/plain, Size: 1270 bytes --]

>>>>> This will wreak havoc on the tree since these are built by default and
>>>>> they are replacing files in revision control.
>>>> ... the next question is why are they (linuxboot.bin and multiboot.bin)
>>>> in revision control since they do not require any strange tool to be built.
>>> They require an i386 cross compiler, which is not available on all
>>> installations.
>> 
>> They are being built always anyway now, since their target 
>> pc-bios/optionrom/linuxboot.bin does not exist on a checked out tree or 
>> even a release tree.  But no one has complained.
>
> They are only built on i386 and x86_64 hosts, look at the configure
> script. We should continue providing all the ROM binaries, even the x86
> ones.

Oops, my bad. :-(  The only reason I see against my patch is that the option
ROMS do not change that often; maybe I felt that it should be done different
only because I screwed up and I'm blaming the code instead of myself.
There is also the problem that they are not built on Darwin and Solaris,
which is possibly even worse.

What about adding to version control a stamp file and using it to avoid
useless changes to pc-bios/*.bin?  Such as overwriting the .bin files only
when the .S file changes.  Something like the attached patch.

Paolo

[-- Attachment #2: qemu-optionrom-v2.patch --]
[-- Type: application/mbox, Size: 2518 bytes --]

Re: [Qemu-devel] Re: [PATCH] write option roms in pc-bios/

[Blue Swirl]

On Fri, Jan 8, 2010 at 10:26 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>>>>> This will wreak havoc on the tree since these are built by default and
>>>>>> they are replacing files in revision control.
>>>>> ... the next question is why are they (linuxboot.bin and multiboot.bin)
>>>>> in revision control since they do not require any strange tool to be built.
>>>> They require an i386 cross compiler, which is not available on all
>>>> installations.
>>>
>>> They are being built always anyway now, since their target
>>> pc-bios/optionrom/linuxboot.bin does not exist on a checked out tree or
>>> even a release tree.  But no one has complained.
>>
>> They are only built on i386 and x86_64 hosts, look at the configure
>> script. We should continue providing all the ROM binaries, even the x86
>> ones.
>
> Oops, my bad. :-(  The only reason I see against my patch is that the option
> ROMS do not change that often; maybe I felt that it should be done different
> only because I screwed up and I'm blaming the code instead of myself.
> There is also the problem that they are not built on Darwin and Solaris,
> which is possibly even worse.
>
> What about adding to version control a stamp file and using it to avoid
> useless changes to pc-bios/*.bin?  Such as overwriting the .bin files only
> when the .S file changes.  Something like the attached patch.

configure could also detect that we are building inside the source
tree (not in an object tree outside of the source directory) and not
compile the roms in that case. Or we could make object tree use
mandatory, it's a good practice anyway.