Re: [PATCH v5 08/15] x86/mtrr: have only one set_mtrr() variant

From: Juergen Gross
Date: Wed Apr 12 2023 - 08:57:25 EST

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

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature