diff mbox series

[3/3] x86: a little bit of 16-bit video mode setting code cleanup

Message ID 5D0387330200007800238476@prv1-mh.provo.novell.com (mailing list archive)
State New, archived
Headers show
Series x86: S3 resume adjustments | expand

Commit Message

Jan Beulich June 14, 2019, 11:38 a.m. UTC
To "compensate" for the code size growth by an earlier change:
- drop "trampoline" labels (in almost all cases the target label is
  reachable with an 8-bit-displacement branch anyway, and a single 16-
  bit-displacement branch is still better than a pair of two branches)
- drop an entirely dead insn
- reduce code size in a few other (obvious I hope) cases, by more
  suitable insn/operands selection

Also drop redundant #define-s (move suitable #include a little earlier
instead) and add two alignment directives.

Signed-off-by: Jan Beulich <jbeulich@suse.com>

Comments

Andrew Cooper Aug. 29, 2019, 2:08 p.m. UTC | #1
On 14/06/2019 12:38, Jan Beulich wrote:
> To "compensate" for the code size growth by an earlier change:
> - drop "trampoline" labels (in almost all cases the target label is
>   reachable with an 8-bit-displacement branch anyway, and a single 16-
>   bit-displacement branch is still better than a pair of two branches)

Do you happen to know why we any to start with?  It can't plausibly be
for manual code alignment reasons.

(probably) whatever the reason, good riddance.

> - drop an entirely dead insn
> - reduce code size in a few other (obvious I hope) cases, by more
>   suitable insn/operands selection

I'm afraid these are rather hard to identify, given no hints.  Comments
in line.

> Also drop redundant #define-s (move suitable #include a little earlier
> instead) and add two alignment directives.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/arch/x86/boot/trampoline.S
> +++ b/xen/arch/x86/boot/trampoline.S
> @@ -176,6 +176,7 @@ start64:
>  
>          jmpq    *%rdi
>  
> +#include "video.h"
>  #include "wakeup.S"
>  
>          .balign 8
> @@ -283,8 +284,6 @@ trampoline_boot_cpu_entry:
>          /* Jump to the common bootstrap entry point. */
>          jmp     trampoline_protmode_entry
>  
> -#include "video.h"
> -
>          .align  2
>  /* Keep in sync with cmdline.c:early_boot_opts_t type! */
>  early_boot_opts:
> --- a/xen/arch/x86/boot/video.S
> +++ b/xen/arch/x86/boot/video.S
> @@ -384,9 +384,6 @@ lmbad:  leaw    bootsym(unknt), %si
>          jmp     mode_menu
>  lmdef:  ret
>  
> -_setrec:    jmp setrec                  # Ugly...
> -_set_80x25: jmp set_80x25
> -
>  # Setting of user mode (AX=mode ID) => CF=success
>  mode_set:
>          movw    %ax, bootsym(boot_vid_mode)
> @@ -396,7 +393,7 @@ mode_set:
>          je      setvesabysize
>  
>          testb   $VIDEO_RECALC>>8, %ah
> -        jnz     _setrec
> +        jnz     setrec
>  
>          cmpb    $VIDEO_FIRST_SPECIAL>>8, %ah
>          jz      setspc
> @@ -421,7 +418,7 @@ setspc: xorb    %bh, %bh
>  
>  setmenu:
>          orb     %al, %al                # 80x25 is an exception
> -        jz      _set_80x25
> +        jz      set_80x25
>          
>          pushw   %bx                     # Set mode chosen from menu
>          call    mode_table              # Build the mode table
> @@ -441,36 +438,32 @@ check_vesa:
>          cmpw    $0x004f, %ax
>          jnz     setbad
>  
> -        leaw    vesa_mode_info, %di
> -        subb    $VIDEO_FIRST_VESA>>8, %bh
> -        movw    %bx, %cx                # Get mode information structure
> +        leaw    vesa_mode_info, %di     # Get mode information structure
> +        leaw    -VIDEO_FIRST_VESA(%bx), %cx
>          movw    $0x4f01, %ax
>          int     $0x10
> -        addb    $VIDEO_FIRST_VESA>>8, %bh

Is this the redundant instruction you are talking about, or ... (near
the end)?

I think I follow this as "no logical change", by leaving %bx intact
throughout, and only clearing %ch as part of the %bx=>%cx copy.

>          cmpw    $0x004f, %ax
>          jnz     setbad
>  
>          movb    (%di), %al              # Check mode attributes.
>          andb    $0x99, %al
>          cmpb    $0x99, %al
> -        jnz     _setbad                 # Doh! No linear frame buffer.
> +        jnz     setbad                  # Doh! No linear frame buffer.
>  
>          pushw   %bx
>          subb    $VIDEO_FIRST_VESA>>8, %bh
> -        orw     $0x4000, %bx            # Use linear frame buffer
> +        orb     $0x40, %bh              # Use linear frame buffer
>          movw    $0x4f02, %ax            # VESA BIOS mode set call
>          int     $0x10
>          popw    %bx
>          cmpw    $0x004f, %ax            # AL=4f if implemented
> -        jnz     _setbad                 # AH=0 if OK
> +        jnz     setbad                  # AH=0 if OK
>  
>          movb    $1, bootsym(graphic_mode)  # flag graphic mode
>          movw    %bx, bootsym(video_mode)
>          stc
>          ret
>  
> -_setbad: jmp    setbad                  # Ugly...
> -
>  # Recalculate vertical display end registers -- this fixes various
>  # inconsistencies of extended modes on many adapters. Called when
>  # the VIDEO_RECALC flag is set in the mode ID.
> @@ -515,7 +508,7 @@ setvesabysize:
>          leaw    modelist,%si
>  1:      add     $8,%si
>          cmpw    $ASK_VGA,-8(%si)        # End?
> -        je      _setbad
> +        je      setbad
>          movw    -6(%si),%ax
>          cmpw    %ax,bootsym(vesa_size)+0
>          jne     1b
> @@ -948,6 +941,7 @@ store_edid:
>  #endif
>          ret
>  
> +                .p2align 1
>  mt_end:         .word   0       # End of video mode table if built
>  edit_buf:       .space  6       # Line editor buffer
>  card_name:      .word   0       # Pointer to adapter name
> @@ -991,6 +985,7 @@ vesa_name:      .asciz  "VESA"
>  
>  name_bann:      .asciz  "Video adapter: "
>  
> +                .p2align 1
>  force_size:     .word   0       # Use this size instead of the one in BIOS vars
>  
>  GLOBAL(boot_vid_info)
> --- a/xen/arch/x86/boot/wakeup.S
> +++ b/xen/arch/x86/boot/wakeup.S
> @@ -30,7 +30,7 @@ ENTRY(wakeup_start)
>          jne     bogus_real_magic
>  
>          # for acpi_sleep=s3_bios
> -        testl   $1, wakesym(video_flags)
> +        testb   $1, wakesym(video_flags)

video_flags is currently .long, and, AFAICT, uses 2 bits even after this
series.  We could get better code reduction by shrinking it to .byte.

>          jz      1f
>          lcall   $0xc000, $3
>          movw    %cs, %ax        # In case messed by BIOS
> @@ -38,9 +38,9 @@ ENTRY(wakeup_start)
>          movw    %ax, %ss        # Need this? How to ret if clobbered?
>  
>  1:      # for acpi_sleep=s3_mode
> -        testl   $2, wakesym(video_flags)
> +        testb   $2, wakesym(video_flags)
>          jz      1f
> -        movl    wakesym(video_mode), %eax
> +        movw    wakesym(video_mode), %ax

Similarly, video_mode can become .word, I think.

>          call    mode_setw
>  
>  1:      # Show some progress if VGA is resumed
> @@ -55,48 +55,26 @@ ENTRY(wakeup_start)
>          lmsw    %ax             # Turn on CR0.PE 
>          ljmpl   $BOOT_CS32, $bootsym_rel(wakeup_32, 6)
>  
> -/* This code uses an extended set of video mode numbers. These include:
> - * Aliases for standard modes
> - *      NORMAL_VGA (-1)
> - *      EXTENDED_VGA (-2)
> - *      ASK_VGA (-3)
> - * Video modes numbered by menu position -- NOT RECOMMENDED because of lack
> - * of compatibility when extending the table. These are between 0x00 and 0xff.
> - */
> -#define VIDEO_FIRST_MENU 0x0000
> -
> -/* Standard BIOS video modes (BIOS number + 0x0100) */
> -#define VIDEO_FIRST_BIOS 0x0100
> -
> -/* VESA BIOS video modes (VESA number + 0x0200) */
> -#define VIDEO_FIRST_VESA 0x0200
> -
> -/* Video7 special modes (BIOS number + 0x0900) */
> -#define VIDEO_FIRST_V7 0x0900
> -
>  # Setting of user mode (AX=mode ID) => CF=success
>  mode_setw:
>          movw    %ax, %bx
>          cmpb    $VIDEO_FIRST_VESA>>8, %ah
>          jnc     check_vesaw
> -        decb    %ah

