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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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.

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

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.

Thinking about it some more, we load cpuidmask_defaults in IDLE and HVM
context, while PV guests (even PV dom0) will have non-default
cpuidmask's, so with a PV dom0, things ought to correct themselves
fairly promptly after S3, but not as early as we expect.

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

I think it might be time to expand the "MSR consistency across the
system" logic from test-tsx to rather more MSRs..

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

Regular firmware I'd expect it to be 0.  Linuxboot, I'd expect whatever
value Linux last left in there for userspace.

~Andrew