x86/S3: Drop {save, restore}_rest_processor_state() completely
diff mbox series

Message ID 20200429110903.15418-1-andrew.cooper3@citrix.com
State New
Headers show
Series
  • x86/S3: Drop {save, restore}_rest_processor_state() completely
Related show

Commit Message

Andrew Cooper April 29, 2020, 11:09 a.m. UTC
There is no need to save/restore FS/GS/XCR0 state.  It will be handled
suitably on the context switch away from the idle.

The CR4 restoration in restore_rest_processor_state() was actually fighting
later code in enter_state() which tried to keep CR4.MCE clear until everything
was set up.  Delete the intermediate restoration, and defer final restoration
until after MCE is reconfigured.

Changing PAT should be done before paging is enabled.  Move it into the
trampoline during setup for 64bit.

The only remaing piece of restoration is load_system_tables(), so suspend.c
can be deleted in its entirety.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monne <roger.pau@citrix.com>
---
 xen/arch/x86/acpi/Makefile      |  2 +-
 xen/arch/x86/acpi/power.c       |  9 ++++----
 xen/arch/x86/acpi/suspend.c     | 49 -----------------------------------------
 xen/arch/x86/acpi/wakeup_prot.S |  4 +---
 xen/arch/x86/boot/trampoline.S  |  5 +++++
 5 files changed, 11 insertions(+), 58 deletions(-)
 delete mode 100644 xen/arch/x86/acpi/suspend.c

Comments

Jan Beulich April 29, 2020, 11:16 a.m. UTC | #1
On 29.04.2020 13:09, Andrew Cooper wrote:
> --- a/xen/arch/x86/boot/trampoline.S
> +++ b/xen/arch/x86/boot/trampoline.S
> @@ -91,6 +91,11 @@ trampoline_protmode_entry:
>          and     %edi,%edx
>          wrmsr
>  1:
> +        /* Set up PAT before enabling paging. */
> +        mov     $XEN_MSR_PAT & 0xffffffff, %eax
> +        mov     $XEN_MSR_PAT >> 32, %edx
> +        mov     $MSR_IA32_CR_PAT, %ecx
> +        wrmsr

Doesn't this also eliminate the need for cpu_init() doing this?
If you agree with that one also dropped
Reviewed-by: Jan Beulich <jbeulich@suse.com>

Jan
Andrew Cooper April 29, 2020, 11:32 a.m. UTC | #2
On 29/04/2020 12:16, Jan Beulich wrote:
> On 29.04.2020 13:09, Andrew Cooper wrote:
>> --- a/xen/arch/x86/boot/trampoline.S
>> +++ b/xen/arch/x86/boot/trampoline.S
>> @@ -91,6 +91,11 @@ trampoline_protmode_entry:
>>          and     %edi,%edx
>>          wrmsr
>>  1:
>> +        /* Set up PAT before enabling paging. */
>> +        mov     $XEN_MSR_PAT & 0xffffffff, %eax
>> +        mov     $XEN_MSR_PAT >> 32, %edx
>> +        mov     $MSR_IA32_CR_PAT, %ecx
>> +        wrmsr
> Doesn't this also eliminate the need for cpu_init() doing this?
> If you agree with that one also dropped
> Reviewed-by: Jan Beulich <jbeulich@suse.com>

That doesn't cover the BSP on either the legacy or EFI paths.

~Andrew
Jan Beulich April 29, 2020, 1:25 p.m. UTC | #3
On 29.04.2020 13:32, Andrew Cooper wrote:
> On 29/04/2020 12:16, Jan Beulich wrote:
>> On 29.04.2020 13:09, Andrew Cooper wrote:
>>> --- a/xen/arch/x86/boot/trampoline.S
>>> +++ b/xen/arch/x86/boot/trampoline.S
>>> @@ -91,6 +91,11 @@ trampoline_protmode_entry:
>>>          and     %edi,%edx
>>>          wrmsr
>>>  1:
>>> +        /* Set up PAT before enabling paging. */
>>> +        mov     $XEN_MSR_PAT & 0xffffffff, %eax
>>> +        mov     $XEN_MSR_PAT >> 32, %edx
>>> +        mov     $MSR_IA32_CR_PAT, %ecx
>>> +        wrmsr
>> Doesn't this also eliminate the need for cpu_init() doing this?
>> If you agree with that one also dropped
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> 
> That doesn't cover the BSP on either the legacy or EFI paths.

The legacy path, afaict, uses it:

.Lskip_realmode:
        /* EBX == 0 indicates we are the BP (Boot Processor). */
        xor     %ebx,%ebx

        /* Jump to the common bootstrap entry point. */
        jmp     trampoline_protmode_entry

