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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 14/05/2024 3:43 pm, Roger Pau Monné wrote:
> On Tue, May 14, 2024 at 02:50:18PM +0100, Andrew Cooper wrote:
>> On 14/05/2024 12:09 pm, Andrew Cooper wrote:
>>> On 13/05/2024 9:59 am, 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.
>>> "Runtime" changes will only be during dom0 boot, if at all, but yes - it
>>> is restricted to a single MTRR at a time.
>>>
>>> It's XENPF_{add,del,read}_memtype, but it's only used by Classic Linux. 
>>> PVOps only issues read_memtype.
>>>
>>> Acked-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>> Actually no - this isn't safe in all cases.
>>
>> There are BIOSes which get MTRRs wrong, and with the APs having UC
>> covering a wider region than the BSP.
>>
>> In this case, creating consistency will alter the MTRRs on all CPUs
>> currently up, and we do need to perform the rendezvous in that case.
> I'm confused, the state that gets applied in mtrr_set_all() is not
> modified to match what's in the started AP registers.
>
> An AP starting with a different set of MTRR registers than the saved
> state will result in the MTRR state on the AP being changed, but not
> the Xen state stored in mtrr_state, and hence there will be no changes
> to synchronize.
>
>> There are 3 cases:
>>
>> 1) Nothing to do.  This is the overwhemlingly common case.
>> 2) Local changes only.  No broadcast, but we do need to enter CD mode.
>> 3) Remote changes needed.  Needs full broadcast.
> Please bear with me, but I don't think 3) is possible during AP
> bringup.  It's possible I'm missing a path where the differences in
> the started AP MTRR state are somehow reconciled with the cached MTRR
> state?

[Summarising a conversation on Matrix]

The problem case is when the BSP has an MTRR covering [$X, $X+2) and an
AP has has an MTRR covering [$X, $X+3).

This is a firmware bug (asymmetric settings), but it has been observed
in practice.  The resolution in this case ought to be to update all CPUs
to use [$X, $X+3), if that is the more UC direction.

However, it appears that Xen always resolves asymmetry like this by
choosing the BSP setting.  Therefore, (whether we should or not), we
don't have a case where observing an AP state results in a change of
state on other CPUs.

Therefore while case 3 exists in reality, we're not losing it as a side
effect of this patch.

So we'll take the improvement here and defer the other bugs to a future
juncture.

~Andrew