... or is this the no functional change?  If so, I'm not sure I agree,
given the clc below.

~Andrew

>  
>  setbadw: clc
>          ret
>  
>  check_vesaw:
>          subb    $VIDEO_FIRST_VESA>>8, %bh
> -        orw     $0x4000, %bx                    # Use linear frame buffer
> +        orb     $0x40, %bh                      # Use linear frame buffer
>          movw    $0x4f02, %ax                    # VESA BIOS mode set call
>          int     $0x10
>          cmpw    $0x004f, %ax                    # AL=4f if implemented
> -        jnz     _setbadw                        # AH=0 if OK
> +        jnz     setbadw                         # AH=0 if OK
>  
>          stc
>          ret
>  
> -_setbadw: jmp    setbadw
> -
>  bogus_real_magic:
>          movw    $0x0e00 + 'B', %fs:(0x12)
>          jmp     bogus_real_magic
>
>
>
Jan Beulich Aug. 29, 2019, 2:23 p.m. UTC | #2
On 29.08.2019 16:08, Andrew Cooper wrote:
> On 14/06/2019 12:38, Jan Beulich wrote:
>> To "compensate" for the code size growth by an earlier change:
>> - drop "trampoline" labels (in almost all cases the target label is
>>   reachable with an 8-bit-displacement branch anyway, and a single 16-
>>   bit-displacement branch is still better than a pair of two branches)
> 
> Do you happen to know why we any to start with?  It can't plausibly be
> for manual code alignment reasons.

I have no idea - my guess is that some pre-386 code was cloned, or
it was written by someone used to the pre-386 limitations.

>> @@ -421,7 +418,7 @@ setspc: xorb    %bh, %bh
>>  
>>  setmenu:
>>          orb     %al, %al                # 80x25 is an exception
>> -        jz      _set_80x25
>> +        jz      set_80x25
>>          
>>          pushw   %bx                     # Set mode chosen from menu
>>          call    mode_table              # Build the mode table
>> @@ -441,36 +438,32 @@ check_vesa:
>>          cmpw    $0x004f, %ax
>>          jnz     setbad
>>  
>> -        leaw    vesa_mode_info, %di
>> -        subb    $VIDEO_FIRST_VESA>>8, %bh
>> -        movw    %bx, %cx                # Get mode information structure
>> +        leaw    vesa_mode_info, %di     # Get mode information structure
>> +        leaw    -VIDEO_FIRST_VESA(%bx), %cx
>>          movw    $0x4f01, %ax
>>          int     $0x10
>> -        addb    $VIDEO_FIRST_VESA>>8, %bh
> 
> Is this the redundant instruction you are talking about, or ... (near
> the end)?

No, here it's simply ...

> I think I follow this as "no logical change", by leaving %bx intact
> throughout, and only clearing %ch as part of the %bx=>%cx copy.

... as you say.

>> --- a/xen/arch/x86/boot/wakeup.S
>> +++ b/xen/arch/x86/boot/wakeup.S
>> @@ -30,7 +30,7 @@ ENTRY(wakeup_start)
>>          jne     bogus_real_magic
>>  
>>          # for acpi_sleep=s3_bios
>> -        testl   $1, wakesym(video_flags)
>> +        testb   $1, wakesym(video_flags)
> 
> video_flags is currently .long, and, AFAICT, uses 2 bits even after this
> series.  We could get better code reduction by shrinking it to .byte.

Well, the goal of this patch is to play with the assembly code. To
do what you suggest I'd have to touch a C (or really .h) file as
well. I'm fine doing this change though, but preferably in a separate
patch.

>> @@ -38,9 +38,9 @@ ENTRY(wakeup_start)
>>          movw    %ax, %ss        # Need this? How to ret if clobbered?
>>  
>>  1:      # for acpi_sleep=s3_mode
>> -        testl   $2, wakesym(video_flags)
>> +        testb   $2, wakesym(video_flags)
>>          jz      1f
>> -        movl    wakesym(video_mode), %eax
>> +        movw    wakesym(video_mode), %ax
> 
> Similarly, video_mode can become .word, I think.

But a word is less efficient to access (because of the operand size
override), so I'd prefer to not shrink this one. Just let me know
whether you agree, and I'll cook up a patch accordingly.

>> @@ -55,48 +55,26 @@ ENTRY(wakeup_start)
>>          lmsw    %ax             # Turn on CR0.PE 
>>          ljmpl   $BOOT_CS32, $bootsym_rel(wakeup_32, 6)
>>  
>> -/* This code uses an extended set of video mode numbers. These include:
>> - * Aliases for standard modes
>> - *      NORMAL_VGA (-1)
>> - *      EXTENDED_VGA (-2)
>> - *      ASK_VGA (-3)
>> - * Video modes numbered by menu position -- NOT RECOMMENDED because of lack
>> - * of compatibility when extending the table. These are between 0x00 and 0xff.
>> - */
>> -#define VIDEO_FIRST_MENU 0x0000
>> -
>> -/* Standard BIOS video modes (BIOS number + 0x0100) */
>> -#define VIDEO_FIRST_BIOS 0x0100
>> -
>> -/* VESA BIOS video modes (VESA number + 0x0200) */
>> -#define VIDEO_FIRST_VESA 0x0200
>> -
>> -/* Video7 special modes (BIOS number + 0x0900) */
>> -#define VIDEO_FIRST_V7 0x0900
>> -
>>  # Setting of user mode (AX=mode ID) => CF=success
>>  mode_setw:
>>          movw    %ax, %bx
>>          cmpb    $VIDEO_FIRST_VESA>>8, %ah
>>          jnc     check_vesaw
>> -        decb    %ah
> 
> ... or is this the no functional change?  If so, I'm not sure I agree,
> given the clc below.

How does the CLC matter? CF, as the comment says, indicates success.
Whether or not there's a DEC ahead of it (which doesn't even alter
CF) doesn't matter, does it?

In any event - yes, that's the dead insn. I'll mention the function
name in the description.

Jan

>>  setbadw: clc
>>          ret
Andrew Cooper Aug. 29, 2019, 2:38 p.m. UTC | #3
On 29/08/2019 15:23, Jan Beulich wrote:
> On 29.08.2019 16:08, Andrew Cooper wrote:
>> On 14/06/2019 12:38, Jan Beulich wrote:
>>> To "compensate" for the code size growth by an earlier change:
>>> - drop "trampoline" labels (in almost all cases the target label is
>>>   reachable with an 8-bit-displacement branch anyway, and a single 16-
>>>   bit-displacement branch is still better than a pair of two branches)
>> Do you happen to know why we any to start with?  It can't plausibly be
>> for manual code alignment reasons.
> I have no idea - my guess is that some pre-386 code was cloned, or
> it was written by someone used to the pre-386 limitations.
>
>>> @@ -421,7 +418,7 @@ setspc: xorb    %bh, %bh
>>>  
>>>  setmenu:
>>>          orb     %al, %al                # 80x25 is an exception
>>> -        jz      _set_80x25
>>> +        jz      set_80x25
>>>          
>>>          pushw   %bx                     # Set mode chosen from menu
>>>          call    mode_table              # Build the mode table
>>> @@ -441,36 +438,32 @@ check_vesa:
>>>          cmpw    $0x004f, %ax
>>>          jnz     setbad
>>>  
>>> -        leaw    vesa_mode_info, %di
>>> -        subb    $VIDEO_FIRST_VESA>>8, %bh
>>> -        movw    %bx, %cx                # Get mode information structure
>>> +        leaw    vesa_mode_info, %di     # Get mode information structure
>>> +        leaw    -VIDEO_FIRST_VESA(%bx), %cx
>>>          movw    $0x4f01, %ax
>>>          int     $0x10
>>> -        addb    $VIDEO_FIRST_VESA>>8, %bh
>> Is this the redundant instruction you are talking about, or ... (near
>> the end)?
> No, here it's simply ...
>
>> I think I follow this as "no logical change", by leaving %bx intact
>> throughout, and only clearing %ch as part of the %bx=>%cx copy.
> ... as you say.
>
>>> --- a/xen/arch/x86/boot/wakeup.S
>>> +++ b/xen/arch/x86/boot/wakeup.S
>>> @@ -30,7 +30,7 @@ ENTRY(wakeup_start)
>>>          jne     bogus_real_magic
>>>  
>>>          # for acpi_sleep=s3_bios
>>> -        testl   $1, wakesym(video_flags)
>>> +        testb   $1, wakesym(video_flags)
>> video_flags is currently .long, and, AFAICT, uses 2 bits even after this
>> series.  We could get better code reduction by shrinking it to .byte.
> Well, the goal of this patch is to play with the assembly code. To
> do what you suggest I'd have to touch a C (or really .h) file as
> well. I'm fine doing this change though, but preferably in a separate
> patch.

