Re: Yet another S3 issue in Xen 4.14

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 02/10/2020 16:39, Jan Beulich wrote:
> On 02.10.2020 17:08, Marek Marczykowski-Górecki wrote:
>> I've done another bisect on the commit broken up in separate changes
>> (https://xenbits.xen.org/gitweb/?p=people/andrewcoop/xen.git;a=shortlog;h=refs/heads/dbg-s3)
>> and the bad part seems to be this:
>>
>> From dbdb32f8c265295d6af7cd4cd0aa12b6d04a0430 Mon Sep 17 00:00:00 2001
>> From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> Date: Fri, 2 Oct 2020 15:40:22 +0100
>> Subject: [PATCH 1/1] CR4
> Interesting - I was wild guessing so yesterday, but couldn't come
> up with even a vague reason why this would be. I think you could
> further split it up:
>
>> --- 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());
> This one should be possible to leave in place despite ...
>
>>      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());
> ... this change.
>
> Further, while I can't see how the set_in_cr4() in mcheck_init()
> could badly interact with the CR4 writes here, another option
> might be to suppress it when system_state == SYS_STATE_resume
> && c == &boot_cpu_data (or !bsp && c == &boot_cpu_data).
>
>> --- a/xen/arch/x86/acpi/suspend.c
>> +++ b/xen/arch/x86/acpi/suspend.c
>> @@ -23,7 +23,4 @@ void save_rest_processor_state(void)
>>  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);
>>  }
> This one should be possible to leave in place despite the other
> changes.

We're continuing to debug in private.  mmu_cr4_features and read_cr4()
are equivalent (as expected), but very different from MINIMAL_CR4 which
is what the trampoline configures, so I think we're suffering a
CR4-related #UD/#GP somewhere in device_power_up() or mcheck_init().

Its not INVPCID.  Trying others.

~Andrew