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 <[email protected]> >>> 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
