Re: S3 resume issue in xstate_init

[Jan Beulich <jbeulich@xxxxxxxx>]

On 17.08.2021 15:29, Andrew Cooper wrote:
> 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 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.

Would you mind explaining why? I did consider doing so, but didn't
see the point when the first thing we do is write the register with
xfeature_mask (which can't be zero, and which hence would always get
written through to the register when the cached value is zero).

>  %xcr0 is possibly the only lazy register we also do
> extra sanity checks on.

I don't think we do?

(All of this is independent of my "x86/xstate: replace xsave_cntxt_size
and drop XCNTXT_MASK" I think, despite its reworking of the logic, and
which I'm still hoping to get reviewed at some point.)

Jan