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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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

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