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

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


  • To: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • From: Jan Beulich <jbeulich@xxxxxxxx>
  • Date: Thu, 26 Aug 2021 09:40:34 +0200
  • Arc-authentication-results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=suse.com; dmarc=pass action=none header.from=suse.com; dkim=pass header.d=suse.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=A2f1ZJz7UGKiKdVtixzxUoCAeaTNNvJ3KIXZkRUv1yE=; b=E+SCvxcHCjIha4fLWaYbXAsRHGjSu2qPV56Tiov/J3xxgGRrB8/6r8JGDQqjYIc4xvkPfsDWz5vY1LtQcH7I/dGuL1+Sz//t4fufys+Z/NodF6z1LTVNf1IS32fSkAZztX7nXmHdqdJuF/E0QyQY31lVaCaUsJFB959o3baKstdEHSzweSQklqjdX3EXEFk1fc1pJ61YK2MDWCSqdpQHmQTPeIhod3wFkeXNovMoEFQ8+S4Fqw2tUMbno9XV7bNmhXef/Erm9FuzON/lYkSUS/1FMo1g2dgqAUcVodG2Kv933rwaEpu6OjABYGpp1jPMSr4RXIEIu1MrVsCTSSJXfQ==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=idSHL73UXfGoXJ9tNFJPbx0jWjs7dQJX8Zmm7TGaSJ0qvY0fUhuOGV7dFRU520Pq7BuamGQ6UhDLIrrBaRqlhZeWxkoV6MnEcOUjPhXQQYnqmr8r7Z0NoWxMbWZSUic5t8qB9ExVBl7k02ReKAvTihZ/mFj4zU1nhJV981TURz6aqxsMhMzHVuGTl20IhEjcXnxB153auYMqOTobTuOQn5VJKLyX18sD017AmimVufcLaOWm6PuS0t8fu0XMGl/RJn4QemgSTnZoFoLtFy8bjOrAXEildACHEYP5x9FFUo3IhDpr7hEXWYus8FVWUWMiHqz4GHVYWnT9St4HG03HcA==
  • Authentication-results: lists.xenproject.org; dkim=none (message not signed) header.d=none;lists.xenproject.org; dmarc=none action=none header.from=suse.com;
  • Cc: Roger Pau Monné <roger.pau@xxxxxxxxxx>, Wei Liu <wl@xxxxxxx>, Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>, xen-devel@xxxxxxxxxxxxxxxxxxxx
  • Delivery-date: Thu, 26 Aug 2021 07:40:55 +0000
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 25.08.2021 18:49, Andrew Cooper wrote:
> 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.

So where do you see it getting filled?

>>  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).

Surely not all of them together, but I don't think it's completely
unreasonable for one (say the XSAVE one, if e.g. XSAVE is to be turned
off altogether for guests) to be all zero.

> 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.

Well, as you say - provided the individual fields are all non-zero.

>>> 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.

Urgh, indeed. Prior to 6e2fdc0f8902 there was a read-modify-write
operation. With the introduction of the cache variable this went
away, while the cache gets filled for BSP only.

Jan




 


Rackspace

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