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

Re: S3 resume issue in xstate_init


  • To: Jan Beulich <jbeulich@xxxxxxxx>
  • From: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
  • Date: Tue, 17 Aug 2021 14:29:20 +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=wmSTbMcyqFYQARv6Xx0iZ9QnEzqoQ/xS14oClgrkyrU=; b=eiBx0s30Et7bZmhhged1MIv1o16qr6m5mFNJ6RfrCZDxCXX2eUkpAZ9DQlgcQkjogK9WPQGxILx/o3BmcSWMaHm1NGvdr4v/u4YVy2dIAD+dXEx8kAwoiKKzJbsJC7d09wFa2vIY3T7GQVe+XEHWfBaalCvZ7DAisCkIbEyKzG9M8pggHrALSCfNmfDZTougq6dZFd+IX5uSnWxqrdLMIjn9NEFN5obFr+3dZVfFfPUi+ODCgKthB9jK2YcbCRhcHhXmVxFt5AhYUhDOv1dMo+l83ChDWqCpMh5k7a4/267W47c9a40f8lIlsAAEuPyKHy9FBg6aWvG4jONXOLGULw==
  • Arc-seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=VaMJlgDNmkU/2UP6UfaPE/o2FN67QsuU+pLS0UEtsBM835UdgPL0MS5x3Mmqjf8zWted5k/KtYqc+x8JAYEJujE1Ai/hRe3dBa0YO1DY0wYC6AD8yEeQg8JJ1odlOoVWMJYgvLseYXZCjcOBIYBy21kGO6IJ5uyOEYFkuNBciuDdT4nKVZHheuGbIk9yJxz1urjjuGP91VgT5qVqtlcyYrtR2k0gHfMbNbFM26uo3QQCQdoW2/ff6ePXmJoeZF4S+2Zz30agUTLh5EsjTX+nm1KifFNFKAwLGH3qZrQN0/dfpJuUH4NHed6RfzKexhQSb/8ItuyiSg+yfq0+2USf8Q==
  • Authentication-results: esa1.hc3370-68.iphmx.com; dkim=pass (signature verified) header.i=@citrix.onmicrosoft.com
  • Cc: xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>, Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>
  • Delivery-date: Tue, 17 Aug 2021 13:29:47 +0000
  • Ironport-hdrordr: A9a23:S4dqtKjj1O6uMNk/0XymWXMVC3BQXiwji2hC6mlwRA09TyX5ra 2TdTogtSMc6QxhPk3I/OrrBEDuexzhHPJOj7X5eI3SPjUO21HYS72Kj7GSoAEIcheWnoJgPO VbAs1D4bXLZmSS5vyKhDVQfexA/DGGmprY+ts3zR1WPH9Xg3cL1XYJNu6ZeHcGNDWvHfACZe OhDlIsnUvcRZwQBP7LfkUtbqz4iPDgsonpWhICDw5P0njzsdv5gISKaCRxx30lIkly/Ys=
  • Ironport-sdr: UaP5wR5z87QXi2C9Zot0t/Gu8FA6hVO6V4JSCZxTEPjVzSP4JThGvqjRwETxIl3IwP72axNxC2 4YMweIsLHOILVQV/aV/RQ9Qyt1/RPrOk0ecoJ797n3haosm+GPxgOU85WKUspzxf6EU5xZOcp/ mVJKeSN7bRBRU1UNjAIS8C5cYUNOSOIbss+xqREpQuhPtRJHTXWubIgatf8Gayijr0DtEEjAP5 Q01Rbj8swEqjQB5KFt2EtoLfpJJAqEx8Z0+31lmH9CZv3oI+UIK5YU65qSmwc56lFN1afDQn7c qGjPjUnMd4LKjfHZ1WPbCFRh
  • List-id: Xen developer discussion <xen-devel.lists.xenproject.org>

On 17/08/2021 14:21, Jan Beulich wrote:
> On 17.08.2021 15:06, Andrew Cooper wrote:
>> On 17/08/2021 13:53, Andrew Cooper wrote:
>>> On 17/08/2021 13:21, Jan Beulich wrote:
>>>> I'm kind of guessing that set_xcr0() mistakenly skips the actual XCR0
>>>> write, due to the cached value matching the to-be-written one, yet
>>>> the cache having gone stale across S3.
>>> This is a rats nest, and area for concern, but ...
>>>
>>>>  I think this is to be expected
>>>> for previously parked CPUs, as those don't have their per-CPU data
>>>> de-allocated (and hence also not re-setup, and thus also not starting
>>>> out as zero).
>>> ... we don't deallocate regular APs either, so I don't see why the smt=
>>> setting would make a difference in this case.
>>>
>>> To be clear - I think your guess about set_xcr0() skipping the write is
>>> correct, because 0x240 is correct for xcr0=X87, but I don't see why smt=
>>> makes a difference.
> Right - as per my later reply to Marek I don't see an immediate reason
> anymore either. I could see an indirect reason via different scheduler
> decisions, affecting what ran last on the respective CPUs.

Modern Linux has stripped all MPX support, so won't set
%xcr0.bnd{reg,csr} in the first place, and will differ from Xen's
default setting.

Therefore, I suppose we have a real difference in Xen's idea of the
lazily-cached value depending on whether we're in half or full idle context.

>>>>  I guess an easy fix would be to write 0 to
>>>> this_cpu(xcr0) directly early in xstate_init(), maybe in an "else"
>>>> to the early "if ( bsp )".
>>>>
>>>> I'm not sure though this would be a good fix longer term, as there
>>>> might easily be other similar issues elsewhere. IOW we may need to
>>>> see whether per-CPU data initialization wouldn't want changing.
>>> We've got other registers too, like MSR_TSC_AUX, but I don't think we
>>> want to be doing anything as drastic as changing how the initialisation
>>> works.
>>>
>>> The S3 path needs to explicitly write every register we do lazy context
>>> switching of.
>> Actually no - that's a dumb suggestion because the APs don't know
>> better, and we don't want for_each_cpu() loops running from the BSP.
>>
>> Perhaps we want the cpu_down() logic to explicitly invalidate their
>> lazily cached values?
> I'd rather do this on the cpu_up() path (no point clobbering what may
> get further clobbered, and then perhaps not to a value of our liking),
> yet then we can really avoid doing this from a notifier and instead do
> it early enough in xstate_init() (taking care of XSS at the same time).

What we actually want to do is read the hardware register into the
cached location.  %xcr0 is possibly the only lazy register we also do
extra sanity checks on.

~Andrew



 


Rackspace

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