Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 12 Apr 2023 14:30:44 +0200
From:   Borislav Petkov <bp@alien8.de>
To:     Juergen Gross <jgross@suse.com>
Cc:     linux-kernel@vger.kernel.org, x86@kernel.org,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        "H. Peter Anvin" <hpa@zytor.com>
Subject: Re: [PATCH v5 08/15] x86/mtrr: have only one set_mtrr() variant
Message-ID: <20230412123044.GJZDakdLatRW26J+1k@fat_crate.local>
References: <20230401063652.23522-1-jgross@suse.com>
 <20230401063652.23522-9-jgross@suse.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <20230401063652.23522-9-jgross@suse.com>
Precedence: bulk

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?

If you do, please explain that properly in the commit message - this is
not a guessing game.

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.

"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.

Thx.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette