Messages in this thread | | | Date | Wed, 12 Apr 2023 14:56:33 +0200 | From | Juergen Gross <> | Subject | Re: [PATCH v5 08/15] x86/mtrr: have only one set_mtrr() variant |
| |
On 12.04.23 14:30, Borislav Petkov wrote: > On Sat, Apr 01, 2023 at 08:36:45AM +0200, Juergen Gross wrote: >> Today there are two variants of set_mtrr(): one calling stop_machine() > > "... two variants which set MTRRs: set_mtrr() and set_mtrr_cpuslocked(), > first calling ..." > >> and one calling stop_machine_cpuslocked(). >> >> The first one (set_mtrr()) has only one caller, and this caller is >> always running with only one CPU online and interrupts being off. > > Wait, whaaat? > > It's only caller is mtrr_restore() and that is part of syscorse ops > which is registered for > > "The CPU has no MTRR and seems to not support SMP." > > Do you mean that, per chance?
I'm not sure the comment is accurate ("has no MTRR" is a bad way to say that X86_FEATURE_MTRR is 0, BTW).
The main point is that mtrr_restore() is called with interrupts off and before any other potential CPUs are started again.
> If you do, please explain that properly in the commit message - this is > not a guessing game.
Okay.
> By the looks of that syscore thing, it is needed for the very old MTRR > implementations which weren't SMP (K6, Centaur, Cyrix etc). > > Please explain that in the commit message too. It needs to say *why* the > transformation you're doing is ok.
I've outlined the "why" above. I'll replace the paragraph you were referring to.
> > "this caller is always running with only one CPU online" is not nearly > beginning to explain what the situation is. > >> Remove the first variant completely and replace the call of it with >> a call of mtrr_if->set(). >> >> Rename the second variant set_mtrr_cpuslocked() to set_mtrr() now that >> there is only one variant left. > > Those are visible from the diff itself - you don't need to explain what > the patch does but why.
Okay, I'll drop that paragraph.
Juergen [unhandled content-type:application/pgp-keys][unhandled content-type:application/pgp-signature] | |