This is fine, and may indeed want to wait until David has finished
trampoline work.

>
>>> @@ -38,9 +38,9 @@ ENTRY(wakeup_start)
>>>          movw    %ax, %ss        # Need this? How to ret if clobbered?
>>>  
>>>  1:      # for acpi_sleep=s3_mode
>>> -        testl   $2, wakesym(video_flags)
>>> +        testb   $2, wakesym(video_flags)
>>>          jz      1f
>>> -        movl    wakesym(video_mode), %eax
>>> +        movw    wakesym(video_mode), %ax
>> Similarly, video_mode can become .word, I think.
> But a word is less efficient to access (because of the operand size
> override), so I'd prefer to not shrink this one. Just let me know
> whether you agree, and I'll cook up a patch accordingly.

This is 16 bit code so it is movl which has the operand size prefix, not
movw.

It is extern'd in C, but not wrapped in bootsym(), and I can't see it
being read anywhere.

>
>>> @@ -55,48 +55,26 @@ ENTRY(wakeup_start)
>>>          lmsw    %ax             # Turn on CR0.PE 
>>>          ljmpl   $BOOT_CS32, $bootsym_rel(wakeup_32, 6)
>>>  
>>> -/* This code uses an extended set of video mode numbers. These include:
>>> - * Aliases for standard modes
>>> - *      NORMAL_VGA (-1)
>>> - *      EXTENDED_VGA (-2)
>>> - *      ASK_VGA (-3)
>>> - * Video modes numbered by menu position -- NOT RECOMMENDED because of lack
>>> - * of compatibility when extending the table. These are between 0x00 and 0xff.
>>> - */
>>> -#define VIDEO_FIRST_MENU 0x0000
>>> -
>>> -/* Standard BIOS video modes (BIOS number + 0x0100) */
>>> -#define VIDEO_FIRST_BIOS 0x0100
>>> -
>>> -/* VESA BIOS video modes (VESA number + 0x0200) */
>>> -#define VIDEO_FIRST_VESA 0x0200
>>> -
>>> -/* Video7 special modes (BIOS number + 0x0900) */
>>> -#define VIDEO_FIRST_V7 0x0900
>>> -
>>>  # Setting of user mode (AX=mode ID) => CF=success
>>>  mode_setw:
>>>          movw    %ax, %bx
>>>          cmpb    $VIDEO_FIRST_VESA>>8, %ah
>>>          jnc     check_vesaw
>>> -        decb    %ah
>> ... or is this the no functional change?  If so, I'm not sure I agree,
>> given the clc below.
> How does the CLC matter? CF, as the comment says, indicates success.
> Whether or not there's a DEC ahead of it (which doesn't even alter
> CF) doesn't matter, does it?

I'd forgotten that dec left CF intact, and was thinking more like sub
$1, where it would matter.

> In any event - yes, that's the dead insn. I'll mention the function
> name in the description.

Please do.

