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

["Oleksii K." <oleksii.kurochko@xxxxxxxxx>]

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.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.
Considering it as a bugfix it will be good to have it in 4.19 release:
 Release-acked-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>

~ 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();
>  }
>  
>  /**