Re: [Xen-devel] [PATCH 6/6] x86/suspend: Drop save_rest_processor_state() completely

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Dec 13, 2019 at 07:04:36PM +0000, Andrew Cooper wrote:
> Construct the system linkage MSRs using percpu_traps_init(), brining the S3
> path in line with the BSP/AP path.  Restore xcr0 from the per-cpu shadow copy.
> 
> The FS/GS base values are unused in Xen context, and will be loaded
> appropriately by the next vcpu context switch.
> 
> Trim the include list substantially, as most are unused.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

Reviewed-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Wei Liu <wl@xxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> ---
>  xen/arch/x86/acpi/suspend.c     | 54 
> +++--------------------------------------
>  xen/arch/x86/acpi/wakeup_prot.S |  2 --
>  2 files changed, 3 insertions(+), 53 deletions(-)
> 
> diff --git a/xen/arch/x86/acpi/suspend.c b/xen/arch/x86/acpi/suspend.c
> index 32d0f71ffd..629d117965 100644
> --- a/xen/arch/x86/acpi/suspend.c
> +++ b/xen/arch/x86/acpi/suspend.c
> @@ -4,40 +4,8 @@
>   *  Copyright (c) 2001 Patrick Mochel <mochel@xxxxxxxx>
>   */
>  
> -#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/system.h>
>  #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;
> -
> -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();
> -}
> -
>  
>  void restore_rest_processor_state(void)
>  {
> @@ -46,25 +14,9 @@ void restore_rest_processor_state(void)
>      /* Restore full CR4 (inc MCE) now that the IDT is in place. */
>      write_cr4(mmu_cr4_features);
>  
> -    /* 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);
> -
> -    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);
> -    }
> +    percpu_traps_init();
>  
> -    if ( cpu_has_xsave && !set_xcr0(saved_xcr0) )
> +    if ( cpu_has_xsave && !set_xcr0(get_xcr0()) )
>          BUG();
>  
>      wrmsrl(MSR_IA32_CR_PAT, XEN_MSR_PAT);

Given what this functions does after this change, would it be feasible
to place such calls directly in enter_state?

AFAICT there's already some restoring done there anyway.

Thanks, Roger.

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