* [Xen-devel] [PATCH 0/3] x86: S3 resume adjustments
@ 2019-06-14 11:30 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
` (2 more replies)
0 siblings, 3 replies; 14+ messages in thread
From: Jan Beulich @ 2019-06-14 11:30 UTC (permalink / raw)
To: xen-devel; +Cc: George Dunlap, Andrew Cooper, Wei Liu, Roger Pau Monne
1: x86/ACPI: re-park previously parked CPUs upon resume from S3
2: x86/ACPI: restore VESA mode upon resume from S3
3: x86: a little bit of 16-bit video mode setting code cleanup
Patch 2 is meant to address an issue I've observed while testing
patch 1, and patch 3 is simply a collection a misc changes noticed
while putting together patch 2 as possibly worthwhile to make.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* [Xen-devel] [PATCH 1/3] x86/ACPI: re-park previously parked CPUs upon resume from S3
2019-06-14 11:30 [Xen-devel] [PATCH 0/3] x86: S3 resume adjustments Jan Beulich
@ 2019-06-14 11:37 ` Jan Beulich
2019-06-14 16:52 ` 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-06-14 11:38 ` [Xen-devel] [PATCH 3/3] x86: a little bit of 16-bit video mode setting code cleanup Jan Beulich
2 siblings, 2 replies; 14+ messages in thread
From: Jan Beulich @ 2019-06-14 11:37 UTC (permalink / raw)
To: xen-devel
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Ian Jackson, Tim Deegan,
Julien Grall
Aiui when resuming from S3, CPUs come back out of RESET/INIT. Therefore
they need to undergo the same procedure as was added elsewhere by
commits d8f974f1a6 ("x86: command line option to avoid use of secondary
hyper-threads") and 8797d20a6e ("x86: possibly bring up all CPUs even
if not all are supposed to be used").
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/common/cpu.c
+++ b/xen/common/cpu.c
@@ -105,7 +105,7 @@ int cpu_down(unsigned int cpu)
if ( err )
goto fail;
- if ( unlikely(system_state < SYS_STATE_active) )
+ if ( system_state < SYS_STATE_active || system_state == SYS_STATE_resume )
on_selected_cpus(cpumask_of(cpu), _take_cpu_down, NULL, true);
else if ( (err = stop_machine_run(take_cpu_down, NULL, cpu)) < 0 )
goto fail;
@@ -207,15 +207,19 @@ void enable_nonboot_cpus(void)
printk("Enabling non-boot CPUs ...\n");
- for_each_cpu ( cpu, &frozen_cpus )
+ for_each_present_cpu ( cpu )
{
+ if ( park_offline_cpus ? cpu == smp_processor_id()
+ : !cpumask_test_cpu(cpu, &frozen_cpus) )
+ continue;
if ( (error = cpu_up(cpu)) )
{
printk("Error bringing CPU%d up: %d\n", cpu, error);
BUG_ON(error == -EBUSY);
}
- else
- __cpumask_clear_cpu(cpu, &frozen_cpus);
+ else if ( !__cpumask_test_and_clear_cpu(cpu, &frozen_cpus) &&
+ (error = cpu_down(cpu)) )
+ printk("Error re-offlining CPU%d: %d\n", cpu, error);
}
for_each_cpu ( cpu, &frozen_cpus )
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* [Xen-devel] [PATCH RFC 2/3] x86/ACPI: restore VESA mode upon resume from S3
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 11:37 ` Jan Beulich
2019-08-29 14:45 ` Andrew Cooper
2019-06-14 11:38 ` [Xen-devel] [PATCH 3/3] x86: a little bit of 16-bit video mode setting code cleanup Jan Beulich
2 siblings, 1 reply; 14+ messages in thread
From: Jan Beulich @ 2019-06-14 11:37 UTC (permalink / raw)
To: xen-devel; +Cc: George Dunlap, Andrew Cooper, WeiLiu, Roger Pau Monne
In order for "acpi_sleep=s3_mode" to have any effect, we should record
the video mode we switched to during boot. Since right now there's mode
setting code for VESA modes only in the resume case, record the mode
just in that one case.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
RFC: On the box that I've been trying to test this on this didn't really
make a difference (in the random cases where resume works in the
first place there): The graphics card looks to remain powered off
even after the Dom0 kernel has resumed. Additionally using
"acpi_sleep=s3_bios" didn't make a difference either. Furthermore
it looks like the serial console (connected via PCI card) doesn't
work (yet) immediately after resume (I suppose it too is powered
down), and resume hangs altogether with it in use. Hence it's sort
of difficult to actually debug anything here.
---
I'm wondering actually whether the user having to explicitly request the
mode restoration is a good model: Why would we _not_ want to restore the
mode we've set during boot? In the worst case Dom0 kernel or X will
change the mode another time.
--- a/xen/arch/x86/boot/video.S
+++ b/xen/arch/x86/boot/video.S
@@ -455,14 +455,17 @@ check_vesa:
cmpb $0x99, %al
jnz _setbad # Doh! No linear frame buffer.
+ pushw %bx
subb $VIDEO_FIRST_VESA>>8, %bh
orw $0x4000, %bx # 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
movb $1, bootsym(graphic_mode) # flag graphic mode
+ movw %bx, bootsym(video_mode)
stc
ret
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* [Xen-devel] [PATCH 3/3] x86: a little bit of 16-bit video mode setting code cleanup
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 11:37 ` [Xen-devel] [PATCH RFC 2/3] x86/ACPI: restore VESA mode " Jan Beulich
@ 2019-06-14 11:38 ` Jan Beulich
2019-08-29 14:08 ` Andrew Cooper
2 siblings, 1 reply; 14+ messages in thread
From: Jan Beulich @ 2019-06-14 11:38 UTC (permalink / raw)
To: xen-devel; +Cc: George Dunlap, Andrew Cooper, WeiLiu, Roger Pau Monne
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>
--- 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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Xen-devel] [PATCH 1/3] x86/ACPI: re-park previously parked CPUs upon resume from S3
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-08-29 13:37 ` Andrew Cooper
1 sibling, 1 reply; 14+ messages in thread
From: Julien Grall @ 2019-06-14 16:52 UTC (permalink / raw)
To: Jan Beulich, xen-devel
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Andrew Cooper, Tim Deegan, Ian Jackson
Hi Jan,
The title and commit message are a bit odd to read because you are modifying
common code but everything is geared towards x86.
On 14/06/2019 12:37, Jan Beulich wrote:
> Aiui when resuming from S3, CPUs come back out of RESET/INIT. Therefore
> they need to undergo the same procedure as was added elsewhere by
> commits d8f974f1a6 ("x86: command line option to avoid use of secondary
> hyper-threads") and 8797d20a6e ("x86: possibly bring up all CPUs even
> if not all are supposed to be used").
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> --- a/xen/common/cpu.c
> +++ b/xen/common/cpu.c
> @@ -105,7 +105,7 @@ int cpu_down(unsigned int cpu)
> if ( err )
> goto fail;
>
> - if ( unlikely(system_state < SYS_STATE_active) )
> + if ( system_state < SYS_STATE_active || system_state == SYS_STATE_resume )
So this change here is necessary because enable_nonboot_cpus() may call
cpu_down(), am I right? If so, could you please mention it in the commit message?
> on_selected_cpus(cpumask_of(cpu), _take_cpu_down, NULL, true);
> else if ( (err = stop_machine_run(take_cpu_down, NULL, cpu)) < 0 )
> goto fail;
> @@ -207,15 +207,19 @@ void enable_nonboot_cpus(void)
>
> printk("Enabling non-boot CPUs ...\n");
>
> - for_each_cpu ( cpu, &frozen_cpus )
> + for_each_present_cpu ( cpu )
> {
> + if ( park_offline_cpus ? cpu == smp_processor_id()
park_offline_cpus is x86 specific, so it will not build on Arm.
> + : !cpumask_test_cpu(cpu, &frozen_cpus) )
> + continue;
> if ( (error = cpu_up(cpu)) )
> {
> printk("Error bringing CPU%d up: %d\n", cpu, error);
> BUG_ON(error == -EBUSY);
> }
> - else
> - __cpumask_clear_cpu(cpu, &frozen_cpus);
> + else if ( !__cpumask_test_and_clear_cpu(cpu, &frozen_cpus) &&
> + (error = cpu_down(cpu)) )
> + printk("Error re-offlining CPU%d: %d\n", cpu, error);
> }
>
> for_each_cpu ( cpu, &frozen_cpus )
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Xen-devel] [PATCH 1/3] x86/ACPI: re-park previously parked CPUs upon resume from S3
2019-06-14 16:52 ` Julien Grall
@ 2019-06-17 6:40 ` Jan Beulich
2019-06-17 8:12 ` Julien Grall
0 siblings, 1 reply; 14+ messages in thread
From: Jan Beulich @ 2019-06-17 6:40 UTC (permalink / raw)
To: Julien Grall
Cc: Stefano Stabellini, WeiLiu, Konrad Rzeszutek Wilk, George Dunlap,
Andrew Cooper, Ian Jackson, Tim Deegan, xen-devel
>>> On 14.06.19 at 18:52, <julien.grall@arm.com> wrote:
> The title and commit message are a bit odd to read because you are modifying
> common code but everything is geared towards x86.
Indeed. There's no caller of {en,dis}able_nonboot_cpus() in Arm code
at present, afaics. Hence the code changed (but not the file) is truly
x86-specific at the moment. I've explicitly thought about the
"inconsistency" between title and contents, but I've deliberately put it
as is: The change _is_ x86 / ACPI only, _despite_ touching common
code (and hence needing a REST maintainer ack).
>> --- a/xen/common/cpu.c
>> +++ b/xen/common/cpu.c
>> @@ -105,7 +105,7 @@ int cpu_down(unsigned int cpu)
>> if ( err )
>> goto fail;
>>
>> - if ( unlikely(system_state < SYS_STATE_active) )
>> + if ( system_state < SYS_STATE_active || system_state == SYS_STATE_resume )
>
> So this change here is necessary because enable_nonboot_cpus() may call
> cpu_down(), am I right?
Yes (albeit likely s/necessary/wanted/).
> If so, could you please mention it in the commit message?
Hmm, I could. But this is just paralleling what we're already doing for
the boot path, so it didn't seem imperative to me to call it out. But
anyway, I've added a sentence.
>> @@ -207,15 +207,19 @@ void enable_nonboot_cpus(void)
>>
>> printk("Enabling non-boot CPUs ...\n");
>>
>> - for_each_cpu ( cpu, &frozen_cpus )
>> + for_each_present_cpu ( cpu )
>> {
>> + if ( park_offline_cpus ? cpu == smp_processor_id()
>
> park_offline_cpus is x86 specific, so it will not build on Arm.
And that's intentional, even more so that (as said above) Arm doesn't
call here in the first place. And even if it did - whether to do things the
"new" way would then still (intentionally) depend on whether Arm had
any way of park_offline_cpus being "true".
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Xen-devel] [PATCH 1/3] x86/ACPI: re-park previously parked CPUs upon resume from S3
2019-06-17 6:40 ` Jan Beulich
@ 2019-06-17 8:12 ` Julien Grall
0 siblings, 0 replies; 14+ messages in thread
From: Julien Grall @ 2019-06-17 8:12 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, WeiLiu, Konrad Rzeszutek Wilk, George Dunlap,
Andrew Cooper, Ian Jackson, Tim Deegan, xen-devel
Hi Jan,
On 6/17/19 7:40 AM, Jan Beulich wrote:
>>>> On 14.06.19 at 18:52, <julien.grall@arm.com> wrote:
>> The title and commit message are a bit odd to read because you are modifying
>> common code but everything is geared towards x86.
>
> Indeed. There's no caller of {en,dis}able_nonboot_cpus() in Arm code
> at present, afaics. Hence the code changed (but not the file) is truly
> x86-specific at the moment. I've explicitly thought about the
> "inconsistency" between title and contents, but I've deliberately put it
> as is: The change _is_ x86 / ACPI only, _despite_ touching common
> code (and hence needing a REST maintainer ack).
Bear in mind that I have nearly no knowledge of x86, so trying to write
a commit message fully the x86 way is not going to help me understand
why this makes sense for everyone (today or in the future).
>>> @@ -207,15 +207,19 @@ void enable_nonboot_cpus(void)
>>>
>>> printk("Enabling non-boot CPUs ...\n");
>>>
>>> - for_each_cpu ( cpu, &frozen_cpus )
>>> + for_each_present_cpu ( cpu )
>>> {
>>> + if ( park_offline_cpus ? cpu == smp_processor_id()
>>
>> park_offline_cpus is x86 specific, so it will not build on Arm.
>
> And that's intentional, even more so that (as said above) Arm doesn't
> call here in the first place.
Calling and building are two separate things... A function may be built
even if it is not called.
> And even if it did - whether to do things the
> "new" way would then still (intentionally) depend on whether Arm had
> any way of park_offline_cpus being "true".
Looking again, we are defining park_offline_cpus to false on Arm (see
a6448adfd3 "xen/cpu: Fix ARM build following c/s 597fbb8"). So there are
no build issue as I first thought.
Cheers,
--
Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Xen-devel] [PATCH 1/3] x86/ACPI: re-park previously parked CPUs upon resume from S3
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-08-29 13:37 ` Andrew Cooper
1 sibling, 0 replies; 14+ messages in thread
From: Andrew Cooper @ 2019-08-29 13:37 UTC (permalink / raw)
To: Jan Beulich, xen-devel
Cc: Stefano Stabellini, Wei Liu, Konrad Rzeszutek Wilk,
George Dunlap, Ian Jackson, Tim Deegan, Julien Grall
On 14/06/2019 12:37, Jan Beulich wrote:
> Aiui when resuming from S3, CPUs come back out of RESET/INIT.
From a programmers point of view, all APs are in Wait-for-SIPI after
resume, and this is confirmed by several of the BWG flowcharts. This is
a logical consequence of the CPU losing all power, meaning that its
startup sequence will be the same whether it is booting from S5 or S3.
> Therefore
> they need to undergo the same procedure as was added elsewhere by
> commits d8f974f1a6 ("x86: command line option to avoid use of secondary
> hyper-threads") and 8797d20a6e ("x86: possibly bring up all CPUs even
> if not all are supposed to be used").
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Xen-devel] [PATCH 3/3] x86: a little bit of 16-bit video mode setting code cleanup
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
0 siblings, 1 reply; 14+ messages in thread
From: Andrew Cooper @ 2019-08-29 14:08 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: George Dunlap, WeiLiu, Roger Pau Monne
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
>
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Xen-devel] [PATCH 3/3] x86: a little bit of 16-bit video mode setting code cleanup
2019-08-29 14:08 ` Andrew Cooper
@ 2019-08-29 14:23 ` Jan Beulich
2019-08-29 14:38 ` Andrew Cooper
0 siblings, 1 reply; 14+ messages in thread
From: Jan Beulich @ 2019-08-29 14:23 UTC (permalink / raw)
To: Andrew Cooper; +Cc: George Dunlap, xen-devel, WeiLiu, Roger Pau Monne
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Xen-devel] [PATCH 3/3] x86: a little bit of 16-bit video mode setting code cleanup
2019-08-29 14:23 ` Jan Beulich
@ 2019-08-29 14:38 ` Andrew Cooper
2019-08-29 15:07 ` Jan Beulich
0 siblings, 1 reply; 14+ messages in thread
From: Andrew Cooper @ 2019-08-29 14:38 UTC (permalink / raw)
To: Jan Beulich; +Cc: George Dunlap, xen-devel, WeiLiu, Roger Pau Monne
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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Xen-devel] [PATCH RFC 2/3] x86/ACPI: restore VESA mode upon resume from S3
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
0 siblings, 1 reply; 14+ messages in thread
From: Andrew Cooper @ 2019-08-29 14:45 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: George Dunlap, WeiLiu, Roger Pau Monne
On 14/06/2019 12:37, Jan Beulich wrote:
> In order for "acpi_sleep=s3_mode" to have any effect, we should record
> the video mode we switched to during boot. Since right now there's mode
> setting code for VESA modes only in the resume case, record the mode
> just in that one case.
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> RFC: On the box that I've been trying to test this on this didn't really
> make a difference (in the random cases where resume works in the
> first place there): The graphics card looks to remain powered off
> even after the Dom0 kernel has resumed. Additionally using
> "acpi_sleep=s3_bios" didn't make a difference either. Furthermore
> it looks like the serial console (connected via PCI card) doesn't
> work (yet) immediately after resume (I suppose it too is powered
> down), and resume hangs altogether with it in use. Hence it's sort
> of difficult to actually debug anything here.
While you were away, I had an awful time debugging c/s 7aae9c1c91, even
with COM1.
I think we should take some proactive steps to try and make the serial
settings more robust, so printk() does continue to function before the
console irq gets restored.
In the case of legacy COM1/COM2, this should be falling back to polled
mode which at least lets the characters get out, and for PCI serial
cards, we should probably make an attempt to bring it out of D3 ahead of
relying on dom0 to do this.
> ---
> I'm wondering actually whether the user having to explicitly request the
> mode restoration is a good model: Why would we _not_ want to restore the
> mode we've set during boot? In the worst case Dom0 kernel or X will
> change the mode another time.
I think I agree. I can't see anything good which can come from offering
a choice here, and I am all for reducing the quantity of 16bit VGA code
we have.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Xen-devel] [PATCH 3/3] x86: a little bit of 16-bit video mode setting code cleanup
2019-08-29 14:38 ` Andrew Cooper
@ 2019-08-29 15:07 ` Jan Beulich
0 siblings, 0 replies; 14+ messages in thread
From: Jan Beulich @ 2019-08-29 15:07 UTC (permalink / raw)
To: Andrew Cooper; +Cc: George Dunlap, xen-devel, WeiLiu, Roger Pau Monne
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [Xen-devel] [PATCH RFC 2/3] x86/ACPI: restore VESA mode upon resume from S3
2019-08-29 14:45 ` Andrew Cooper
@ 2019-08-29 15:18 ` Jan Beulich
0 siblings, 0 replies; 14+ messages in thread
From: Jan Beulich @ 2019-08-29 15:18 UTC (permalink / raw)
To: Andrew Cooper; +Cc: George Dunlap, xen-devel, WeiLiu, Roger Pau Monne
On 29.08.2019 16:45, Andrew Cooper wrote:
> On 14/06/2019 12:37, Jan Beulich wrote:
>> ---
>> I'm wondering actually whether the user having to explicitly request the
>> mode restoration is a good model: Why would we _not_ want to restore the
>> mode we've set during boot? In the worst case Dom0 kernel or X will
>> change the mode another time.
>
> I think I agree. I can't see anything good which can come from offering
> a choice here, and I am all for reducing the quantity of 16bit VGA code
> we have.
Well, mode restoration may of course hang due to BIOS issues. But in such
a case the question is how sensible it is to use S3 on such a system.
Also note that adjusting this isn't going to reduce the quantity of
16-bit code; it would only be a change to what video_flags defaults to.
Right now, without the command line option provided, mode restoration
would happen only if reset_videomode_after_s3() gets invoked, i.e.
just on a single Toshiba laptop model which I guess didn't even have
64-bit capable CPUs.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2019-08-29 15:18 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
2019-08-29 14:38 ` Andrew Cooper
2019-08-29 15:07 ` Jan Beulich
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).