Re: [PATCH v4 09/12] x86/mtrr: construct a memory map with cache modes

From: Juergen Gross <jgross@suse.com>
To: Borislav Petkov <bp@alien8.de>
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 v4 09/12] x86/mtrr: construct a memory map with cache modes
Date: Fri, 31 Mar 2023 15:23:13 +0200	[thread overview]
Message-ID: <8860c326-3ba5-bd4c-1dab-17772e8fcda0@suse.com> (raw)
In-Reply-To: <20230331125538.GBZCbYSqr8kMP4bpwS@fat_crate.local>

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

On 31.03.23 14:55, Borislav Petkov wrote:
> On Wed, Mar 29, 2023 at 03:39:35PM +0200, Juergen Gross wrote:
>> No. :-)
> 
> Because?

In general the critical case is add_map_entry_at() returning 2 (in the
case it is returning 1, the index can be set to -1, but there is always
the "continue" statement right after that, which would execute the "i++"
of the "for" statement).

add_map_entry_at() can return 2 only, if it detects "merge_prev" and
"merge_next". "merge_prev" can be set only if the current index was > 0,
which makes it impossible to return 2 if the index was 0.

>> The final form of the code is the result of an iterative process. :-)
> 
> I have a similar iterative process: until it hasn't been reviewed and
> explained properly, this is not going anywhere.

Of course.

> So however you wanna do it, fine by me.
> 
>> I've reused the wording from cleanup.c (just above amd_special_default_mtrr()).
> 
> That got added with K8. K8 is ancient history so nothing magic about
> that anymore. It is basically a bit in the SYSCFG MSR which says that
> 
> 	[4G ... TOP_MEM2]
> 
> is WB.

How should it be named? AMD TOP_MEM2 MSR?

>>> Why not in mtrr_bp_init()? That is the first CPU.
>>
>> Yeah, but generic_set_mtrr() can be called after boot, too.
> 
> That function sets a single MTRR register so you'd have to merge the
> ranges, AFAICT. Not rebuild the whole map...

The problem isn't an added MTRR register, but a possibly replaced or removed
one. Handling that is much more complicated, so I've chosen to do it the simple
way.

In the end I'd expect setting of MTRRs to be a rare event, so there shouldn't be
a performance issue with that approach.

> 
>> Umm, not really. I want to do the copy even in the Xen PV case.
> 
> How about some comments? Or you're expecting me to be able to read your
> mind?!

Okay, I'll add some more comments.

OTOH, what was hard to write should be hard to read (just kidding).

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 --]