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

Re: [PATCH] x86/xstate: reset cached register values on resume

  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • Date: Wed, 25 Aug 2021 17:49:27 +0100
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=citrix.com; dmarc=pass action=none header.from=citrix.com; dkim=pass header.d=citrix.com; arc=none
  • Arc-message-signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=w2J6Iia5k5bATlDvnLUA0oX+Q7p+at0L6fvq3tynrx8=; b=TxoHoY4g4bps3NxATh0LbfK+MXATRnjBJbDacBxJwBthMWJrSQxo/ZWj9ZsDbLbMQSczmjltJDUNj8ek5Q5RGkOqrOIrNys0HWN54raNCDbMApFj1jzc4btkit9xO07YSLsmNNTqx3ie12LKMXcqQmxmLGD0HA70Hby37xuQgyXI6AgpiEIJ22+oeq6Y0rN6teCaarCPX1e2Tx99nPNpy/k0PZB0dI6935DRvzztlnCGGHGB7JXno8Ox/ETCpHVRG+e7svKpp2EXEg8+DLUdFoZvTAeVPFeGLK5X6aI9TtpiY+Qu7zMWMYQ+4DQQv0jpYEuYtfbIzfre2bzEH3L2gg==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=JLXQXEr0b7i93cNMorp8gQtHdh42aCbrZyMln6i2E8J0b9ML2/724Rz1A1hUX6hYZoBogJIZBqgZ5EaVczA2Lng85kfO96oqlByc11RZJhazr+hHyi8IRLCKICe/YuEHVzzTRMIQdNHdpvVUIR2bI3++vIAz7SmIQu+4bfou46umX6d1KuwGjRDmuJO1PvBT0sgRRIWuFbcWZgyKzw2y3PoYAJb51HXW9y8demUtkctEzKn1VmSDUJ38/BJQ/PhvWUtOFLGa9+/KIwQi7RbmAf94OxhncyXQw7ma2l58w1W9jb5oLQX8fe51+miqPcwknz0G1afZ76g+wjoDWNBD7w==
  • Authentication-results: esa3.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: Roger Pau Monné <roger.pau@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>, <xen-devel@xxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Wed, 25 Aug 2021 16:49:48 +0000
  • Ironport-hdrordr: A9a23:YWeNZ6kHVelEUqtXRczuHIy4xQHpDfOiimdD5ihNYBxZY6Wkfp +V88jzhCWZtN9OYhwdcLC7WZVpQRvnhPtICPoqTMiftW7dyReVxeBZnPbfKljbdREWmdQtrZ uIH5IOb+EYSGIK9/oSgzPIY+rIouP3iZxA7N22pxwGLXAIGtNdBkVCe2Km+yVNNXh77PECZf yhD6R81lidkDgsH7+G7i5vZZm8mzSHruOoXTc2QzocrCWehzKh77D3VzCewxclSjtKhZMv63 LMnQDV7riq96jT8G6c60bjq7Bt3PfxwNpKA8KBzuATNzXXkw6tIKBsQaeLsjwZqPymrHwqjN 7PiRE9ONkb0QKeQkiF5T/WnyXw2jcn7HHvjXeenHvYuMT8AAk3DsJQ7LgpOCfx2g4FhpVRwa hL12WWu958FhXbhhnw4NDOSlVDile0iWBKq59Qs1VvFa8lLJNBp40W+01YVL0aGjjh1YwhGO 5ySOnB+fdtd0+AZXyxhBgt/DWVZAV2Iv66eDlEhiTMuAIm2kyRjnFohPD3p01wsa7UEPJ/lr 352s0CrsA8cicUBZgNT9vpD/HHUlAk7Hr3QRSvyG/cZdU60kT22tbKCYUOlZSXkaMzvewPcb T6IR5lXD0JCg7T4fPn5uwDzvmKehTnYQjQ
  • Ironport-sdr: r0kEU9FsFgKc61yHRkhMP+HtYCRtl8aCogwLxg4wXuN2wugHDnG8eRoZphQ+hMaBzecQjI98Ty Zi4iNC/p0ENdVy7zFmlStBg+a/IoLHn8o3KAtJQKVtaelC98gAc/0fVkisM6SCRjmZJlZECIHk wtO2xiaSoGZQW/dM8nUxJwQeRalsYemx0wbfjuoa/2Qk0lprIo2dsHa/VkG7Woe5N6qgxg19/q e5/Qa2ABCLKj/fn6lrvgEu1Nrbf1Da5ropGaBIOmxgUL3yvv0MMUdMOqv9VETtUgW1mY26e0Z9 Fdi8Oi3pDLgDqib+sqV2vE7x
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 25/08/2021 16:02, Jan Beulich wrote:
> On 24.08.2021 23:11, Andrew Cooper wrote:
>> On 18/08/2021 13:44, Andrew Cooper wrote:
>>> On 18/08/2021 12:30, Marek Marczykowski-Górecki wrote:
>>>> set_xcr0() and set_msr_xss() use cached value to avoid setting the
>>>> register to the same value over and over. But suspend/resume implicitly
>>>> reset the registers and since percpu areas are not deallocated on
>>>> suspend anymore, the cache gets stale.
>>>> Reset the cache on resume, to ensure the next write will really hit the
>>>> hardware. Choose value 0, as it will never be a legitimate write to
>>>> those registers - and so, will force write (and cache update).
>>>> Note the cache is used io get_xcr0() and get_msr_xss() too, but:
>>>> - set_xcr0() is called few lines below in xstate_init(), so it will
>>>>   update the cache with appropriate value
>>>> - get_msr_xss() is not used anywhere - and thus not before any
>>>>   set_msr_xss() that will fill the cache
>>>> Fixes: aca2a985a55a "xen: don't free percpu areas during suspend"
>>>> Signed-off-by: Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
>>> I'd prefer to do this differently.  As I said in the thread, there are
>>> other registers such as MSR_TSC_AUX which fall into the same category,
>>> and I'd like to make something which works systematically.
>> Ok - after some searching, I think we have problems with:
>> cpu/common.c:47:DEFINE_PER_CPU(struct cpuidmasks, cpuidmasks);
> Don't we have a problem here even during initial boot? I can't see
> the per-CPU variable to get filled by what the registers hold.

