[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] x86/MTRR: hook mtrr_bp_restore() back up
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. Possibly with that somehow noted in the commit message: Acked-by: Roger Pau Monné <roger.pau@xxxxxxxxxx> Thanks, Roger.
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |