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

[Juergen Gross <jgross@suse.com>]

[-- Attachment #1.1.1: Type: text/plain, Size: 1892 bytes --]

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 #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 3149 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 495 bytes --]