~Andrew
Jan Beulich Aug. 29, 2019, 3:07 p.m. UTC | #4
On 29.08.2019 16:38, Andrew Cooper wrote:
> On 29/08/2019 15:23, Jan Beulich wrote:
>> On 29.08.2019 16:08, Andrew Cooper wrote:
>>> On 14/06/2019 12:38, Jan Beulich wrote:
>>>> @@ -38,9 +38,9 @@ ENTRY(wakeup_start)
>>>>          movw    %ax, %ss        # Need this? How to ret if clobbered?
>>>>  
>>>>  1:      # for acpi_sleep=s3_mode
>>>> -        testl   $2, wakesym(video_flags)
>>>> +        testb   $2, wakesym(video_flags)
>>>>          jz      1f
>>>> -        movl    wakesym(video_mode), %eax
>>>> +        movw    wakesym(video_mode), %ax
>>> Similarly, video_mode can become .word, I think.
>> But a word is less efficient to access (because of the operand size
>> override), so I'd prefer to not shrink this one. Just let me know
>> whether you agree, and I'll cook up a patch accordingly.
> 
> This is 16 bit code so it is movl which has the operand size prefix, not
> movw.
> 
> It is extern'd in C, but not wrapped in bootsym(), and I can't see it
> being read anywhere.

Oh, indeed. I'll ditch the extern then.

Jan
diff mbox series

Patch

--- a/xen/arch/x86/boot/trampoline.S
+++ b/xen/arch/x86/boot/trampoline.S
@@ -176,6 +176,7 @@  start64:
 
         jmpq    *%rdi
 
+#include "video.h"
 #include "wakeup.S"
 
         .balign 8
@@ -283,8 +284,6 @@  trampoline_boot_cpu_entry:
         /* Jump to the common bootstrap entry point. */
         jmp     trampoline_protmode_entry
 
-#include "video.h"
-
         .align  2
 /* Keep in sync with cmdline.c:early_boot_opts_t type! */
 early_boot_opts:
--- a/xen/arch/x86/boot/video.S
+++ b/xen/arch/x86/boot/video.S
@@ -384,9 +384,6 @@  lmbad:  leaw    bootsym(unknt), %si
         jmp     mode_menu
 lmdef:  ret
 
-_setrec:    jmp setrec                  # Ugly...
-_set_80x25: jmp set_80x25
-
 # Setting of user mode (AX=mode ID) => CF=success
 mode_set:
         movw    %ax, bootsym(boot_vid_mode)
@@ -396,7 +393,7 @@  mode_set:
         je      setvesabysize
 
         testb   $VIDEO_RECALC>>8, %ah
-        jnz     _setrec
+        jnz     setrec
 
         cmpb    $VIDEO_FIRST_SPECIAL>>8, %ah
         jz      setspc
@@ -421,7 +418,7 @@  setspc: xorb    %bh, %bh
 
 setmenu:
         orb     %al, %al                # 80x25 is an exception
-        jz      _set_80x25
+        jz      set_80x25
         
         pushw   %bx                     # Set mode chosen from menu
         call    mode_table              # Build the mode table
@@ -441,36 +438,32 @@  check_vesa:
         cmpw    $0x004f, %ax
         jnz     setbad
 
-        leaw    vesa_mode_info, %di
-        subb    $VIDEO_FIRST_VESA>>8, %bh
-        movw    %bx, %cx                # Get mode information structure
+        leaw    vesa_mode_info, %di     # Get mode information structure
+        leaw    -VIDEO_FIRST_VESA(%bx), %cx
         movw    $0x4f01, %ax
         int     $0x10
-        addb    $VIDEO_FIRST_VESA>>8, %bh
         cmpw    $0x004f, %ax
         jnz     setbad
 
         movb    (%di), %al              # Check mode attributes.
         andb    $0x99, %al
         cmpb    $0x99, %al
-        jnz     _setbad                 # Doh! No linear frame buffer.
+        jnz     setbad                  # Doh! No linear frame buffer.
 
         pushw   %bx
         subb    $VIDEO_FIRST_VESA>>8, %bh
-        orw     $0x4000, %bx            # Use linear frame buffer
+        orb     $0x40, %bh              # Use linear frame buffer
         movw    $0x4f02, %ax            # VESA BIOS mode set call
         int     $0x10
         popw    %bx
         cmpw    $0x004f, %ax            # AL=4f if implemented
-        jnz     _setbad                 # AH=0 if OK
+        jnz     setbad                  # AH=0 if OK
 
         movb    $1, bootsym(graphic_mode)  # flag graphic mode
         movw    %bx, bootsym(video_mode)
         stc
         ret
 
-_setbad: jmp    setbad                  # Ugly...
-
 # Recalculate vertical display end registers -- this fixes various
 # inconsistencies of extended modes on many adapters. Called when
 # the VIDEO_RECALC flag is set in the mode ID.
@@ -515,7 +508,7 @@  setvesabysize:
         leaw    modelist,%si
 1:      add     $8,%si
         cmpw    $ASK_VGA,-8(%si)        # End?
-        je      _setbad
+        je      setbad
         movw    -6(%si),%ax
         cmpw    %ax,bootsym(vesa_size)+0
         jne     1b
@@ -948,6 +941,7 @@  store_edid:
 #endif
         ret
 
+                .p2align 1
 mt_end:         .word   0       # End of video mode table if built
 edit_buf:       .space  6       # Line editor buffer
 card_name:      .word   0       # Pointer to adapter name
@@ -991,6 +985,7 @@  vesa_name:      .asciz  "VESA"
 
 name_bann:      .asciz  "Video adapter: "
 
+                .p2align 1
 force_size:     .word   0       # Use this size instead of the one in BIOS vars
 
 GLOBAL(boot_vid_info)
--- a/xen/arch/x86/boot/wakeup.S
+++ b/xen/arch/x86/boot/wakeup.S
@@ -30,7 +30,7 @@  ENTRY(wakeup_start)
         jne     bogus_real_magic
 
         # for acpi_sleep=s3_bios
-        testl   $1, wakesym(video_flags)
+        testb   $1, wakesym(video_flags)
         jz      1f
         lcall   $0xc000, $3
         movw    %cs, %ax        # In case messed by BIOS
@@ -38,9 +38,9 @@  ENTRY(wakeup_start)
         movw    %ax, %ss        # Need this? How to ret if clobbered?
 
 1:      # for acpi_sleep=s3_mode
-        testl   $2, wakesym(video_flags)
+        testb   $2, wakesym(video_flags)
         jz      1f
-        movl    wakesym(video_mode), %eax
+        movw    wakesym(video_mode), %ax
         call    mode_setw
 
 1:      # Show some progress if VGA is resumed
@@ -55,48 +55,26 @@  ENTRY(wakeup_start)
         lmsw    %ax             # Turn on CR0.PE 
         ljmpl   $BOOT_CS32, $bootsym_rel(wakeup_32, 6)
 
-/* This code uses an extended set of video mode numbers. These include:
- * Aliases for standard modes
- *      NORMAL_VGA (-1)
- *      EXTENDED_VGA (-2)
- *      ASK_VGA (-3)
- * Video modes numbered by menu position -- NOT RECOMMENDED because of lack
- * of compatibility when extending the table. These are between 0x00 and 0xff.
- */
-#define VIDEO_FIRST_MENU 0x0000
-
-/* Standard BIOS video modes (BIOS number + 0x0100) */
-#define VIDEO_FIRST_BIOS 0x0100
-
-/* VESA BIOS video modes (VESA number + 0x0200) */
-#define VIDEO_FIRST_VESA 0x0200
-
-/* Video7 special modes (BIOS number + 0x0900) */
-#define VIDEO_FIRST_V7 0x0900
-
 # Setting of user mode (AX=mode ID) => CF=success
 mode_setw:
         movw    %ax, %bx
         cmpb    $VIDEO_FIRST_VESA>>8, %ah
         jnc     check_vesaw
-        decb    %ah
 
 setbadw: clc
         ret
 
 check_vesaw:
         subb    $VIDEO_FIRST_VESA>>8, %bh
-        orw     $0x4000, %bx                    # Use linear frame buffer
+        orb     $0x40, %bh                      # Use linear frame buffer
         movw    $0x4f02, %ax                    # VESA BIOS mode set call
         int     $0x10
         cmpw    $0x004f, %ax                    # AL=4f if implemented
-        jnz     _setbadw                        # AH=0 if OK
+        jnz     setbadw                         # AH=0 if OK
 
         stc
         ret
 
-_setbadw: jmp    setbadw
-
 bogus_real_magic:
         movw    $0x0e00 + 'B', %fs:(0x12)
         jmp     bogus_real_magic