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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

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

> msr.c:35:DEFINE_PER_CPU(uint32_t, tsc_aux);

And again no boot time setup at all for this one as it looks. Not 
sure how likely it is for firmware or bootloaders to use this MSR
(and then leave it non-zero).

Jan