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

[Jan Beulich <jbeulich@suse.com>]

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.

Jan