On 27/03/2025 11:03 am, Jan Beulich wrote: > On 27.03.2025 11:53, Roger Pau Monné wrote: >> On Thu, Mar 27, 2025 at 10:54:23AM +0100, Jan Beulich wrote: >>> mtrr_set_all() has quite a bit of overhead, which is entirely useless >>> when set_mtrr_state() really does nothing. Furthermore, with >>> mtrr_state.def_type never initialized from hardware, post_set()'s >>> unconditional writing of the MSR means would leave us running in UC >>> mode after the sync. >>> >>> Signed-off-by: Jan Beulich <[email protected]> >>> >>> --- a/xen/arch/x86/cpu/mtrr/main.c >>> +++ b/xen/arch/x86/cpu/mtrr/main.c >>> @@ -605,13 +605,15 @@ void mtrr_aps_sync_begin(void) >>> >>> void mtrr_aps_sync_end(void) >>> { >>> - set_mtrr(~0U, 0, 0, 0); >>> + if (mtrr_if) >>> + set_mtrr(~0U, 0, 0, 0); >>> hold_mtrr_updates_on_aps = 0; >>> } >>> >>> void mtrr_bp_restore(void) >> Maybe I'm blind, but I cannot find any caller to mtrr_bp_restore()? >> Am I missing something obvious? > You don't. It was lost in 4304ff420e51 ("x86/S3: Drop > {save,restore}_rest_processor_state() completely"), with there being no > indication in the description that this was actually intentional. Looks like > another S3 regression we need to fix. Unless you, Andrew, have an explanation > for this.
Hmm, I don't think I intended to make a change without discussing it. However, I think I'd concluded that it was redundant with the mtrr_aps_sync_end() call. ~Andrew
