From: Jan Beulich <jbeulich@suse.com>
To: Andrew Cooper <andrew.cooper3@citrix.com>
Cc: George Dunlap <George.Dunlap@eu.citrix.com>,
xen-devel <xen-devel@lists.xenproject.org>, WeiLiu <wl@xen.org>,
Roger Pau Monne <roger.pau@citrix.com>
Subject: Re: [Xen-devel] [PATCH 3/3] x86: a little bit of 16-bit video mode setting code cleanup
Date: Thu, 29 Aug 2019 16:23:38 +0200 [thread overview]
Message-ID: <a8d05ee8-3206-a028-4275-d21ed7acc42f@suse.com> (raw)
In-Reply-To: <b4b51025-7ef0-8d11-8f2a-bf6d48e5db4b@citrix.com>
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
next prev parent reply other threads:[~2019-08-29 14:24 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-06-14 11:30 [Xen-devel] [PATCH 0/3] x86: S3 resume adjustments Jan Beulich
2019-06-14 11:37 ` [Xen-devel] [PATCH 1/3] x86/ACPI: re-park previously parked CPUs upon resume from S3 Jan Beulich
2019-06-14 16:52 ` Julien Grall
2019-06-17 6:40 ` Jan Beulich
2019-06-17 8:12 ` Julien Grall
2019-08-29 13:37 ` Andrew Cooper
2019-06-14 11:37 ` [Xen-devel] [PATCH RFC 2/3] x86/ACPI: restore VESA mode " Jan Beulich
2019-08-29 14:45 ` Andrew Cooper
2019-08-29 15:18 ` Jan Beulich
2019-06-14 11:38 ` [Xen-devel] [PATCH 3/3] x86: a little bit of 16-bit video mode setting code cleanup Jan Beulich
2019-08-29 14:08 ` Andrew Cooper
2019-08-29 14:23 ` Jan Beulich [this message]
2019-08-29 14:38 ` Andrew Cooper
2019-08-29 15:07 ` Jan Beulich
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a8d05ee8-3206-a028-4275-d21ed7acc42f@suse.com \
--to=jbeulich@suse.com \
--cc=George.Dunlap@eu.citrix.com \
--cc=andrew.cooper3@citrix.com \
--cc=roger.pau@citrix.com \
--cc=wl@xen.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).