| Message ID | 20191213190436.24475-3-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/suspend: State cleanup | expand |
On Fri, Dec 13, 2019 at 07:04:32PM +0000, Andrew Cooper wrote: > The trampoline has already set up the idle pagetables (which are the correct > ones to use), and sanitised the flags state. I wonder why do we have wakeup.S and wakeup_prot.S, it would be easier to follow if it all was in the same file IMO. > > For %ss, __HYPERVISOR_DS64 is the correct descriptor to restore. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks, Roger.
On 17/12/2019 11:52, Roger Pau Monné wrote: > On Fri, Dec 13, 2019 at 07:04:32PM +0000, Andrew Cooper wrote: >> The trampoline has already set up the idle pagetables (which are the correct >> ones to use), and sanitised the flags state. > I wonder why do we have wakeup.S and wakeup_prot.S, it would be easier > to follow if it all was in the same file IMO. wakeup.S is the 16bit entry point, and lives in the trampoline below 1M. wakeup_prot.S is a bit of logic which lives in the main hypervisor. The naming could probably do with some improvement, but they can't feasibly be part of the same file. > >> For %ss, __HYPERVISOR_DS64 is the correct descriptor to restore. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com> Thanks, ~Andrew
On Tue, Dec 17, 2019 at 12:06:01PM +0000, Andrew Cooper wrote: > On 17/12/2019 11:52, Roger Pau Monné wrote: > > On Fri, Dec 13, 2019 at 07:04:32PM +0000, Andrew Cooper wrote: > >> The trampoline has already set up the idle pagetables (which are the correct > >> ones to use), and sanitised the flags state. > > I wonder why do we have wakeup.S and wakeup_prot.S, it would be easier > > to follow if it all was in the same file IMO. > > wakeup.S is the 16bit entry point, and lives in the trampoline below 1M. > > wakeup_prot.S is a bit of logic which lives in the main hypervisor. > > The naming could probably do with some improvement, but they can't > feasibly be part of the same file. Hm, I'm not sure I follow. Isn't this trampoline copied by Xen in a suitable position below the 1M boundary, and hence could use symbols in order to figure out which part to copy? Ie: both the low and the high part could live in the same file as long as Xen knows how to differentiate those and which chunk needs positioning below 1M? Thanks, Roger.
On 17/12/2019 12:18, Roger Pau Monné wrote: > On Tue, Dec 17, 2019 at 12:06:01PM +0000, Andrew Cooper wrote: >> On 17/12/2019 11:52, Roger Pau Monné wrote: >>> On Fri, Dec 13, 2019 at 07:04:32PM +0000, Andrew Cooper wrote: >>>> The trampoline has already set up the idle pagetables (which are the correct >>>> ones to use), and sanitised the flags state. >>> I wonder why do we have wakeup.S and wakeup_prot.S, it would be easier >>> to follow if it all was in the same file IMO. >> wakeup.S is the 16bit entry point, and lives in the trampoline below 1M. >> >> wakeup_prot.S is a bit of logic which lives in the main hypervisor. >> >> The naming could probably do with some improvement, but they can't >> feasibly be part of the same file. > Hm, I'm not sure I follow. Isn't this trampoline copied by Xen in a > suitable position below the 1M boundary, and hence could use symbols > in order to figure out which part to copy? > > Ie: both the low and the high part could live in the same file as long > as Xen knows how to differentiate those and which chunk needs > positioning below 1M? There is one trampoline.S (and trampoline.o) which gathers together various files (including wakeup.S) to construct the trampoline. It is not something which can be constructed simply by putting code/data in the requisite sections. There are two main entrypoints, one with a 4k alignment requirement, one with 16 byte alignment, and we split the trampoline into two parts - one which is BSP-only and is several pages in size, and one which is post-boot which is only a single page. ~Andrew
On Tue, Dec 17, 2019 at 12:26:24PM +0000, Andrew Cooper wrote: > On 17/12/2019 12:18, Roger Pau Monné wrote: > > On Tue, Dec 17, 2019 at 12:06:01PM +0000, Andrew Cooper wrote: > >> On 17/12/2019 11:52, Roger Pau Monné wrote: > >>> On Fri, Dec 13, 2019 at 07:04:32PM +0000, Andrew Cooper wrote: > >>>> The trampoline has already set up the idle pagetables (which are the correct > >>>> ones to use), and sanitised the flags state. > >>> I wonder why do we have wakeup.S and wakeup_prot.S, it would be easier > >>> to follow if it all was in the same file IMO. > >> wakeup.S is the 16bit entry point, and lives in the trampoline below 1M. > >> > >> wakeup_prot.S is a bit of logic which lives in the main hypervisor. > >> > >> The naming could probably do with some improvement, but they can't > >> feasibly be part of the same file. > > Hm, I'm not sure I follow. Isn't this trampoline copied by Xen in a > > suitable position below the 1M boundary, and hence could use symbols > > in order to figure out which part to copy? > > > > Ie: both the low and the high part could live in the same file as long > > as Xen knows how to differentiate those and which chunk needs > > positioning below 1M? > > There is one trampoline.S (and trampoline.o) which gathers together > various files (including wakeup.S) to construct the trampoline. Oh, I see it's all included to make a single unit, and the symbols used to mark the start and end of the trampoline chunk are defined outside of the included file. > It is not something which can be constructed simply by putting code/data > in the requisite sections. There are two main entrypoints, one with a > 4k alignment requirement, one with 16 byte alignment, and we split the > trampoline into two parts - one which is BSP-only and is several pages > in size, and one which is post-boot which is only a single page. Given the size of s3_resume I would guess there's space in that single page to fit it, but since it doesn't need to live below the 1M boundary it could be seen as a waste. Anyway, leaving it as-is is fine since placing it in wakeup.S would be a waste of space or require some restructuring of how the trampoline code is assembled. Thanks, Roger.
diff --git a/xen/arch/x86/acpi/wakeup_prot.S b/xen/arch/x86/acpi/wakeup_prot.S index 8c525a802b..35fd7a5e9f 100644 --- a/xen/arch/x86/acpi/wakeup_prot.S +++ b/xen/arch/x86/acpi/wakeup_prot.S @@ -29,17 +29,10 @@ ENTRY(do_suspend_lowlevel) SAVE_GREG(13) SAVE_GREG(14) SAVE_GREG(15) - pushfq; - popq SAVED_GREG(flags) - - mov %ss, REF(saved_ss) mov %cr0, GREG(ax) mov GREG(ax), REF(saved_cr0) - mov %cr3, GREG(ax) - mov GREG(ax), REF(saved_cr3) - call save_rest_processor_state /* enter sleep state physically */ @@ -55,6 +48,7 @@ ENTRY(do_suspend_lowlevel) * * The trampoline re-intercepts here. State is: * - 64bit mode + * - %cr3 => idle_pg_table[] * * Everything else, including the stack, needs restoring. */ @@ -65,13 +59,11 @@ ENTRY(s3_resume) mov REF(mmu_cr4_features), GREG(ax) mov GREG(ax), %cr4 - mov REF(saved_cr3), GREG(ax) - mov GREG(ax), %cr3 - mov REF(saved_cr0), GREG(ax) mov GREG(ax), %cr0 - mov REF(saved_ss), %ss + mov $__HYPERVISOR_DS64, %eax + mov %eax, %ss LOAD_GREG(sp) /* Reload code selector */ @@ -80,8 +72,6 @@ ENTRY(s3_resume) pushq %rax lretq 1: - pushq SAVED_GREG(flags) - popfq call restore_rest_processor_state @@ -109,8 +99,6 @@ ENTRY(s3_resume) GLOBAL(saved_magic) .long 0x9abcdef0 -saved_ss: .word 0 - .align 8 DECLARE_GREG(sp) DECLARE_GREG(bp) @@ -120,7 +108,6 @@ DECLARE_GREG(cx) DECLARE_GREG(dx) DECLARE_GREG(si) DECLARE_GREG(di) -DECLARE_GREG(flags) DECLARE_GREG(8) DECLARE_GREG(9) @@ -132,4 +119,3 @@ DECLARE_GREG(14) DECLARE_GREG(15) saved_cr0: .quad 0 -saved_cr3: .quad 0
The trampoline has already set up the idle pagetables (which are the correct ones to use), and sanitised the flags state. For %ss, __HYPERVISOR_DS64 is the correct descriptor to restore. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Wei Liu <wl@xen.org> CC: Roger Pau Monné <roger.pau@citrix.com> --- xen/arch/x86/acpi/wakeup_prot.S | 20 +++----------------- 1 file changed, 3 insertions(+), 17 deletions(-)