Re: [PATCH 1/3] x86/S3: Use percpu_traps_init() rather than opencoding SYSCALL/SYSENTER restoration

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 21/04/2020 08:24, Jan Beulich wrote:
> On 20.04.2020 16:59, Andrew Cooper wrote:
>> @@ -46,24 +36,13 @@ 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);
>> +    /* (re)initialise SYSCALL/SYSENTER state, amongst other things. */
>> +    percpu_traps_init();
> Without tweaks to subarch_percpu_traps_init() this assumes that,
> now and going forward, map_domain_page() will work reliably at
> this early point of resume. I'm not opposed, i.e.
> Acked-by: Jan Beulich <jbeulich@xxxxxxxx>

I think this reasonable to expect, and robust going forward.

We are properly in d[IDLE]v0 context when it comes to pagetables, and
there is nothing interesting between this point and coming fully back
online.

That said, I could easily move the call to later in the resume path for
even more certainty.

diff --git a/xen/arch/x86/acpi/power.c b/xen/arch/x86/acpi/power.c
index 3ad7dfc9a3..d5a468eddd 100644
--- a/xen/arch/x86/acpi/power.c
+++ b/xen/arch/x86/acpi/power.c
@@ -297,6 +297,8 @@ static int enter_state(u32 state)
     ci->spec_ctrl_flags |= (default_spec_ctrl_flags & SCF_ist_wrmsr);
     spec_ctrl_exit_idle(ci);
 
+    /* (re)initialise SYSCALL/SYSENTER state, amongst other things. */
+    percpu_traps_init();
+
  done:
     spin_debug_enable();
     local_irq_restore(flags);

In fact - I prefer this, because it works towards one low priority goal
of deleting {save,restore}_rest_processor_state() which I've still got a
pending series for.

Would your ack still stand if I tweak the patch in this way?

> but it would feel less fragile if the redundant re-writing of
> the stubs would be skipped in the BSP resume case (I didn't
> check whether it's also redundant for APs, but I suspect it's
> not, as the stub pages may get allocated anew).

I don't really agree.  Symmetry (even if it is expected to be redundant)
is much more easy to reason about in terms of robustness.  S3 is not a
fastpath.

~Andrew