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 02:50:18PM +0100, Andrew Cooper wrote:
> On 14/05/2024 12:09 pm, Andrew Cooper wrote:
> > On 13/05/2024 9:59 am, Roger Pau Monne wrote:
> >> There's no point in forcing a system wide update of the MTRRs on all 
> >> processors
> >> when there are no changes to be propagated.  On AP startup it's only the AP
> >> that needs to write the system wide MTRR values in order to match the rest 
> >> of
> >> the already online CPUs.
> >>
> >> We have occasionally seen the watchdog trigger during `xen-hptool 
> >> cpu-online`
> >> in one Intel Cascade Lake box with 448 CPUs due to the re-setting of the 
> >> MTRRs
> >> on all the CPUs in the system.
> >>
> >> While there adjust the comment to clarify why the system-wide resetting of 
> >> the
> >> MTRR registers is not needed for the purposes of mtrr_ap_init().
> >>
> >> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> >> ---
> >> For consideration for 4.19: it's a bugfix of a rare instance of the 
> >> watchdog
> >> triggering, but it's also a good performance improvement when performing
> >> cpu-online.
> >>
> >> Hopefully runtime changes to MTRR will affect a single MSR at a time, 
> >> lowering
> >> the chance of the watchdog triggering due to the system-wide resetting of 
> >> the
> >> range.
> > "Runtime" changes will only be during dom0 boot, if at all, but yes - it
> > is restricted to a single MTRR at a time.
> >
> > It's XENPF_{add,del,read}_memtype, but it's only used by Classic Linux. 
> > PVOps only issues read_memtype.
> >
> > Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> 
> Actually no - this isn't safe in all cases.
> 
> There are BIOSes which get MTRRs wrong, and with the APs having UC
> covering a wider region than the BSP.
> 
> In this case, creating consistency will alter the MTRRs on all CPUs
> currently up, and we do need to perform the rendezvous in that case.

I'm confused, the state that gets applied in mtrr_set_all() is not
modified to match what's in the started AP registers.

An AP starting with a different set of MTRR registers than the saved
state will result in the MTRR state on the AP being changed, but not
the Xen state stored in mtrr_state, and hence there will be no changes
to synchronize.

> There are 3 cases:
> 
> 1) Nothing to do.  This is the overwhemlingly common case.
> 2) Local changes only.  No broadcast, but we do need to enter CD mode.
> 3) Remote changes needed.  Needs full broadcast.

Please bear with me, but I don't think 3) is possible during AP
bringup.  It's possible I'm missing a path where the differences in
the started AP MTRR state are somehow reconciled with the cached MTRR
state?

Thanks, Roger.