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

[Marek Marczykowski-Górecki <marmarek@xxxxxxxxxxxxxxxxxxxxxx>]

On Thu, Oct 21, 2021 at 03:44:27PM +0200, Roger Pau Monné wrote:
> On Mon, Oct 18, 2021 at 10:21:28AM +0200, 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);
> > > cpu/common.c:120:static DEFINE_PER_CPU(uint64_t, msr_misc_features);
> > > msr.c:35:DEFINE_PER_CPU(uint32_t, tsc_aux);
> > > xstate.c:36:static DEFINE_PER_CPU(uint64_t, xcr0);
> > > xstate.c:79:static DEFINE_PER_CPU(uint64_t, xss);
> > > 
> > > There is also:
> > > 
> > > traps.c:100:DEFINE_PER_CPU(uint64_t, efer);
> > > 
> > > which we *almost* handle correctly, but fail to update the cache on the
> > > BSP out of S3.
> > > 
> > > 
> > > For the APIC, I think we have issues with:
> > > 
> > > irq.c:1083:static DEFINE_PER_CPU(struct pending_eoi,
> > > pending_eoi[NR_DYNAMIC_VECTORS]);
> > > 
> > > because we don't defer S3 until all pending EOIs are complete.
> > 
> > As your planned more extensive rework appears to not have made much
> > progress yet, may I suggest that we go with Marek's fix for 4.16,
> > with the one adjustment I suggested alongside giving my R-b?
> 
> I think that's the only viable solution in order to avoid shipping a
> broken 4.16 so we should go ahead with it.

Do you want me to post v2 with `this_cpu(xss) = ~0` change? IIUC that's
the only thing requested in this patch specifically.

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

Attachment: signature.asc
Description: PGP signature