Re: [PATCH] x86/MTRR: hook mtrr_bp_restore() back up

[Jan Beulich <jbeulich@xxxxxxxx>]

On 27.03.2025 15:10, Roger Pau Monné wrote:
> On Thu, Mar 27, 2025 at 02:28:42PM +0100, Jan Beulich wrote:
>> On 27.03.2025 13:48, Roger Pau Monné wrote:
>>> On Thu, Mar 27, 2025 at 01:30:44PM +0100, Jan Beulich wrote:
>>>> On 27.03.2025 12:38, Roger Pau Monné wrote:
>>>>> On Thu, Mar 27, 2025 at 12:20:47PM +0100, Jan Beulich wrote:
>>>>>> Unlike stated in the offending commit's description,
>>>>>> load_system_tables() wasn't the only thing left to retain from the
>>>>>> earlier restore_rest_processor_state().
>>>>>>
>>>>>> While there also do Misra-related tidying for the function itself: The
>>>>>> function being used from assembly only means it doesn't need to have a
>>>>>> declaration, but wants to be asmlinkage.
>>>>>
>>>>> I wonder, maybe the intention was for the MTRR restoring on the BSP to
>>>>> also be done by the mtrr_aps_sync_end() call in enter_state()?
>>>>>
>>>>> AFAICT that will set the MTRRs uniformly on all CPUs, by calling
>>>>> mtrr_set_all() just like mtrr_bp_restore(), but later in the restore
>>>>> process.
>>>>
>>>> Hmm, yes, that's possible. The comment in set_mtrr() is somewhat misleading
>>>> then, though, as for the BP the writing then isn't just "okay" but 
>>>> necessary.
>>>> Question is whether doing this so much later is actually good enough.
>>>
>>> Hm, no idea really.  We do the device restore ahead of the MTRR
>>> restore, so I wonder whether we could have issues by using unexpected
>>> effective cache attributes for device memory accesses as a result of
>>> MTRRs not being initialized?
>>
>> That's just one of the possible problems. The father the MTRRs we run with
>> diverged from what firmware puts in place, the bigger the possible trouble.
>> I think the restoring better is done as being switched to here again. The
>> absence of any discussion of MTRRs in that earlier change leaves me pretty
>> certain that the behavioral change there wasn't intended. Andrew is usually
>> pretty good at spelling out all intended effects.
> 
> No objection, however for the BSP we now end up restoring the MTRRs
> twice, as we will also do it in mtrr_aps_sync_end().
> 
> Might be worth to mention in the commit message that the MTRR state
> was restored in mtrr_aps_sync_end() for the BSP also, but that it
> might be too late.

I've added "Note that MTRR state was still reloaded via mtrr_aps_sync_end(),
but that happens quite a bit later in the resume process."

> Possibly with that somehow noted in the commit message:
> 
> Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>

Thanks. Any chance of getting another one for the 3rd patch in this (split
up) group? Maybe the duplicate one for the "don't hard-code" one was actually
meant to go there?

Jan