Patchwork Fix geode lx VSA loading

login
register
about
Submitter Stefan Reinauer
Date 2010-06-04 16:40:31
Message ID <4C092C7F.5030302@coresystems.de>
Download mbox | patch
Permalink /patch/1466/
State Accepted
Headers show

Comments

Stefan Reinauer - 2010-06-04 16:40:31
On 6/4/10 6:37 PM, Patrick Georgi wrote:
> Am Freitag, den 04.06.2010, 16:25 +0100 schrieb Edwin Beasant:
>   
>> This patch fixes the option rom code that was buggy when it switched
>> segment registers before restoring register values. This was breaking
>> the Geode VSA, and probably would have hurt other option roms as well.
>>
>>  
>>
>> Signed-off by: Edwin Beasant edwin_beasant@virtensys.com
>>     
> Except for the first two parts (the magic values in ecx/edx and
> whitespace), and assuming this is tested, the patch is
>
> Acked-by: Patrick Georgi <patrick.georgi@coresystems.de>
>
>
>   

I did a new version of the patch with those issues fixed (and pushing
one register instead of all), see attachment

Signed-off-by: Edwin Beasant <edwin_beasant@virtensys.com>
Signed-off-by: Stefan Reinauer <stepan@coresystems.de>
Myles Watson - 2010-06-04 16:53:29
I don't see where ax gets set to 0.  Does the comment need to be updated, or
do you want all the segments set to ax?

+	/* initialize registers for option rom lcall */
+	movl	__registers +  0, %eax
+	movl	__registers +  4, %ebx
+	movl	__registers +  8, %ecx
+	movl	__registers + 12, %edx
+	movl	__registers + 16, %esi
+	movl	__registers + 20, %edi	
+
 	/* Set all segments to 0x0000, ds to 0x0040 */
+	push	%ax
 	mov	%ax, %es
 	mov	%ax, %fs
 	mov	%ax, %gs
 	mov	$0x40, %ax
 	mov	%ax, %ds
+	pop	%ax

Maybe you want a

mov $0, %ax

right after the push %ax?

Thanks,
Myles
ron minnich - 2010-06-06 04:50:12
On Fri, Jun 4, 2010 at 9:53 AM, Myles Watson <mylesgw@gmail.com> wrote:

> mov $0, %ax

use the classic:
xorl %ax, %ax

ron
Edwin Beasant - 2010-06-07 08:30:27
Agreed - ax should be 0, it is only 0 in this case due to the argument passed in!
Before the segment setup, but after the _registers section, an xorl %ax, %al would be appropriate.
Thanks!
Edwin

-----Original Message-----
From: coreboot-bounces+edwin_beasant=virtensys.com@coreboot.org [mailto:coreboot-bounces+edwin_beasant=virtensys.com@coreboot.org] On Behalf Of ron minnich
Sent: 06 June 2010 05:50
To: Myles Watson
Cc: Stefan Reinauer; coreboot@coreboot.org
Subject: Re: [coreboot] [PATCH] Fix geode lx VSA loading

On Fri, Jun 4, 2010 at 9:53 AM, Myles Watson <mylesgw@gmail.com> wrote:

> mov $0, %ax

use the classic:
xorl %ax, %ax

ron

--
coreboot mailing list: coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot
Stefan Reinauer - 2010-06-07 08:38:02
On 6/7/10 10:30 AM, Edwin Beasant wrote:
> Agreed - ax should be 0, it is only 0 in this case due to the argument passed in!
> Before the segment setup, but after the _registers section, an xorl %ax, %al would be appropriate.
> Thanks!
> Edwin
>
>   

Did I miss one?

Stefan

> -----Original Message-----
> From: coreboot-bounces+edwin_beasant=virtensys.com@coreboot.org [mailto:coreboot-bounces+edwin_beasant=virtensys.com@coreboot.org] On Behalf Of ron minnich
> Sent: 06 June 2010 05:50
> To: Myles Watson
> Cc: Stefan Reinauer; coreboot@coreboot.org
> Subject: Re: [coreboot] [PATCH] Fix geode lx VSA loading
>
> On Fri, Jun 4, 2010 at 9:53 AM, Myles Watson <mylesgw@gmail.com> wrote:
>
>   
>> mov $0, %ax
>>     
> use the classic:
> xorl %ax, %ax
>
> ron
>
> --
> coreboot mailing list: coreboot@coreboot.org
> http://www.coreboot.org/mailman/listinfo/coreboot
>
>
Edwin Beasant - 2010-06-07 08:57:28
Nope - I was just behind with my subversion revisions (merge problems on update, that will teach me...!)
Just seen the patch as finalised, thanks for that, looks good and should fix the issues seen.

