[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH for-4.19] x86/mtrr: avoid system wide rendezvous when setting AP MTRRs
On Tue, May 14, 2024 at 01:57:13PM +0200, Jan Beulich wrote: > On 13.05.2024 10:59, Roger Pau Monne wrote: > > --- a/xen/arch/x86/cpu/mtrr/main.c > > +++ b/xen/arch/x86/cpu/mtrr/main.c > > @@ -573,14 +573,15 @@ void mtrr_ap_init(void) > > if (!mtrr_if || hold_mtrr_updates_on_aps) > > return; > > /* > > - * Ideally we should hold mtrr_mutex here to avoid mtrr entries changed, > > - * but this routine will be called in cpu boot time, holding the lock > > - * breaks it. This routine is called in two cases: 1.very earily time > > - * of software resume, when there absolutely isn't mtrr entry changes; > > - * 2.cpu hotadd time. We let mtrr_add/del_page hold cpuhotplug lock to > > - * prevent mtrr entry changes > > + * hold_mtrr_updates_on_aps takes care of preventing unnecessary MTRR > > + * updates when batch starting the CPUs (see > > + * mtrr_aps_sync_{begin,end}()). > > + * > > + * Otherwise just apply the current system wide MTRR values to this AP. > > + * Note this doesn't require synchronization with the other CPUs, as > > + * there are strictly no modifications of the current MTRR values. > > */ > > - set_mtrr(~0U, 0, 0, 0); > > + mtrr_set_all(); > > } > > While I agree with the change here, it doesn't go quite far enough. Originally > I meant to ask that, with this (supposedly) sole use of ~0U gone, you please > also drop the handling of that special case from set_mtrr(). But another > similar call exist in mtrr_aps_sync_end(). Yet while that's "fine" for the > boot case (watchdog being started only slightly later), it doesn't look to be > for the S3 resume one: The watchdog is re-enabled quite a bit earlier there. > I actually wonder whether mtrr_aps_sync_{begin,end}() wouldn't better > themselves invoke watchdog_{dis,en}able(), thus also making the boot case > explicitly safe, not just safe because of ordering. Hm, I don't like disabling the watchdog, I guess it could be acceptable here because both usages of mtrr_aps_sync_end() are limited to specific scenarios (boot or resume from suspension). I can prepare a separate patch, but I don't think the watchdog disabling should be part of this patch. Thanks, Roger.
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |