On Mon, 2024-05-13 at 10:59 +0200, 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....@citrix.com>
> ---
> 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.
Considering it as a bugfix it will be good to have it in 4.19 release:
Release-acked-by: Oleksii Kurochko <oleksii.kuroc...@gmail.com>
~ Oleksii
> ---
> xen/arch/x86/cpu/mtrr/main.c | 15 ++++++++-------
> 1 file changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/xen/arch/x86/cpu/mtrr/main.c
> b/xen/arch/x86/cpu/mtrr/main.c
> index 90b235f57e68..0a44ebbcb04f 100644
> --- 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();
> }
>
> /**
