Re: [PATCH] x86/S3: Restore CR4 earlier during resume

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Fri, Oct 02, 2020 at 10:36:50PM +0100, Andrew Cooper wrote:
> c/s 4304ff420e5 "x86/S3: Drop {save,restore}_rest_processor_state()
> completely" moved CR4 restoration up into C, to account for the fact that MCE
> was explicitly handled later.
> 
> However, time_resume() ends up making an EFI Runtime Service call, and EFI
> explodes without OSFXSR, presumably when trying to spill %xmm registers onto
> the stack.
> 
> Given this codepath, and the potential for other issues of a similar kind (TLB
> flushing vs INVPCID, HVM logic vs VMXE, etc), restore CR4 in asm before
> entering C.
> 
> Ignore the previous MCE special case, because its not actually necessary.  The
> handler is already suitably configured from before suspend.
> 
> Fixes: 4304ff420e5 ("x86/S3: Drop {save,restore}_rest_processor_state() 
> completely")
> Reported-by: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>

This one doesn't build, wakeup_prot.S misses #include <asm/asm_defns.h>
. With that fixed:

Tested-by: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>

> ---
> CC: Jan Beulich <JBeulich@xxxxxxxx>
> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> CC: Wei Liu <wl@xxxxxxx>
> CC: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
> 
> This is one definite bug fix.  It doesn't appear to be the only S3 bug
> however.
> ---
>  xen/arch/x86/acpi/power.c       | 3 ---
>  xen/arch/x86/acpi/wakeup_prot.S | 5 +++++
>  2 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/x86/acpi/power.c b/xen/arch/x86/acpi/power.c
> index 4fb1e7a148..7f162a4df9 100644
> --- a/xen/arch/x86/acpi/power.c
> +++ b/xen/arch/x86/acpi/power.c
> @@ -276,9 +276,6 @@ static int enter_state(u32 state)
>  
>      mcheck_init(&boot_cpu_data, false);
>  
> -    /* 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);
>  
>      if ( (state == ACPI_STATE_S3) && error )
> diff --git a/xen/arch/x86/acpi/wakeup_prot.S b/xen/arch/x86/acpi/wakeup_prot.S
> index c6b3fcc93d..1ee5551fb5 100644
> --- a/xen/arch/x86/acpi/wakeup_prot.S
> +++ b/xen/arch/x86/acpi/wakeup_prot.S
> @@ -110,6 +110,11 @@ ENTRY(s3_resume)
>  
>          call    load_system_tables
>  
> +        /* Restore CR4 from the cpuinfo block. */
> +        GET_STACK_END(bx)
> +        mov     STACK_CPUINFO_FIELD(cr4)(%rbx), %rax
> +        mov     %rax, %cr4
> +
>  .Lsuspend_err:
>          pop     %r15
>          pop     %r14

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?

Attachment: signature.asc
Description: PGP signature