[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • Date: Thu, 23 Apr 2020 19:24:45 +0100
  • Authentication-results: esa5.hc3370-68.iphmx.com; dkim=none (message not signed) header.i=none; spf=None smtp.pra=andrew.cooper3@xxxxxxxxxx; spf=Pass smtp.mailfrom=Andrew.Cooper3@xxxxxxxxxx; spf=None smtp.helo=postmaster@xxxxxxxxxxxxxxx
  • Cc: Xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Roger Pau Monné <roger.pau@xxxxxxxxxx>
  • Delivery-date: Thu, 23 Apr 2020 18:24:58 +0000
  • Ironport-sdr: Oy3YT2v5iaX1icqMggkX0F81kutOMdVNB+pKUOJXDGRofviPmU3r3zHo7Mqz7+QcsBZXYvEfhm ZW3Ejix3Ol0aEXmwECU+wQhkXxpSbkq0zf/FKUUoD2RnY9XfEppbYVVryh1BN1qQMikNWP7Z3q aV4TUtsAgdLTtubludoWIlp1YOdbuv2VgqBccvx+ILUIwQ1nilfp6nXcridXxOZveFJSd4k4Fk slCFeaeVTCAdA9sDH3cBn/nLa/6G8z+4KdQUeKqVWfcZifvVsviHcdM3/pr2+oew/vf7aD9/U7 iLE=
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.