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

[Jan Beulich <jbeulich@xxxxxxxx>]

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