| Message ID | 56fbfae0-aac7-4841-ab3c-a7e00dda3744@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86/MTRR: constrain AP sync and BSP restore | expand |
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 <jbeulich@suse.com> > > --- 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? Thanks, Roger.
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 <jbeulich@suse.com> >> >> --- 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. Jan
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 <jbeulich@suse.com> >>> >>> --- 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
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 <jbeulich@suse.com> Acked-by: Roger Pau Monné <roger.pau@citrix.com> Conditional on reaching consensus on whether the mtrr_bp_restore() needs re-adding to the resume path. Otherwise the code needs to be removed. Thanks, Roger.
--- 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) { - mtrr_set_all(); + if (mtrr_if) + mtrr_set_all(); } static int __init cf_check mtrr_init_finialize(void)
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 <jbeulich@suse.com>