Re: [PATCH for-4.19] x86/mtrr: avoid system wide rendezvous when setting AP MTRRs

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.