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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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@xxxxxxxx>
>
> --- 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
>
>
>


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel