* [Xen-devel] [PATCH] x86/suspend: Simplify resume path
@ 2019-08-12 18:21 Andrew Cooper
2019-08-12 18:21 ` [Xen-devel] [PATCH] x86/suspend: Sanity check more properties in enter_state() Andrew Cooper
` (3 more replies)
0 siblings, 4 replies; 7+ messages in thread
From: Andrew Cooper @ 2019-08-12 18:21 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich, Roger Pau Monné
This started off as "get rid of load_TR()" as identified in the TSS cleanup
series, and morphed slightly.
Andrew Cooper (3):
x86/suspend: Sanity check more properties in enter_state()
x86/desc: Move boot_gdtr into .rodata
x86/suspend: Simplify system table handling on resume
xen/arch/x86/acpi/power.c | 2 ++
xen/arch/x86/acpi/suspend.c | 14 +++++++++++++-
xen/arch/x86/acpi/wakeup_prot.S | 13 +------------
xen/arch/x86/desc.c | 2 +-
xen/arch/x86/traps.c | 21 ---------------------
xen/include/asm-x86/desc.h | 2 --
6 files changed, 17 insertions(+), 37 deletions(-)
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* [Xen-devel] [PATCH] x86/suspend: Sanity check more properties in enter_state()
2019-08-12 18:21 [Xen-devel] [PATCH] x86/suspend: Simplify resume path Andrew Cooper
@ 2019-08-12 18:21 ` Andrew Cooper
2019-08-12 18:21 ` [Xen-devel] [PATCH] x86/desc: Move boot_gdtr into .rodata Andrew Cooper
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: Andrew Cooper @ 2019-08-12 18:21 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich, Roger Pau Monné
The logic depends on being run on CPU0, and in IDLE context. Having this
explicitly identified allows for simplification of the whole S3 path.
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/power.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/xen/arch/x86/acpi/power.c b/xen/arch/x86/acpi/power.c
index aecc754fdb..d83e8cdd52 100644
--- a/xen/arch/x86/acpi/power.c
+++ b/xen/arch/x86/acpi/power.c
@@ -174,6 +174,8 @@ static int enter_state(u32 state)
return -EBUSY;
BUG_ON(system_state != SYS_STATE_active);
+ BUG_ON(!is_idle_vcpu(current));
+ BUG_ON(smp_processor_id() != 0);
system_state = SYS_STATE_suspend;
printk(XENLOG_INFO "Preparing system for ACPI S%d state.\n", state);
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [Xen-devel] [PATCH] x86/desc: Move boot_gdtr into .rodata
2019-08-12 18:21 [Xen-devel] [PATCH] x86/suspend: Simplify resume path Andrew Cooper
2019-08-12 18:21 ` [Xen-devel] [PATCH] x86/suspend: Sanity check more properties in enter_state() Andrew Cooper
@ 2019-08-12 18:21 ` Andrew Cooper
2019-08-12 18:21 ` [Xen-devel] [PATCH] x86/suspend: Simplify system table handling on resume Andrew Cooper
2019-08-27 14:17 ` [Xen-devel] [PATCH] x86/suspend: Simplify resume path Jan Beulich
3 siblings, 0 replies; 7+ messages in thread
From: Andrew Cooper @ 2019-08-12 18:21 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich, Roger Pau Monné
It is never written to.
This was an oversight when it was moved from asm into C.
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/desc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/xen/arch/x86/desc.c b/xen/arch/x86/desc.c
index 42ccdc2f8c..dfeb1beaa8 100644
--- a/xen/arch/x86/desc.c
+++ b/xen/arch/x86/desc.c
@@ -89,7 +89,7 @@ seg_desc_t boot_compat_gdt[PAGE_SIZE / sizeof(seg_desc_t)] =
* References boot_cpu_gdt_table for a short period, until the CPUs switch
* onto their per-CPU GDTs.
*/
-struct desc_ptr boot_gdtr = {
+const struct desc_ptr boot_gdtr = {
.limit = LAST_RESERVED_GDT_BYTE,
.base = (unsigned long)(boot_gdt - FIRST_RESERVED_GDT_ENTRY),
};
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [Xen-devel] [PATCH] x86/suspend: Simplify system table handling on resume
2019-08-12 18:21 [Xen-devel] [PATCH] x86/suspend: Simplify resume path Andrew Cooper
2019-08-12 18:21 ` [Xen-devel] [PATCH] x86/suspend: Sanity check more properties in enter_state() Andrew Cooper
2019-08-12 18:21 ` [Xen-devel] [PATCH] x86/desc: Move boot_gdtr into .rodata Andrew Cooper
@ 2019-08-12 18:21 ` Andrew Cooper
2019-08-28 14:10 ` Jan Beulich
2019-08-27 14:17 ` [Xen-devel] [PATCH] x86/suspend: Simplify resume path Jan Beulich
3 siblings, 1 reply; 7+ messages in thread
From: Andrew Cooper @ 2019-08-12 18:21 UTC (permalink / raw)
To: Xen-devel; +Cc: Andrew Cooper, Wei Liu, Jan Beulich, Roger Pau Monné
load_TR() is used exclusively in the resume path, but jumps through a lot of
unnecessary hoops.
As suspend/resume is strictly on CPU0 in idle context, the correct GDT to use
is boot_gdt, which means it doesn't need saving on suspend. Similarly, the
correct IDT to use can be derived, and the LDT is guaranteed to be NUL.
The TR is still correct in the GDT, but needs the busy bit clearing before we
can reload it.
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>
A slightly different option would be to call load_system_tables() rather than
opencoding part of it in restore_rest_processor_state(). However, that is
more setup than is necessary. Thoughts?
---
xen/arch/x86/acpi/suspend.c | 14 +++++++++++++-
xen/arch/x86/acpi/wakeup_prot.S | 13 +------------
xen/arch/x86/traps.c | 21 ---------------------
xen/include/asm-x86/desc.h | 2 --
4 files changed, 14 insertions(+), 36 deletions(-)
diff --git a/xen/arch/x86/acpi/suspend.c b/xen/arch/x86/acpi/suspend.c
index ba9d2e13a7..a6f2584645 100644
--- a/xen/arch/x86/acpi/suspend.c
+++ b/xen/arch/x86/acpi/suspend.c
@@ -41,7 +41,19 @@ void save_rest_processor_state(void)
void restore_rest_processor_state(void)
{
- load_TR();
+ unsigned int cpu = smp_processor_id();
+ seg_desc_t *gdt = per_cpu(gdt, cpu) - FIRST_RESERVED_GDT_ENTRY;
+ struct tss64 *tss = &per_cpu(tss_page, cpu).tss;
+ const struct desc_ptr idtr = {
+ .base = (unsigned long)idt_tables[cpu],
+ .limit = (IDT_ENTRIES * sizeof(idt_entry_t)) - 1,
+ };
+
+ _set_tssldt_desc(gdt + TSS_ENTRY, (unsigned long)tss,
+ sizeof(*tss) - 1, SYS_DESC_tss_avail);
+ lidt(&idtr);
+ ltr(TSS_SELECTOR);
+ lldt(0);
/* Recover syscall MSRs */
wrmsrl(MSR_LSTAR, saved_lstar);
diff --git a/xen/arch/x86/acpi/wakeup_prot.S b/xen/arch/x86/acpi/wakeup_prot.S
index 9e9fcc1ab6..74261cb4f1 100644
--- a/xen/arch/x86/acpi/wakeup_prot.S
+++ b/xen/arch/x86/acpi/wakeup_prot.S
@@ -34,10 +34,6 @@ ENTRY(do_suspend_lowlevel)
mov %ss, REF(saved_ss)
- sgdt REF(saved_gdt)
- sidt REF(saved_idt)
- sldt REF(saved_ldt)
-
mov %cr0, GREG(ax)
mov GREG(ax), REF(saved_cr0)
@@ -55,6 +51,7 @@ ENTRY(do_suspend_lowlevel)
ENTRY(__ret_point)
+ lgdt boot_gdtr(%rip)
/* mmu_cr4_features contains latest cr4 setting */
mov REF(mmu_cr4_features), GREG(ax)
@@ -66,10 +63,6 @@ ENTRY(__ret_point)
mov REF(saved_cr0), GREG(ax)
mov GREG(ax), %cr0
- lgdt REF(saved_gdt)
- lidt REF(saved_idt)
- lldt REF(saved_ldt)
-
mov REF(saved_ss), %ss
LOAD_GREG(sp)
@@ -129,9 +122,5 @@ DECLARE_GREG(13)
DECLARE_GREG(14)
DECLARE_GREG(15)
-saved_gdt: .quad 0,0
-saved_idt: .quad 0,0
-saved_ldt: .quad 0,0
-
saved_cr0: .quad 0
saved_cr3: .quad 0
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 23069e25ec..b424687f85 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -1901,27 +1901,6 @@ static void __init set_intr_gate(unsigned int n, void *addr)
__set_intr_gate(n, 0, addr);
}
-void load_TR(void)
-{
- struct tss64 *tss = &this_cpu(tss_page).tss;
- struct desc_ptr old_gdt, tss_gdt = {
- .base = (long)(this_cpu(gdt) - FIRST_RESERVED_GDT_ENTRY),
- .limit = LAST_RESERVED_GDT_BYTE
- };
-
- _set_tssldt_desc(
- this_cpu(gdt) + TSS_ENTRY - FIRST_RESERVED_GDT_ENTRY,
- (unsigned long)tss, sizeof(*tss) - 1, SYS_DESC_tss_avail);
- _set_tssldt_desc(
- this_cpu(compat_gdt) + TSS_ENTRY - FIRST_RESERVED_GDT_ENTRY,
- (unsigned long)tss, sizeof(*tss) - 1, SYS_DESC_tss_busy);
-
- /* Switch to non-compat GDT (which has B bit clear) to execute LTR. */
- asm volatile (
- "sgdt %0; lgdt %2; ltr %w1; lgdt %0"
- : "=m" (old_gdt) : "rm" (TSS_SELECTOR), "m" (tss_gdt) : "memory" );
-}
-
static unsigned int calc_ler_msr(void)
{
switch ( boot_cpu_data.x86_vendor )
diff --git a/xen/include/asm-x86/desc.h b/xen/include/asm-x86/desc.h
index 603b9a9013..24db3e9510 100644
--- a/xen/include/asm-x86/desc.h
+++ b/xen/include/asm-x86/desc.h
@@ -216,8 +216,6 @@ DECLARE_PER_CPU(seg_desc_t *, compat_gdt);
DECLARE_PER_CPU(l1_pgentry_t, compat_gdt_l1e);
DECLARE_PER_CPU(bool, full_gdt_loaded);
-extern void load_TR(void);
-
static inline void lgdt(const struct desc_ptr *gdtr)
{
__asm__ __volatile__ ( "lgdt %0" :: "m" (*gdtr) : "memory" );
--
2.11.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Xen-devel] [PATCH] x86/suspend: Simplify resume path
2019-08-12 18:21 [Xen-devel] [PATCH] x86/suspend: Simplify resume path Andrew Cooper
` (2 preceding siblings ...)
2019-08-12 18:21 ` [Xen-devel] [PATCH] x86/suspend: Simplify system table handling on resume Andrew Cooper
@ 2019-08-27 14:17 ` Jan Beulich
3 siblings, 0 replies; 7+ messages in thread
From: Jan Beulich @ 2019-08-27 14:17 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel, Wei Liu, Roger Pau Monné
On 12.08.2019 20:21, Andrew Cooper wrote:
> This started off as "get rid of load_TR()" as identified in the TSS cleanup
> series, and morphed slightly.
>
> Andrew Cooper (3):
> x86/suspend: Sanity check more properties in enter_state()
> x86/desc: Move boot_gdtr into .rodata
> x86/suspend: Simplify system table handling on resume
Acked-by: Jan Beulich <jbeulich@suse.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Xen-devel] [PATCH] x86/suspend: Simplify system table handling on resume
2019-08-12 18:21 ` [Xen-devel] [PATCH] x86/suspend: Simplify system table handling on resume Andrew Cooper
@ 2019-08-28 14:10 ` Jan Beulich
2019-08-28 14:16 ` Andrew Cooper
0 siblings, 1 reply; 7+ messages in thread
From: Jan Beulich @ 2019-08-28 14:10 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Xen-devel, Wei Liu, Roger Pau Monné
On 12.08.2019 20:21, Andrew Cooper wrote:
> load_TR() is used exclusively in the resume path, but jumps through a lot of
> unnecessary hoops.
>
> As suspend/resume is strictly on CPU0 in idle context, the correct GDT to use
> is boot_gdt, which means it doesn't need saving on suspend. Similarly, the
> correct IDT to use can be derived, and the LDT is guaranteed to be NUL.
>
> The TR is still correct in the GDT, but needs the busy bit clearing before we
> can reload it.
>
> 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>
>
> A slightly different option would be to call load_system_tables() rather than
> opencoding part of it in restore_rest_processor_state(). However, that is
> more setup than is necessary. Thoughts?
This might indeed be better (despite the parts not needed on this
path), as it would further centralize the handling. Of course only
as long as there's no (significant) change needed to the function
in order to be used here.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Xen-devel] [PATCH] x86/suspend: Simplify system table handling on resume
2019-08-28 14:10 ` Jan Beulich
@ 2019-08-28 14:16 ` Andrew Cooper
0 siblings, 0 replies; 7+ messages in thread
From: Andrew Cooper @ 2019-08-28 14:16 UTC (permalink / raw)
To: Jan Beulich; +Cc: Xen-devel, Wei Liu, Roger Pau Monné
On 28/08/2019 15:10, Jan Beulich wrote:
> On 12.08.2019 20:21, Andrew Cooper wrote:
>> load_TR() is used exclusively in the resume path, but jumps through a lot of
>> unnecessary hoops.
>>
>> As suspend/resume is strictly on CPU0 in idle context, the correct GDT to use
>> is boot_gdt, which means it doesn't need saving on suspend. Similarly, the
>> correct IDT to use can be derived, and the LDT is guaranteed to be NUL.
>>
>> The TR is still correct in the GDT, but needs the busy bit clearing before we
>> can reload it.
>>
>> 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>
>>
>> A slightly different option would be to call load_system_tables() rather than
>> opencoding part of it in restore_rest_processor_state(). However, that is
>> more setup than is necessary. Thoughts?
> This might indeed be better (despite the parts not needed on this
> path), as it would further centralize the handling. Of course only
> as long as there's no (significant) change needed to the function
> in order to be used here.
No change at all. The AP's already do go through load_system_tables()
on S3 resume.
It is slightly suboptimal that we get two identical lgdt's this close
together, but it is the only way of ending up entering C with
__HYPERVISOR_CS.
At some future point, I'll see how easy it is to arrange the trampoline
GDT to be usable with __HYPERVISOR_CS, which would simplify the boot and
resume paths.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-08-28 14:17 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-12 18:21 [Xen-devel] [PATCH] x86/suspend: Simplify resume path Andrew Cooper
2019-08-12 18:21 ` [Xen-devel] [PATCH] x86/suspend: Sanity check more properties in enter_state() Andrew Cooper
2019-08-12 18:21 ` [Xen-devel] [PATCH] x86/desc: Move boot_gdtr into .rodata Andrew Cooper
2019-08-12 18:21 ` [Xen-devel] [PATCH] x86/suspend: Simplify system table handling on resume Andrew Cooper
2019-08-28 14:10 ` Jan Beulich
2019-08-28 14:16 ` Andrew Cooper
2019-08-27 14:17 ` [Xen-devel] [PATCH] x86/suspend: Simplify resume path 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).