Re: S3 resume issue in xstate_init

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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