The xen.efi entry path really should have the change you make
mirrored anyway.

Jan
Andrew Cooper April 29, 2020, 1:36 p.m. UTC | #4
On 29/04/2020 14:25, Jan Beulich wrote:
> On 29.04.2020 13:32, Andrew Cooper wrote:
>> On 29/04/2020 12:16, Jan Beulich wrote:
>>> On 29.04.2020 13:09, Andrew Cooper wrote:
>>>> --- a/xen/arch/x86/boot/trampoline.S
>>>> +++ b/xen/arch/x86/boot/trampoline.S
>>>> @@ -91,6 +91,11 @@ trampoline_protmode_entry:
>>>>          and     %edi,%edx
>>>>          wrmsr
>>>>  1:
>>>> +        /* Set up PAT before enabling paging. */
>>>> +        mov     $XEN_MSR_PAT & 0xffffffff, %eax
>>>> +        mov     $XEN_MSR_PAT >> 32, %edx
>>>> +        mov     $MSR_IA32_CR_PAT, %ecx
>>>> +        wrmsr
>>> Doesn't this also eliminate the need for cpu_init() doing this?
>>> If you agree with that one also dropped
>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>> That doesn't cover the BSP on either the legacy or EFI paths.
> The legacy path, afaict, uses it:
>
> .Lskip_realmode:
>         /* EBX == 0 indicates we are the BP (Boot Processor). */
>         xor     %ebx,%ebx
>
>         /* Jump to the common bootstrap entry point. */
>         jmp     trampoline_protmode_entry

Oh, of course.

> The xen.efi entry path really should have the change you make
> mirrored anyway.

Are you happy for it to go in efi_arch_post_exit_boot()?  We don't
disable paging, but that is the point where we switch from the EFI
pagetables to Xen's.

~Andrew
Jan Beulich April 29, 2020, 1:43 p.m. UTC | #5
On 29.04.2020 15:36, Andrew Cooper wrote:
> On 29/04/2020 14:25, Jan Beulich wrote:
>> On 29.04.2020 13:32, Andrew Cooper wrote:
>>> On 29/04/2020 12:16, Jan Beulich wrote:
>>>> On 29.04.2020 13:09, Andrew Cooper wrote:
>>>>> --- a/xen/arch/x86/boot/trampoline.S
>>>>> +++ b/xen/arch/x86/boot/trampoline.S
>>>>> @@ -91,6 +91,11 @@ trampoline_protmode_entry:
>>>>>          and     %edi,%edx
>>>>>          wrmsr
>>>>>  1:
>>>>> +        /* Set up PAT before enabling paging. */
>>>>> +        mov     $XEN_MSR_PAT & 0xffffffff, %eax
>>>>> +        mov     $XEN_MSR_PAT >> 32, %edx
>>>>> +        mov     $MSR_IA32_CR_PAT, %ecx
>>>>> +        wrmsr
>>>> Doesn't this also eliminate the need for cpu_init() doing this?
>>>> If you agree with that one also dropped
>>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>> That doesn't cover the BSP on either the legacy or EFI paths.
>> The legacy path, afaict, uses it:
>>
>> .Lskip_realmode:
>>         /* EBX == 0 indicates we are the BP (Boot Processor). */
>>         xor     %ebx,%ebx
>>
>>         /* Jump to the common bootstrap entry point. */
>>         jmp     trampoline_protmode_entry
> 
> Oh, of course.
> 
>> The xen.efi entry path really should have the change you make
>> mirrored anyway.
> 
> Are you happy for it to go in efi_arch_post_exit_boot()?  We don't
> disable paging, but that is the point where we switch from the EFI
> pagetables to Xen's.

Yes, that's the most "symmetrical" place, I think.

Jan

Patch
diff mbox series

diff --git a/xen/arch/x86/acpi/Makefile b/xen/arch/x86/acpi/Makefile
index 1b9e625713..041377e2bb 100644
--- a/xen/arch/x86/acpi/Makefile
+++ b/xen/arch/x86/acpi/Makefile
@@ -1,4 +1,4 @@ 
 obj-y += cpufreq/
 
-obj-y += lib.o power.o suspend.o cpu_idle.o cpuidle_menu.o
+obj-y += lib.o power.o cpu_idle.o cpuidle_menu.o
 obj-bin-y += boot.init.o wakeup_prot.o
diff --git a/xen/arch/x86/acpi/power.c b/xen/arch/x86/acpi/power.c
index 6dfd4c7891..0cda362045 100644
--- a/xen/arch/x86/acpi/power.c
+++ b/xen/arch/x86/acpi/power.c
@@ -195,7 +195,6 @@  static int enter_state(u32 state)
     unsigned long flags;
     int error;
     struct cpu_info *ci;
-    unsigned long cr4;
 
     if ( (state <= ACPI_STATE_S0) || (state > ACPI_S_STATES_MAX) )
         return -EINVAL;
@@ -270,15 +269,15 @@  static int enter_state(u32 state)
 
     system_state = SYS_STATE_resume;
 
-    /* Restore CR4 and EFER from cached values. */
-    cr4 = read_cr4();
-    write_cr4(cr4 & ~X86_CR4_MCE);
+    /* Restore EFER from cached value. */
     write_efer(read_efer());
 
     device_power_up(SAVED_ALL);
 
     mcheck_init(&boot_cpu_data, false);
-    write_cr4(cr4);
+
+    /* Restore CR4 from cached value, now MCE is set up. */
+    write_cr4(read_cr4());
 
     printk(XENLOG_INFO "Finishing wakeup from ACPI S%d state.\n", state);
 
diff --git a/xen/arch/x86/acpi/suspend.c b/xen/arch/x86/acpi/suspend.c
deleted file mode 100644
index 3c1a3cbb34..0000000000
--- a/xen/arch/x86/acpi/suspend.c
+++ /dev/null
@@ -1,49 +0,0 @@ 
-/*
- * Portions are:
- *  Copyright (c) 2002 Pavel Machek <pavel@suse.cz>
- *  Copyright (c) 2001 Patrick Mochel <mochel@osdl.org>
- */
-
-#include <xen/acpi.h>
-#include <xen/smp.h>
-#include <asm/processor.h>
-#include <asm/msr.h>
-#include <asm/debugreg.h>
-#include <asm/hvm/hvm.h>
-#include <asm/hvm/support.h>
-#include <asm/i387.h>
-#include <asm/xstate.h>
-#include <xen/hypercall.h>
-
-static unsigned long saved_fs_base, saved_gs_base, saved_kernel_gs_base;
-static uint64_t saved_xcr0;
-
-void save_rest_processor_state(void)
-{
-    saved_fs_base = rdfsbase();
-    saved_gs_base = rdgsbase();
-    rdmsrl(MSR_SHADOW_GS_BASE, saved_kernel_gs_base);
-
-    if ( cpu_has_xsave )
-        saved_xcr0 = get_xcr0();
-}
-
-
-void restore_rest_processor_state(void)
-{
-    load_system_tables();
-
-    /* Restore full CR4 (inc MCE) now that the IDT is in place. */
-    write_cr4(mmu_cr4_features);
-
-    wrfsbase(saved_fs_base);
-    wrgsbase(saved_gs_base);
-    wrmsrl(MSR_SHADOW_GS_BASE, saved_kernel_gs_base);
-
-    if ( cpu_has_xsave && !set_xcr0(saved_xcr0) )
-        BUG();
-
-    wrmsrl(MSR_IA32_CR_PAT, XEN_MSR_PAT);
-
-    mtrr_bp_restore();
-}
diff --git a/xen/arch/x86/acpi/wakeup_prot.S b/xen/arch/x86/acpi/wakeup_prot.S
index 0ce96e26a9..4dba6020a7 100644
--- a/xen/arch/x86/acpi/wakeup_prot.S
+++ b/xen/arch/x86/acpi/wakeup_prot.S
@@ -15,8 +15,6 @@  ENTRY(do_suspend_lowlevel)
         mov     %cr0, %rax
         mov     %rax, saved_cr0(%rip)
 
-        call    save_rest_processor_state
-
         /* enter sleep state physically */
         mov     $3, %edi
         call    acpi_enter_sleep_state
@@ -51,7 +49,7 @@  ENTRY(s3_resume)
         lretq
 1:
 
-        call restore_rest_processor_state
+        call    load_system_tables
 
 .Lsuspend_err:
         pop     %r15
diff --git a/xen/arch/x86/boot/trampoline.S b/xen/arch/x86/boot/trampoline.S
index 18c6638924..662e6bdd3c 100644
--- a/xen/arch/x86/boot/trampoline.S
+++ b/xen/arch/x86/boot/trampoline.S
@@ -91,6 +91,11 @@  trampoline_protmode_entry:
         and     %edi,%edx
         wrmsr
 1:
+        /* Set up PAT before enabling paging. */
+        mov     $XEN_MSR_PAT & 0xffffffff, %eax
+        mov     $XEN_MSR_PAT >> 32, %edx
+        mov     $MSR_IA32_CR_PAT, %ecx
+        wrmsr
 
         /* Set up EFER (Extended Feature Enable Register). */
         movl    $MSR_EFER,%ecx