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

["Oleksii K." <oleksii.kurochko@gmail.com>]

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@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.kurochko@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();
>  }
>  
>  /**