Re: S3 resume issue in xstate_init

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

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

Jan