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

[Jan Beulich <jbeulich@xxxxxxxx>]

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