I was going to refine it today, but you're way ahead of me :-)
Thanks,
Edwin

-----Original Message-----
From: coreboot-bounces@coreboot.org [mailto:coreboot-bounces@coreboot.org] On Behalf Of Stefan Reinauer
Sent: 07 June 2010 09:38
To: coreboot@coreboot.org
Subject: Re: [coreboot] [PATCH] Fix geode lx VSA loading

On 6/7/10 10:30 AM, Edwin Beasant wrote:
> Agreed - ax should be 0, it is only 0 in this case due to the argument passed in!
> Before the segment setup, but after the _registers section, an xorl %ax, %al would be appropriate.
> Thanks!
> Edwin
>
>

Did I miss one?

Stefan

> -----Original Message-----
> From: coreboot-bounces+edwin_beasant=virtensys.com@coreboot.org [mailto:coreboot-bounces+edwin_beasant=virtensys.com@coreboot.org] On Behalf Of ron minnich
> Sent: 06 June 2010 05:50
> To: Myles Watson
> Cc: Stefan Reinauer; coreboot@coreboot.org
> Subject: Re: [coreboot] [PATCH] Fix geode lx VSA loading
>
> On Fri, Jun 4, 2010 at 9:53 AM, Myles Watson <mylesgw@gmail.com> wrote:
>
>
>> mov $0, %ax
>>
> use the classic:
> xorl %ax, %ax
>
> ron
>
> --
> coreboot mailing list: coreboot@coreboot.org
> http://www.coreboot.org/mailman/listinfo/coreboot
>
>


--
coreboot mailing list: coreboot@coreboot.org
http://www.coreboot.org/mailman/listinfo/coreboot

Patch

Index: src/devices/oprom/x86_asm.S
===================================================================
--- src/devices/oprom/x86_asm.S	(revision 5608)
+++ src/devices/oprom/x86_asm.S	(working copy)
@@ -141,21 +141,23 @@ 
 	mov	%ax, %ds
 	lidt	__realmode_idt
 
+	/* initialize registers for option rom lcall */
+	movl	__registers +  0, %eax
+	movl	__registers +  4, %ebx
+	movl	__registers +  8, %ecx
+	movl	__registers + 12, %edx
+	movl	__registers + 16, %esi
+	movl	__registers + 20, %edi	
+
 	/* Set all segments to 0x0000, ds to 0x0040 */
+	push	%ax
 	mov	%ax, %es
 	mov	%ax, %fs
 	mov	%ax, %gs
 	mov	$0x40, %ax
 	mov	%ax, %ds
+	pop	%ax
 
-	/* initialize registers for option rom lcall */
-	movl	__registers +  0, %eax
-	movl	__registers +  4, %ebx
-	movl	__registers +  8, %ecx
-	movl	__registers + 12, %edx
-	movl	__registers + 16, %esi
-	movl	__registers + 20, %edi
-
 	/* ************************************ */
 __lcall_instr = RELOCATED(.)
 	.byte 0x9a
@@ -262,12 +264,6 @@ 
 	mov	%ax, %ds
 	lidt	__realmode_idt
 
-	/* Set all segments to 0x0000 */
-	mov	%ax, %ds
-	mov	%ax, %es
-	mov	%ax, %fs
-	mov	%ax, %gs
-
 	/* initialize registers for intXX call */
 	movl	__registers +  0, %eax
 	movl	__registers +  4, %ebx
@@ -276,6 +272,14 @@ 
 	movl	__registers + 16, %esi
 	movl	__registers + 20, %edi
 
+	/* Set all segments to 0x0000 */
+	push	%ax
+	mov	%ax, %ds
+	mov	%ax, %es
+	mov	%ax, %fs
+	mov	%ax, %gs
+	pop	%ax
+
 __intXX_instr = RELOCATED(.)
 	.byte 0xcd, 0x00 /* This becomes intXX */
 
@@ -376,7 +380,7 @@ 
 	mov	%ax, %ds
 	lidt	__realmode_idt
 
-	/* Set up segment registers to segment 0x0000 */
+	/* Set up segment registers to segment 0x0000 and ds to 0x040 */
 	mov	%ax, %es
 	mov	%ax, %fs
 	mov	%ax, %gs