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....@citrix.com> 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
