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