Re: [PATCH v6 00/16] x86/mtrr: fix handling with PAT but without MTRR
On 01.06.23 16:33, Borislav Petkov wrote: On Thu, Jun 01, 2023 at 03:22:33PM +0200, Borislav Petkov wrote: Now lemme restart testing. This is from another box, with the latest changes incorporated: https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/log/?h=rc1-mtrr --- proc-mtrr.before2011-03-04 01:03:35.243994733 +0100 +++ proc-mtrr.after 2023-06-01 16:28:54.95456 +0200 @@ -1,3 +1,3 @@ reg00: base=0x0 (0MB), size= 2048MB, count=1: write-back reg01: base=0x08000 ( 2048MB), size= 1024MB, count=1: write-back -reg02: base=0x0c000 ( 3072MB), size= 256MB, count=1: write-back +reg02: base=0x0c000 ( 3072MB), size= 128MB, count=1: write-back Want mtrr=debug output again? Yes, please Juergen OpenPGP_0xB0DE9DD628BF132F.asc Description: OpenPGP public key OpenPGP_signature Description: OpenPGP digital signature
Re: [PATCH v6 00/16] x86/mtrr: fix handling with PAT but without MTRR
On Thu, Jun 01, 2023 at 03:22:33PM +0200, Borislav Petkov wrote: > Now lemme restart testing. This is from another box, with the latest changes incorporated: https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/log/?h=rc1-mtrr --- proc-mtrr.before2011-03-04 01:03:35.243994733 +0100 +++ proc-mtrr.after 2023-06-01 16:28:54.95456 +0200 @@ -1,3 +1,3 @@ reg00: base=0x0 (0MB), size= 2048MB, count=1: write-back reg01: base=0x08000 ( 2048MB), size= 1024MB, count=1: write-back -reg02: base=0x0c000 ( 3072MB), size= 256MB, count=1: write-back +reg02: base=0x0c000 ( 3072MB), size= 128MB, count=1: write-back Want mtrr=debug output again? -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCH v6 00/16] x86/mtrr: fix handling with PAT but without MTRR
On Thu, Jun 01, 2023 at 10:19:01AM +0200, Juergen Gross wrote: > Patch 2 wants this diff on top: Obviously. :-) That fixes it, thx. Now lemme restart testing. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCH v6 00/16] x86/mtrr: fix handling with PAT but without MTRR
On 31.05.23 19:48, Borislav Petkov wrote:
On Wed, May 31, 2023 at 04:20:08PM +0200, Juergen Gross wrote:
One other note: why does mtrr_cleanup() think that using 8 instead of 6
variable MTRRs would be an "optimal setting"?
Maybe the more extensive debug output below would help answer that...
IMO it should replace the original setup only in case it is using _less_
MTRRs than before.
Right.
The attached patch will do that.
Juergen
From 7989ef9822115a708fc2ba3f7740888a350cb40f Mon Sep 17 00:00:00 2001
From: Juergen Gross
Date: Thu, 1 Jun 2023 14:40:58 +0200
Subject: [PATCH v7] x86/mtrr: Let mtrr_cleanup() not increase number of used
MTRRs
Today mtrr_cleanup() will always use the best found alternative MTRR
setting, even if this setting is using more variable MTRRs than the
BIOS provided setup.
Add a check that only settings with less variable MTRRs are used.
Signed-off-by: Juergen Gross
---
V7:
- new patch
---
arch/x86/kernel/cpu/mtrr/cleanup.c | 16 +---
1 file changed, 13 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kernel/cpu/mtrr/cleanup.c b/arch/x86/kernel/cpu/mtrr/cleanup.c
index a7cb5d32d03d..a5d331722092 100644
--- a/arch/x86/kernel/cpu/mtrr/cleanup.c
+++ b/arch/x86/kernel/cpu/mtrr/cleanup.c
@@ -567,7 +567,7 @@ static int __init mtrr_need_cleanup(void)
num_var_ranges - num[MTRR_NUM_TYPES])
return 0;
- return 1;
+ return num_var_ranges - num[MTRR_NUM_TYPES];
}
static unsigned long __initdata range_sums;
@@ -673,6 +673,7 @@ int __init mtrr_cleanup(void)
u64 chunk_size, gran_size;
mtrr_type type;
int index_good;
+ int num_used;
int i;
if (!cpu_feature_enabled(X86_FEATURE_MTRR) || enable_mtrr_cleanup < 1)
@@ -693,7 +694,8 @@ int __init mtrr_cleanup(void)
}
/* Check if we need handle it and can handle it: */
- if (!mtrr_need_cleanup())
+ num_used = mtrr_need_cleanup();
+ if (!num_used)
return 0;
/* Print original var MTRRs at first, for debugging: */
@@ -728,6 +730,10 @@ int __init mtrr_cleanup(void)
mtrr_print_out_one_result(i);
if (!result[i].bad) {
+ if (result[i].num_reg >= num_used) {
+Dprintk("BIOS provided MTRR setting is better than found one\n");
+return 0;
+ }
set_var_mtrr_all();
Dprintk("New variable MTRRs\n");
print_out_mtrr_range_state();
@@ -762,8 +768,12 @@ int __init mtrr_cleanup(void)
index_good = mtrr_search_optimal_index();
if (index_good != -1) {
- pr_info("Found optimal setting for mtrr clean up\n");
i = index_good;
+ if (result[i].num_reg >= num_used) {
+ Dprintk("BIOS provided MTRR setting is better than found one\n");
+ return 0;
+ }
+ pr_info("Found optimal setting for mtrr clean up\n");
mtrr_print_out_one_result(i);
/* Convert ranges to var ranges state: */
--
2.35.3
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
OpenPGP_signature
Description: OpenPGP digital signature
Re: [PATCH v6 00/16] x86/mtrr: fix handling with PAT but without MTRR
On 01.06.23 14:48, Borislav Petkov wrote: On Thu, Jun 01, 2023 at 08:39:17AM +0200, Juergen Gross wrote: Does this translate to: "we should remove that cleanup crap"? I'd be positive to that. :-) Why, what's wrong with that thing? Why do you need it if you don't think adding MTRRs dynamically is important? Having a sub-optimal MTRR setup doesn't matter unless you are running out of MTRRs to use. When you are not adding MTRRs, you can't run out of them. This in turn means you don't need mtrr_cleanup(). Juergen OpenPGP_0xB0DE9DD628BF132F.asc Description: OpenPGP public key OpenPGP_signature Description: OpenPGP digital signature
Re: [PATCH v6 00/16] x86/mtrr: fix handling with PAT but without MTRR
On Thu, Jun 01, 2023 at 08:39:17AM +0200, Juergen Gross wrote: > Does this translate to: "we should remove that cleanup crap"? I'd be > positive to that. :-) Why, what's wrong with that thing? -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCH v6 00/16] x86/mtrr: fix handling with PAT but without MTRR
On 31.05.23 19:48, Borislav Petkov wrote:
On Wed, May 31, 2023 at 04:20:08PM +0200, Juergen Gross wrote:
One other note: why does mtrr_cleanup() think that using 8 instead of 6
variable MTRRs would be an "optimal setting"?
Maybe the more extensive debug output below would help answer that...
Patch 2 wants this diff on top:
diff --git a/arch/x86/kernel/cpu/mtrr/generic.c
b/arch/x86/kernel/cpu/mtrr/generic.c
index 59b48bd8380c..ce254ca89c62 100644
--- a/arch/x86/kernel/cpu/mtrr/generic.c
+++ b/arch/x86/kernel/cpu/mtrr/generic.c
@@ -655,7 +655,7 @@ static bool set_mtrr_var_ranges(unsigned int index, struct
mtrr_var_range *vr)
bool changed = false;
rdmsr(MTRRphysBase_MSR(index), lo, hi);
- if ((vr->base_lo & MTRR_PHYSBASE_RSVD) != (lo & MTRR_PHYSBASE_RSVD)
+ if ((vr->base_lo & ~MTRR_PHYSBASE_RSVD) != (lo & ~MTRR_PHYSBASE_RSVD)
|| (vr->base_hi & ~phys_hi_rsvd) != (hi & ~phys_hi_rsvd)) {
mtrr_wrmsr(MTRRphysBase_MSR(index), vr->base_lo, vr->base_hi);
@@ -664,7 +664,7 @@ static bool set_mtrr_var_ranges(unsigned int index, struct
mtrr_var_range *vr)
rdmsr(MTRRphysMask_MSR(index), lo, hi);
- if ((vr->mask_lo & MTRR_PHYSMASK_RSVD) != (lo & MTRR_PHYSMASK_RSVD)
+ if ((vr->mask_lo & ~MTRR_PHYSMASK_RSVD) != (lo & ~MTRR_PHYSMASK_RSVD)
|| (vr->mask_hi & ~phys_hi_rsvd) != (hi & ~phys_hi_rsvd)) {
mtrr_wrmsr(MTRRphysMask_MSR(index), vr->mask_lo, vr->mask_hi);
changed = true;
Juergen
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
OpenPGP_signature
Description: OpenPGP digital signature
Re: [PATCH v6 00/16] x86/mtrr: fix handling with PAT but without MTRR
On 31.05.23 19:48, Borislav Petkov wrote:
On Wed, May 31, 2023 at 04:20:08PM +0200, Juergen Gross wrote:
One other note: why does mtrr_cleanup() think that using 8 instead of 6
variable MTRRs would be an "optimal setting"?
Maybe the more extensive debug output below would help answer that...
Above question isn't answered, but it at least tells me that the plan was
to write MTRR values as seen on the original kernel.
Looking into the issue with that information in mind.
IMO it should replace the original setup only in case it is using _less_
MTRRs than before.
Right.
I'll look into that later, unless my question below will be answered with
"yes".
Additionally I believe mtrr_cleanup() would make much more sense if it
wouldn't be __init, but being usable when trying to add additional MTRRs
in the running system in case we run out of MTRRs.
It should probably be based on the new MTRR map anyway...
So I'm not really sure we really care about adding additional MTRRs.
Does this translate to: "we should remove that cleanup crap"? I'd be
positive to that. :-)
There probably is a use case which does that but I haven't seen one yet
- MTRRs are all legacy crap to me.
I think there are still a few drivers using them. No idea how often
those drivers are in use, though.
Btw, one more patch ontop:
---
From: "Borislav Petkov (AMD)"
Date: Wed, 31 May 2023 19:23:34 +0200
Subject: [PATCH] x86/mtrr: Unify debugging printing
Put all the debugging output behind "mtrr=debug" and get rid of
"mtrr_cleanup_debug" which wasn't even documented anywhere.
No functional changes.
Signed-off-by: Borislav Petkov (AMD)
Reviewed-by: Juergen Gross
Juergen
---
arch/x86/kernel/cpu/mtrr/cleanup.c | 59 --
arch/x86/kernel/cpu/mtrr/generic.c | 2 +-
arch/x86/kernel/cpu/mtrr/mtrr.c| 5 +--
arch/x86/kernel/cpu/mtrr/mtrr.h| 3 ++
4 files changed, 29 insertions(+), 40 deletions(-)
diff --git a/arch/x86/kernel/cpu/mtrr/cleanup.c
b/arch/x86/kernel/cpu/mtrr/cleanup.c
index ed5f84c20ac2..18cf79d6e2c5 100644
--- a/arch/x86/kernel/cpu/mtrr/cleanup.c
+++ b/arch/x86/kernel/cpu/mtrr/cleanup.c
@@ -55,9 +55,6 @@ static int __initdata nr_range;
static struct var_mtrr_range_state __initdata range_state[RANGE_NUM];
-static int __initdata debug_print;
-#define Dprintk(x...) do { if (debug_print) pr_debug(x); } while (0)
-
#define BIOS_BUG_MSG \
"WARNING: BIOS bug: VAR MTRR %d contains strange UC entry under 1M, check
with your system vendor!\n"
@@ -79,12 +76,11 @@ x86_get_mtrr_mem_range(struct range *range, int nr_range,
nr_range = add_range_with_merge(range, RANGE_NUM, nr_range,
base, base + size);
}
- if (debug_print) {
- pr_debug("After WB checking\n");
- for (i = 0; i < nr_range; i++)
- pr_debug("MTRR MAP PFN: %016llx - %016llx\n",
-range[i].start, range[i].end);
- }
+
+ Dprintk("After WB checking\n");
+ for (i = 0; i < nr_range; i++)
+ Dprintk("MTRR MAP PFN: %016llx - %016llx\n",
+range[i].start, range[i].end);
/* Take out UC ranges: */
for (i = 0; i < num_var_ranges; i++) {
@@ -112,24 +108,22 @@ x86_get_mtrr_mem_range(struct range *range, int nr_range,
subtract_range(range, RANGE_NUM, extra_remove_base,
extra_remove_base + extra_remove_size);
- if (debug_print) {
- pr_debug("After UC checking\n");
- for (i = 0; i < RANGE_NUM; i++) {
- if (!range[i].end)
- continue;
- pr_debug("MTRR MAP PFN: %016llx - %016llx\n",
-range[i].start, range[i].end);
- }
+ Dprintk("After UC checking\n");
+ for (i = 0; i < RANGE_NUM; i++) {
+ if (!range[i].end)
+ continue;
+
+ Dprintk("MTRR MAP PFN: %016llx - %016llx\n",
+range[i].start, range[i].end);
}
/* sort the ranges */
nr_range = clean_sort_range(range, RANGE_NUM);
- if (debug_print) {
- pr_debug("After sorting\n");
- for (i = 0; i < nr_range; i++)
- pr_debug("MTRR MAP PFN: %016llx - %016llx\n",
-range[i].start, range[i].end);
- }
+
+ Dprintk("After sorting\n");
+ for (i = 0; i < nr_range; i++)
+ Dprintk("MTRR MAP PFN: %016llx - %016llx\n",
+ range[i].start, range[i].end);
return nr_range;
}
@@ -164,13 +158,6 @@ static int __init enable_mtrr_cleanup_setup(char *str)
}
early_param("enable_mtrr_cleanup", enable_mtrr_cleanup_setup);
-static int __init mtrr_cleanup_debug_setup(char *str)
-{
-
Re: [PATCH v6 00/16] x86/mtrr: fix handling with PAT but without MTRR
On 31.05.23 10:35, Borislav Petkov wrote: [0.018357] MTRR default type: uncachable [0.022347] MTRR fixed ranges enabled: [0.026085] 0-9 write-back [0.029650] A-B uncachable [0.033214] C-F write-protect [0.037039] MTRR variable ranges enabled: [0.041038] 0 base 000 mask 0003FFC write-back [0.047383] 1 base 004 mask 0003FFFC000 write-back [0.053730] 2 base 0044000 mask 0003000 write-back [0.060076] 3 base 000AE00 mask 0003E00 uncachable [0.066421] 4 base 000B000 mask 0003000 uncachable [0.072768] 5 base 000C000 mask 0003FFFC000 uncachable [0.079114] 6 disabled [0.081635] 7 disabled [0.084156] 8 disabled [0.086677] 9 disabled [0.089203] total RAM covered: 16352M [0.093023] Found optimal setting for mtrr clean up [0.097734] gran_size: 64K chunk_size: 64M num_reg: 8 lose cover RAM: 0G One other note: why does mtrr_cleanup() think that using 8 instead of 6 variable MTRRs would be an "optimal setting"? IMO it should replace the original setup only in case it is using _less_ MTRRs than before. Additionally I believe mtrr_cleanup() would make much more sense if it wouldn't be __init, but being usable when trying to add additional MTRRs in the running system in case we run out of MTRRs. It should probably be based on the new MTRR map anyway... Juergen OpenPGP_0xB0DE9DD628BF132F.asc Description: OpenPGP public key OpenPGP_signature Description: OpenPGP digital signature
Re: [PATCH v6 00/16] x86/mtrr: fix handling with PAT but without MTRR
On Wed, May 31, 2023 at 11:31:37AM +0200, Juergen Gross wrote: > What it did would have been printed if pr_debug() would have been > active. :-( Lemme turn those into pr_info(). pr_debug() is nuts. > Did you check whether CONFIG_MTRR_SANITIZER_ENABLE_DEFAULT was the same in > both > kernels you've tested? Yes, it is enabled. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCH v6 00/16] x86/mtrr: fix handling with PAT but without MTRR
On 31.05.23 10:35, Borislav Petkov wrote: On Wed, May 31, 2023 at 09:28:57AM +0200, Juergen Gross wrote: Can you please boot the system with the MTRR patches and specify "mtrr=debug" on the command line? I'd be interested in the raw register values being read and the resulting memory type map. This is exactly why I wanted this option. And you're already putting it to good use. :-P Full dmesg below. [0.012878] last_pfn = 0x45 max_arch_pfn = 0x4 [0.018357] MTRR default type: uncachable [0.022347] MTRR fixed ranges enabled: [0.026085] 0-9 write-back [0.029650] A-B uncachable [0.033214] C-F write-protect [0.037039] MTRR variable ranges enabled: [0.041038] 0 base 000 mask 0003FFC write-back 16 GB WB at address 0. [0.047383] 1 base 004 mask 0003FFFC000 write-back 1 GB WB at address 16GB. [0.053730] 2 base 0044000 mask 0003000 write-back 256MB WB at address 17GB. This means per default 0-44fff are WB. [0.060076] 3 base 000AE00 mask 0003E00 uncachable 32MB UC at AE00 [0.066421] 4 base 000B000 mask 0003000 uncachable 256MB UC at B000 [0.072768] 5 base 000C000 mask 0003FFFC000 uncachable 512MB UC at C000 So an UC hole at AE00-. [0.079114] 6 disabled [0.081635] 7 disabled [0.084156] 8 disabled [0.086677] 9 disabled [0.089203] total RAM covered: 16352M [0.093023] Found optimal setting for mtrr clean up It seems as if mtrr_cleanup() did change the MTRR settings. What it did would have been printed if pr_debug() would have been active. :-( [0.097734] gran_size: 64K chunk_size: 64M num_reg: 8 lose cover RAM: 0G [0.104864] MTRR map: 6 entries (3 fixed + 3 variable; max 23), built from 10 variable MTRRs [0.113294] 0: -0009 write-back [0.119033] 1: 000a-000b uncachable [0.124771] 2: 000c-000f write-protect [0.130769] 3: 0010-adff write-back [0.136508] 4: ae00-afff uncachable [0.142246] 5: 0001-00044fff write-back The MTRR map seems to be fine assuming the MTRR values before the "clean up". [0.147992] x86/PAT: Configuration [0-7]: WB WC UC- UC WB WP UC- WT > [0.155122] e820: update [mem 0xae00-0xafff] usable ==> reserved > [0.161663] e820: update [mem 0xc000-0x] usable ==> reserved > [0.168358] e820: update [mem 0x11000-0x1] usable ==> reserved > [0.175227] WARNING: BIOS bug: CPU MTRRs don't cover all of memory, losing 3840MB of RAM. Clean up messed with the settings, resulting in loss of RAM. Did you check whether CONFIG_MTRR_SANITIZER_ENABLE_DEFAULT was the same in both kernels you've tested? Juergen OpenPGP_0xB0DE9DD628BF132F.asc Description: OpenPGP public key OpenPGP_signature Description: OpenPGP digital signature
Re: [PATCH v6 00/16] x86/mtrr: fix handling with PAT but without MTRR
On Wed, May 31, 2023 at 09:28:57AM +0200, Juergen Gross wrote: > Can you please boot the system with the MTRR patches and specify "mtrr=debug" > on the command line? I'd be interested in the raw register values being read > and the resulting memory type map. This is exactly why I wanted this option. And you're already putting it to good use. :-P Full dmesg below. [0.00] microcode: updated early: 0x710 -> 0x718, date = 2019-05-21 [0.00] Linux version 6.4.0-rc1+ (boris@zn) (gcc (Debian 12.2.0-9) 12.2.0, GNU ld (GNU Binutils for Debian) 2.40) #1 SMP PREEMPT_DYNAMIC Tue May 30 15:54:17 CEST 2023 [0.00] Command line: BOOT_IMAGE=/boot/vmlinuz-6.4.0-rc1+ root=/dev/sda7 ro earlyprintk=ttyS0,115200 console=ttyS0,115200 console=tty0 ras=cec_disable root=/dev/sda7 log_buf_len=10M resume=/dev/sda5 no_console_suspend ignore_loglevel mtrr=debug [0.00] KERNEL supported cpus: [0.00] Intel GenuineIntel [0.00] AMD AuthenticAMD [0.00] Centaur CentaurHauls [0.00] x86/fpu: Supporting XSAVE feature 0x001: 'x87 floating point registers' [0.00] x86/fpu: Supporting XSAVE feature 0x002: 'SSE registers' [0.00] x86/fpu: Supporting XSAVE feature 0x004: 'AVX registers' [0.00] x86/fpu: xstate_offset[2]: 576, xstate_sizes[2]: 256 [0.00] x86/fpu: Enabled xstate features 0x7, context size is 832 bytes, using 'standard' format. [0.00] signal: max sigframe size: 1776 [0.00] BIOS-provided physical RAM map: [0.00] BIOS-e820: [mem 0x-0x0009] usable [0.00] BIOS-e820: [mem 0x0010-0x18ebafff] usable [0.00] BIOS-e820: [mem 0x18ebb000-0x18fe7fff] ACPI NVS [0.00] BIOS-e820: [mem 0x18fe8000-0x18fe8fff] usable [0.00] BIOS-e820: [mem 0x18fe9000-0x18ff] ACPI NVS [0.00] BIOS-e820: [mem 0x1900-0x1dffcfff] usable [0.00] BIOS-e820: [mem 0x1dffd000-0x1dff] ACPI data [0.00] BIOS-e820: [mem 0x1e00-0xac77cfff] usable [0.00] BIOS-e820: [mem 0xac77d000-0xac77] type 20 [0.00] BIOS-e820: [mem 0xac78-0xac780fff] reserved [0.00] BIOS-e820: [mem 0xac781000-0xac782fff] type 20 [0.00] BIOS-e820: [mem 0xac783000-0xac7d9fff] reserved [0.00] BIOS-e820: [mem 0xac7da000-0xac7dafff] type 20 [0.00] BIOS-e820: [mem 0xac7db000-0xac7dcfff] reserved [0.00] BIOS-e820: [mem 0xac7dd000-0xac7e7fff] type 20 [0.00] BIOS-e820: [mem 0xac7e8000-0xac7f1fff] reserved [0.00] BIOS-e820: [mem 0xac7f2000-0xac7f5fff] type 20 [0.00] BIOS-e820: [mem 0xac7f6000-0xac7f9fff] reserved [0.00] BIOS-e820: [mem 0xac7fa000-0xac7fafff] type 20 [0.00] BIOS-e820: [mem 0xac7fb000-0xac803fff] reserved [0.00] BIOS-e820: [mem 0xac804000-0xac810fff] type 20 [0.00] BIOS-e820: [mem 0xac811000-0xac813fff] reserved [0.00] BIOS-e820: [mem 0xac814000-0xad7f] usable [0.00] BIOS-e820: [mem 0xb000-0xb3ff] reserved [0.00] BIOS-e820: [mem 0xfed2-0xfed3] reserved [0.00] BIOS-e820: [mem 0xfed5-0xfed8] reserved [0.00] BIOS-e820: [mem 0xffa0-0xffa3] reserved [0.00] BIOS-e820: [mem 0x0001-0x00044fff] usable [0.00] printk: bootconsole [earlyser0] enabled [0.00] printk: debug: ignoring loglevel setting. [0.00] NX (Execute Disable) protection: active [0.00] efi: EFI v2.0 by American Megatrends [0.00] efi: ACPI 2.0=0x1d98 SMBIOS=0xac811018 [0.00] efi: Remove mem57: MMIO range=[0xb000-0xb3ff] (64MB) from e820 map [0.00] e820: remove [mem 0xb000-0xb3ff] reserved [0.00] efi: Not removing mem58: MMIO range=[0xfed2-0xfed3] (128KB) from e820 map [0.00] efi: Remove mem59: MMIO range=[0xfed5-0xfed8] (0MB) from e820 map [0.00] e820: remove [mem 0xfed5-0xfed8] reserved [0.00] efi: Remove mem60: MMIO range=[0xffa0-0xffa3] (0MB) from e820 map [0.00] e820: remove [mem 0xffa0-0xffa3] reserved [0.00] SMBIOS 2.6 present. [0.00] DMI: Dell Inc. Precision T3600/0PTTT9, BIOS A13 05/11/2014 [0.00] tsc: Fast TSC calibration using PIT [0.00] tsc: Detected 3591.377 MHz processor [0.000767] e820: update [mem 0x-0x0fff] usable ==> reserved [0.007307] e820: remove [mem 0x000a-0x000f] usable [0.012878] last_pfn = 0x45 max_arch_pfn = 0x4 [0.018357] MTRR default
Re: [PATCH v6 00/16] x86/mtrr: fix handling with PAT but without MTRR
On 30.05.23 17:28, Borislav Petkov wrote: On Mon, May 22, 2023 at 04:17:50PM +0200, Juergen Gross wrote: The attached diff is for patch 13. Merged and pushed out into same branch. Next issue. Diffing /proc/mtrr shows: --- proc-mtrr.6.3 2023-05-30 17:00:13.215999483 +0200 +++ proc-mtrr.after 2023-05-30 16:01:38.281997816 +0200 @@ -1,8 +1,8 @@ reg00: base=0x0 (0MB), size= 2048MB, count=1: write-back -reg01: base=0x08000 ( 2048MB), size= 512MB, count=1: write-back +reg01: base=0x08000 ( 2048MB), size= 1024MB, count=1: write-back reg02: base=0x0a000 ( 2560MB), size= 256MB, count=1: write-back reg03: base=0x0ae00 ( 2784MB), size= 32MB, count=1: uncachable -reg04: base=0x1 ( 4096MB), size= 4096MB, count=1: write-back +reg04: base=0x1 ( 4096MB), size= 256MB, count=1: write-back reg05: base=0x2 ( 8192MB), size= 8192MB, count=1: write-back reg06: base=0x4 (16384MB), size= 1024MB, count=1: write-back reg07: base=0x44000 (17408MB), size= 256MB, count=1: write-back Weird. Can you please boot the system with the MTRR patches and specify "mtrr=debug" on the command line? I'd be interested in the raw register values being read and the resulting memory type map. Juergen OpenPGP_0xB0DE9DD628BF132F.asc Description: OpenPGP public key OpenPGP_signature Description: OpenPGP digital signature
Re: [PATCH v6 00/16] x86/mtrr: fix handling with PAT but without MTRR
On Mon, May 22, 2023 at 04:17:50PM +0200, Juergen Gross wrote: > The attached diff is for patch 13. Merged and pushed out into same branch. Next issue. Diffing /proc/mtrr shows: --- proc-mtrr.6.3 2023-05-30 17:00:13.215999483 +0200 +++ proc-mtrr.after 2023-05-30 16:01:38.281997816 +0200 @@ -1,8 +1,8 @@ reg00: base=0x0 (0MB), size= 2048MB, count=1: write-back -reg01: base=0x08000 ( 2048MB), size= 512MB, count=1: write-back +reg01: base=0x08000 ( 2048MB), size= 1024MB, count=1: write-back reg02: base=0x0a000 ( 2560MB), size= 256MB, count=1: write-back reg03: base=0x0ae00 ( 2784MB), size= 32MB, count=1: uncachable -reg04: base=0x1 ( 4096MB), size= 4096MB, count=1: write-back +reg04: base=0x1 ( 4096MB), size= 256MB, count=1: write-back reg05: base=0x2 ( 8192MB), size= 8192MB, count=1: write-back reg06: base=0x4 (16384MB), size= 1024MB, count=1: write-back reg07: base=0x44000 (17408MB), size= 256MB, count=1: write-back -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCH v6 00/16] x86/mtrr: fix handling with PAT but without MTRR
On 11.05.23 18:32, Borislav Petkov wrote:
On Wed, May 10, 2023 at 05:53:15PM +0200, Juergen Gross wrote:
Urgh, yes, there is something missing:
diff --git a/arch/x86/kernel/cpu/mtrr/generic.c
b/arch/x86/kernel/cpu/mtrr/generic.c
index 031f7ea8e72b..9544e7d13bb3 100644
--- a/arch/x86/kernel/cpu/mtrr/generic.c
+++ b/arch/x86/kernel/cpu/mtrr/generic.c
@@ -521,8 +521,12 @@ u8 mtrr_type_lookup(u64 start, u64 end, u8 *uniform)
for (i = 0; i < cache_map_n && start < end; i++) {
if (start >= cache_map[i].end)
continue;
So the loop will go through the map until...
- if (start < cache_map[i].start)
+ if (start < cache_map[i].start) {
... it reaches the first entry where that is true.
type = type_merge(type, mtrr_state.def_type, uniform);
the @type argument is MTRR_TYPE_INVALID, def_type is WRBACK so what
this'll do is simply get you the default WRBACK type:
type_merge:
if (type == MTRR_TYPE_INVALID)
return new_type;
+ start = cache_map[i].start;
+ if (end <= start)
+ break;
Now you break here because end <= start. Why?
You can just as well do:
if (start < cache_map[i].start) {
/* region non-overlapping with the region in the map */
if (end <= cache_map[i].start)
return type_merge(type, mtrr_state.def_type, uniform);
... rest of the processing ...
In general, I get it that your code is slick but I want it to be
maintainable - not slick. I'd like for when people look at this, not
have to add a bunch of debugging output in order to swap the whole
thing back into their brains.
So mtrr_type_lookup() definitely needs comments explaining what goes
where.
You can send it as a diff ontop - I'll merge it.
The attached diff is for patch 13.
Juergen
diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
index 031f7ea8e72b..4171788b8754 100644
--- a/arch/x86/kernel/cpu/mtrr/generic.c
+++ b/arch/x86/kernel/cpu/mtrr/generic.c
@@ -519,15 +519,26 @@ u8 mtrr_type_lookup(u64 start, u64 end, u8 *uniform)
return MTRR_TYPE_INVALID;
for (i = 0; i < cache_map_n && start < end; i++) {
+ /* Region after current map entry? -> continue with next one. */
if (start >= cache_map[i].end)
continue;
- if (start < cache_map[i].start)
+
+ /* Start of region not covered by current map entry? */
+ if (start < cache_map[i].start) {
+ /* At least some part of region has default type. */
type = type_merge(type, mtrr_state.def_type, uniform);
+ /* End of region not covered, too? -> lookup done. */
+ if (end <= cache_map[i].start)
+return type;
+ }
+
+ /* At least part of region covered by map entry. */
type = type_merge(type, cache_map[i].type, uniform);
start = cache_map[i].end;
}
+ /* End of region past last entry in map? -> use default type. */
if (start < end)
type = type_merge(type, mtrr_state.def_type, uniform);
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
OpenPGP_signature
Description: OpenPGP digital signature
Re: [PATCH v6 00/16] x86/mtrr: fix handling with PAT but without MTRR
On Wed, May 10, 2023 at 05:53:15PM +0200, Juergen Gross wrote:
> Urgh, yes, there is something missing:
>
> diff --git a/arch/x86/kernel/cpu/mtrr/generic.c
> b/arch/x86/kernel/cpu/mtrr/generic.c
> index 031f7ea8e72b..9544e7d13bb3 100644
> --- a/arch/x86/kernel/cpu/mtrr/generic.c
> +++ b/arch/x86/kernel/cpu/mtrr/generic.c
> @@ -521,8 +521,12 @@ u8 mtrr_type_lookup(u64 start, u64 end, u8 *uniform)
> for (i = 0; i < cache_map_n && start < end; i++) {
> if (start >= cache_map[i].end)
> continue;
So the loop will go through the map until...
> - if (start < cache_map[i].start)
> + if (start < cache_map[i].start) {
... it reaches the first entry where that is true.
> type = type_merge(type, mtrr_state.def_type, uniform);
the @type argument is MTRR_TYPE_INVALID, def_type is WRBACK so what
this'll do is simply get you the default WRBACK type:
type_merge:
if (type == MTRR_TYPE_INVALID)
return new_type;
> + start = cache_map[i].start;
> + if (end <= start)
> + break;
Now you break here because end <= start. Why?
You can just as well do:
if (start < cache_map[i].start) {
/* region non-overlapping with the region in the map */
if (end <= cache_map[i].start)
return type_merge(type, mtrr_state.def_type, uniform);
... rest of the processing ...
In general, I get it that your code is slick but I want it to be
maintainable - not slick. I'd like for when people look at this, not
have to add a bunch of debugging output in order to swap the whole
thing back into their brains.
So mtrr_type_lookup() definitely needs comments explaining what goes
where.
You can send it as a diff ontop - I'll merge it.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCH v6 00/16] x86/mtrr: fix handling with PAT but without MTRR
On 10.05.23 15:30, Borislav Petkov wrote:
On Wed, May 10, 2023 at 01:36:41AM +0200, Borislav Petkov wrote:
More staring at this tomorrow, on a clear head.
Yeah, I'm going to leave it as is. Tried doing a union with bitfields
but doesn't get any prettier.
Next crapola:
The Intel box says now:
[8.138683] sgx: EPC section 0x8020-0x85ff
[8.204838] pmd_set_huge: Cannot satisfy [mem 0x8020-0x8040] with a
huge-page mapping due to MTRR override, uniform: 0
(I've extended the debug output).
and that happens because
[8.174229] mtrr_type_lookup: mtrr_state_set: 1
[8.178909] mtrr_type_lookup: start: 0x8020, cache_map[3].start:
0x8880
that's
if (start < cache_map[i].start) {
in mtrr_type_lookup(). I fail to see how that check would work for the
range 0x8020-0x8040 and the MTRR map is:
[0.000587] MTRR map: 4 entries (3 fixed + 1 variable; max 23), built from
10 variable MTRRs
[0.000588] 0: -0009 write-back
[0.000589] 1: 000a-000b uncachable
[0.000590] 2: 000c-000f write-protect
[0.000591] 3: 8880- uncachable
so the UC range comes after this one we request.
[8.186372] mtrr_type_lookup: type: 0x6, cache_map[3].type: 0x0
now the next type merging happens and the 3rd region's type is UC, ofc.
[8.192433] type_merge: type: 0x6, new_type: 0x0, effective_type: 0x0, clear
uniform
we clear uniform and we fail:
[8.200331] mtrr_type_lookup: ret, uniform: 0
So this map lookup thing is wrong in this case.
Urgh, yes, there is something missing:
diff --git a/arch/x86/kernel/cpu/mtrr/generic.c
b/arch/x86/kernel/cpu/mtrr/generic.c
index 031f7ea8e72b..9544e7d13bb3 100644
--- a/arch/x86/kernel/cpu/mtrr/generic.c
+++ b/arch/x86/kernel/cpu/mtrr/generic.c
@@ -521,8 +521,12 @@ u8 mtrr_type_lookup(u64 start, u64 end, u8 *uniform)
for (i = 0; i < cache_map_n && start < end; i++) {
if (start >= cache_map[i].end)
continue;
- if (start < cache_map[i].start)
+ if (start < cache_map[i].start) {
type = type_merge(type, mtrr_state.def_type, uniform);
+ start = cache_map[i].start;
+ if (end <= start)
+ break;
+ }
type = type_merge(type, cache_map[i].type, uniform);
start = cache_map[i].end;
Juergen
OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key
OpenPGP_signature
Description: OpenPGP digital signature
Re: [PATCH v6 00/16] x86/mtrr: fix handling with PAT but without MTRR
On Wed, May 10, 2023 at 03:30:24PM +0200, Borislav Petkov wrote: > So this map lookup thing is wrong in this case. Btw, current tree is: https://git.kernel.org/pub/scm/linux/kernel/git/bp/bp.git/log/?h=rc1-mtrr -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCH v6 00/16] x86/mtrr: fix handling with PAT but without MTRR
On Wed, May 10, 2023 at 01:36:41AM +0200, Borislav Petkov wrote:
> More staring at this tomorrow, on a clear head.
Yeah, I'm going to leave it as is. Tried doing a union with bitfields
but doesn't get any prettier.
Next crapola:
The Intel box says now:
[8.138683] sgx: EPC section 0x8020-0x85ff
[8.204838] pmd_set_huge: Cannot satisfy [mem 0x8020-0x8040] with a
huge-page mapping due to MTRR override, uniform: 0
(I've extended the debug output).
and that happens because
[8.174229] mtrr_type_lookup: mtrr_state_set: 1
[8.178909] mtrr_type_lookup: start: 0x8020, cache_map[3].start:
0x8880
that's
if (start < cache_map[i].start) {
in mtrr_type_lookup(). I fail to see how that check would work for the
range 0x8020-0x8040 and the MTRR map is:
[0.000587] MTRR map: 4 entries (3 fixed + 1 variable; max 23), built from
10 variable MTRRs
[0.000588] 0: -0009 write-back
[0.000589] 1: 000a-000b uncachable
[0.000590] 2: 000c-000f write-protect
[0.000591] 3: 8880- uncachable
so the UC range comes after this one we request.
[8.186372] mtrr_type_lookup: type: 0x6, cache_map[3].type: 0x0
now the next type merging happens and the 3rd region's type is UC, ofc.
[8.192433] type_merge: type: 0x6, new_type: 0x0, effective_type: 0x0, clear
uniform
we clear uniform and we fail:
[8.200331] mtrr_type_lookup: ret, uniform: 0
So this map lookup thing is wrong in this case.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCH v6 00/16] x86/mtrr: fix handling with PAT but without MTRR
On Tue, May 09, 2023 at 10:14:37PM +0200, Borislav Petkov wrote:
> On Tue, May 02, 2023 at 02:09:15PM +0200, Juergen Gross wrote:
> > This series tries to fix the rather special case of PAT being available
> > without having MTRRs (either due to CONFIG_MTRR being not set, or
> > because the feature has been disabled e.g. by a hypervisor).
>
> More weird stuff. With the series:
Yah, that was me.
That ->enabled thing is *two* bits. FFS.
More staring at this tomorrow, on a clear head.
diff --git a/arch/x86/include/uapi/asm/mtrr.h b/arch/x86/include/uapi/asm/mtrr.h
index a28e6bbd8f21..f476a1355182 100644
--- a/arch/x86/include/uapi/asm/mtrr.h
+++ b/arch/x86/include/uapi/asm/mtrr.h
@@ -84,7 +84,7 @@ typedef __u8 mtrr_type;
struct mtrr_state_type {
struct mtrr_var_range var_ranges[MTRR_MAX_VAR_RANGES];
mtrr_type fixed_ranges[MTRR_NUM_FIXED_RANGES];
- bool enabled;
+ unsigned char enabled;
bool have_fixed;
mtrr_type def_type;
};
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCH v6 00/16] x86/mtrr: fix handling with PAT but without MTRR
On Tue, May 02, 2023 at 02:09:15PM +0200, Juergen Gross wrote: > This series tries to fix the rather special case of PAT being available > without having MTRRs (either due to CONFIG_MTRR being not set, or > because the feature has been disabled e.g. by a hypervisor). More weird stuff. With the series: [root@vh: ~> cat /proc/mtrr cat: /proc/mtrr: Input/output error before: [root@vh: ~> cat /proc/mtrr reg00: base=0x0 (0MB), size= 2048MB, count=1: write-back reg01: base=0x08000 ( 2048MB), size= 1024MB, count=1: write-back reg02: base=0x0c000 ( 3072MB), size= 256MB, count=1: write-back reg03: base=0x0ff00 ( 4080MB), size= 16MB, count=1: write-protect I think it wrongly determines that MTRRs are disabled by BIOS: MTRRs disabled by BIOS x86/PAT: Configuration [0-7]: WB WC UC- UC WB WP UC- WT which is obviously wrong. But more debugging later. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCH v6 00/16] x86/mtrr: fix handling with PAT but without MTRR
On Wed, May 03, 2023 at 11:14:25AM -0700, Sohil Mehta wrote: > A Nit -> Documentation/process/maintainer-tip.rst suggests: > "The condensed patch description in the subject line should start with a > uppercase letter and ..." Yeah, good point. But my patch massaging script does that automatically so no need to resend. Thx. -- Regards/Gruss, Boris. https://people.kernel.org/tglx/notes-about-netiquette
Re: [PATCH v6 00/16] x86/mtrr: fix handling with PAT but without MTRR
> Juergen Gross (16): > x86/mtrr: remove physical address size calculation > x86/mtrr: replace some constants with defines > x86/mtrr: support setting MTRR state for software defined MTRRs > x86/hyperv: set MTRR state when running as SEV-SNP Hyper-V guest > x86/xen: set MTRR state when running as Xen PV initial domain > x86/mtrr: replace vendor tests in MTRR code > x86/mtrr: have only one set_mtrr() variant > x86/mtrr: move 32-bit code from mtrr.c to legacy.c > x86/mtrr: allocate mtrr_value array dynamically > x86/mtrr: add get_effective_type() service function > x86/mtrr: construct a memory map with cache modes > x86/mtrr: add mtrr=debug command line option > x86/mtrr: use new cache_map in mtrr_type_lookup() > x86/mtrr: don't let mtrr_type_lookup() return MTRR_TYPE_INVALID > x86/mm: only check uniform after calling mtrr_type_lookup() > x86/mtrr: remove unused code > A Nit -> Documentation/process/maintainer-tip.rst suggests: "The condensed patch description in the subject line should start with a uppercase letter and ..." -Sohil
RE: [PATCH v6 00/16] x86/mtrr: fix handling with PAT but without MTRR
From: Juergen Gross Sent: Tuesday, May 2, 2023 5:09 AM > > This series tries to fix the rather special case of PAT being available > without having MTRRs (either due to CONFIG_MTRR being not set, or > because the feature has been disabled e.g. by a hypervisor). > > The main use cases are Xen PV guests and SEV-SNP guests running under > Hyper-V. > > Instead of trying to work around all the issues by adding if statements > here and there, just try to use the complete available infrastructure > by setting up a read-only MTRR state when needed. > > In the Xen PV case the current MTRR MSR values can be read from the > hypervisor, while for the SEV-SNP case all needed is to set the > default caching mode to "WB". > > I have added more cleanup which has been discussed when looking into > the most recent failures. > > Note that I couldn't test the Hyper-V related change (patch 3). > > Running on bare metal and with Xen didn't show any problems with the > series applied. > > It should be noted that patches 9+10 are replacing today's way to > lookup the MTRR cache type for a memory region from looking at the > MTRR register values to building a memory map with the cache types. > This should make the lookup much faster and much easier to understand. > > Changes in V2: > - replaced former patches 1+2 with new patches 1-4, avoiding especially > the rather hacky approach of V1, while making all the MTRR type > conflict tests available for the Xen PV case > - updated patch 6 (was patch 4 in V1) > > Changes in V3: > - dropped patch 5 of V2, as already applied > - split patch 1 of V2 into 2 patches > - new patches 6-10 > - addressed comments > > Changes in V4: > - addressed comments > > Changes in V5 > - addressed comments > - some other small fixes > - new patches 3, 8 and 15 > > Changes in V6: > - patch 1 replaces patches 1+2 of V5 > - new patches 8+12 > - addressed comments > > Juergen Gross (16): > x86/mtrr: remove physical address size calculation > x86/mtrr: replace some constants with defines > x86/mtrr: support setting MTRR state for software defined MTRRs > x86/hyperv: set MTRR state when running as SEV-SNP Hyper-V guest > x86/xen: set MTRR state when running as Xen PV initial domain > x86/mtrr: replace vendor tests in MTRR code > x86/mtrr: have only one set_mtrr() variant > x86/mtrr: move 32-bit code from mtrr.c to legacy.c > x86/mtrr: allocate mtrr_value array dynamically > x86/mtrr: add get_effective_type() service function > x86/mtrr: construct a memory map with cache modes > x86/mtrr: add mtrr=debug command line option > x86/mtrr: use new cache_map in mtrr_type_lookup() > x86/mtrr: don't let mtrr_type_lookup() return MTRR_TYPE_INVALID > x86/mm: only check uniform after calling mtrr_type_lookup() > x86/mtrr: remove unused code > > .../admin-guide/kernel-parameters.txt | 4 + > arch/x86/hyperv/ivm.c | 4 + > arch/x86/include/asm/mtrr.h | 43 +- > arch/x86/include/uapi/asm/mtrr.h | 6 +- > arch/x86/kernel/cpu/mtrr/Makefile | 2 +- > arch/x86/kernel/cpu/mtrr/amd.c| 2 +- > arch/x86/kernel/cpu/mtrr/centaur.c| 11 +- > arch/x86/kernel/cpu/mtrr/cleanup.c| 22 +- > arch/x86/kernel/cpu/mtrr/cyrix.c | 2 +- > arch/x86/kernel/cpu/mtrr/generic.c| 677 -- > arch/x86/kernel/cpu/mtrr/legacy.c | 90 +++ > arch/x86/kernel/cpu/mtrr/mtrr.c | 195 ++--- > arch/x86/kernel/cpu/mtrr/mtrr.h | 18 +- > arch/x86/kernel/setup.c | 2 + > arch/x86/mm/pgtable.c | 24 +- > arch/x86/xen/enlighten_pv.c | 52 ++ > 16 files changed, 721 insertions(+), 433 deletions(-) > create mode 100644 arch/x86/kernel/cpu/mtrr/legacy.c > > -- > 2.35.3 I've tested the full v6 series in a normal Hyper-V guest and in an SEV-SNP guest. In the SNP guest, the page attributes in /sys/kernel/debug/x86/pat_memtype_list are "write-back" in the expected cases. The "mtrr" x86 feature no longer appears in the "flags" output of "lscpu" or /proc/cpuinfo. /proc/mtrr does not exist, again as expected. In a normal VM, the "mtrr" x86 feature appears in the flags, and /proc/mtrr shows expected values. The boot option mtrr=debug works as expected. Tested-by: Michael Kelley