No, I don't think so, but it is a roundabout route.

>  If
> the register started out non-zero (the default on AMD iirc, as it's
> not really masks there) but the first value to be written was zero,
> we'd skip the write.

There is cpuidmask_defaults which does get filled from the MSRs on boot.

AMD are real CPUID settings, while Intel is an and-mask.  i.e. they're
both non-zero (unless someone does something silly with the command line
arguments, and I don't expect Xen to be happy booting if the leaves all
read 0).

Each early_init_*() has an explicit ctxt_switch_levelling(NULL) call
which, given non-zero content in cpuidmask_defaults should latch each
one appropriately in the the per-cpu variable.

Thinking about it some more, we load cpuidmask_defaults in IDLE and HVM
context, while PV guests (even PV dom0) will have non-default
cpuidmask's, so with a PV dom0, things ought to correct themselves
fairly promptly after S3, but not as early as we expect.

>> cpu/common.c:120:static DEFINE_PER_CPU(uint64_t, msr_misc_features);
> Almost the same here - we only initialize the variable on the BSP
> afaics.

No - way way way worse, I think.

For all APs, we write 0 or MSR_MISC_FEATURES_CPUID_FAULTING into
MSR_INTEL_MISC_FEATURES_ENABLES, which amongst other things turns off
Fast String Enable.

I think it might be time to expand the "MSR consistency across the
system" logic from test-tsx to rather more MSRs..

>> msr.c:35:DEFINE_PER_CPU(uint32_t, tsc_aux);
> And again no boot time setup at all for this one as it looks. Not 
> sure how likely it is for firmware or bootloaders to use this MSR
> (and then leave it non-zero).

Regular firmware I'd expect it to be 0.  Linuxboot, I'd expect whatever
value Linux last left in there for userspace.




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