Re: [Xen-devel] [PATCH] x86: store cr4 during suspend/resume

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 11/12/2019 19:36, Andrew Cooper wrote:
> Absolutely nothing remaining in suspend.c should be spilled.  It can all
> be (re)caluclated from the same information source as the AP boot path,
> and the result will be strictly smaller in RAM, and more robust.

And at a cursory glance, the logic in suspend.c doesn't correctly handle
the ler=1 case.  This can trivially be fixed by:

diff --git a/xen/arch/x86/acpi/suspend.c b/xen/arch/x86/acpi/suspend.c
index c9dea67bf3..50dbb14528 100644
--- a/xen/arch/x86/acpi/suspend.c
+++ b/xen/arch/x86/acpi/suspend.c
@@ -15,8 +15,6 @@
 #include <asm/xstate.h>
 #include <xen/hypercall.h>
 
-static unsigned long saved_lstar, saved_cstar;
-static unsigned long saved_sysenter_esp, saved_sysenter_eip;
 static unsigned long saved_fs_base, saved_gs_base, saved_kernel_gs_base;
 static uint64_t saved_xcr0;
 
@@ -24,15 +22,6 @@ void save_rest_processor_state(void)
 {
     saved_fs_base = rdfsbase();
     saved_gs_base = rdgsbase();
-    rdmsrl(MSR_SHADOW_GS_BASE, saved_kernel_gs_base);
-    rdmsrl(MSR_CSTAR, saved_cstar);
-    rdmsrl(MSR_LSTAR, saved_lstar);
-
-    if ( cpu_has_sep )
-    {
-        rdmsrl(MSR_IA32_SYSENTER_ESP, saved_sysenter_esp);
-        rdmsrl(MSR_IA32_SYSENTER_EIP, saved_sysenter_eip);
-    }
 
     if ( cpu_has_xsave )
         saved_xcr0 = get_xcr0();
@@ -42,25 +31,12 @@ void save_rest_processor_state(void)
 void restore_rest_processor_state(void)
 {
     load_system_tables();
-
-    /* Recover syscall MSRs */
-    wrmsrl(MSR_LSTAR, saved_lstar);
-    wrmsrl(MSR_CSTAR, saved_cstar);
-    wrmsrl(MSR_STAR, XEN_MSR_STAR);
-    wrmsrl(MSR_SYSCALL_MASK, XEN_SYSCALL_MASK);
+    percpu_traps_init();
 
     wrfsbase(saved_fs_base);
     wrgsbase(saved_gs_base);
     wrmsrl(MSR_SHADOW_GS_BASE, saved_kernel_gs_base);
 
-    if ( cpu_has_sep )
-    {
-        /* Recover sysenter MSRs */
-        wrmsrl(MSR_IA32_SYSENTER_ESP, saved_sysenter_esp);
-        wrmsrl(MSR_IA32_SYSENTER_EIP, saved_sysenter_eip);
-        wrmsr(MSR_IA32_SYSENTER_CS, __HYPERVISOR_CS, 0);
-    }
-
     if ( cpu_has_xsave && !set_xcr0(saved_xcr0) )
         BUG();
 

for starters.

The fs/gs/shadow values are guest-only state so can just be discarded.

xcr0 isn't needed but this code is latently(soon-to-be?) buggy by not
handling XSS at the same time.  Both should follow the BSP logic for
evaluating the system default.

That lets us delete save_rest_processor_state() entirely.

In the assembly side of things, ss and cr3 obviously don't need to be
spilled.  ss is always 0, and we're already on the correct pagetable. 
We're also probably on the correct cr0.

The caller-clobbered GPRs can just be discarded, and that gets our
stashed state down to almost nothing.

I trust I have made my point blindingly obvious.

~Andrew

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