[PATCH v4 00/12] x86/mtrr: fix handling with PAT but without MTRR

[PATCH v4 00/12] x86/mtrr: fix handling with PAT but without MTRR

[Juergen Gross]

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

Juergen Gross (12):
  x86/mtrr: split off physical address size calculation
  x86/mtrr: optimize mtrr_calc_physbits()
  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: allocate mtrr_value array dynamically
  x86/mtrr: add get_effective_type() service function
  x86/mtrr: construct a memory map with cache modes
  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()

 arch/x86/include/asm/mtrr.h        |  15 +-
 arch/x86/include/uapi/asm/mtrr.h   |   6 +-
 arch/x86/kernel/cpu/mshyperv.c     |   4 +
 arch/x86/kernel/cpu/mtrr/amd.c     |   2 +-
 arch/x86/kernel/cpu/mtrr/centaur.c |   2 +-
 arch/x86/kernel/cpu/mtrr/cleanup.c |   4 +-
 arch/x86/kernel/cpu/mtrr/cyrix.c   |   2 +-
 arch/x86/kernel/cpu/mtrr/generic.c | 492 ++++++++++++++++++-----------
 arch/x86/kernel/cpu/mtrr/mtrr.c    |  94 +++---
 arch/x86/kernel/cpu/mtrr/mtrr.h    |   7 +-
 arch/x86/kernel/setup.c            |   2 +
 arch/x86/mm/pgtable.c              |  24 +-
 arch/x86/xen/enlighten_pv.c        |  52 +++
 13 files changed, 454 insertions(+), 252 deletions(-)

-- 
2.35.3

[PATCH v4 01/12] x86/mtrr: split off physical address size calculation

[Juergen Gross]

Move the calculation of the physical address size in mtrr_bp_init()
into a helper function. This will be needed later.

Do only the pure code movement without optimizing it.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- new patch
V3:
- only move code, split off optimizations (Boris Petkov)
---
 arch/x86/kernel/cpu/mtrr/mtrr.c | 57 ++++++++++++++++++---------------
 1 file changed, 32 insertions(+), 25 deletions(-)

diff --git a/arch/x86/kernel/cpu/mtrr/mtrr.c b/arch/x86/kernel/cpu/mtrr/mtrr.c
index 783f3210d582..8310bdb111d0 100644
--- a/arch/x86/kernel/cpu/mtrr/mtrr.c
+++ b/arch/x86/kernel/cpu/mtrr/mtrr.c
@@ -620,22 +620,14 @@ static struct syscore_ops mtrr_syscore_ops = {
 int __initdata changed_by_mtrr_cleanup;
 
 #define SIZE_OR_MASK_BITS(n)  (~((1ULL << ((n) - PAGE_SHIFT)) - 1))
-/**
- * mtrr_bp_init - initialize mtrrs on the boot CPU
- *
- * This needs to be called early; before any of the other CPUs are
- * initialized (i.e. before smp_init()).
- *
- */
-void __init mtrr_bp_init(void)
+
+static unsigned int __init mtrr_calc_physbits(bool generic)
 {
-	const char *why = "(not available)";
-	u32 phys_addr;
+	unsigned int phys_addr;
 
 	phys_addr = 32;
 
-	if (boot_cpu_has(X86_FEATURE_MTRR)) {
-		mtrr_if = &generic_mtrr_ops;
+	if (generic) {
 		size_or_mask = SIZE_OR_MASK_BITS(36);
 		size_and_mask = 0x00f00000;
 		phys_addr = 36;
@@ -667,29 +659,44 @@ void __init mtrr_bp_init(void)
 			size_and_mask = 0;
 			phys_addr = 32;
 		}
+	} else {
+		size_or_mask = SIZE_OR_MASK_BITS(32);
+		size_and_mask = 0;
+	}
+
+	return phys_addr;
+}
+
+/**
+ * mtrr_bp_init - initialize mtrrs on the boot CPU
+ *
+ * This needs to be called early; before any of the other CPUs are
+ * initialized (i.e. before smp_init()).
+ *
+ */
+void __init mtrr_bp_init(void)
+{
+	const char *why = "(not available)";
+	unsigned int phys_addr;
+
+	phys_addr = mtrr_calc_physbits(boot_cpu_has(X86_FEATURE_MTRR));
+
+	if (boot_cpu_has(X86_FEATURE_MTRR)) {
+		mtrr_if = &generic_mtrr_ops;
 	} else {
 		switch (boot_cpu_data.x86_vendor) {
 		case X86_VENDOR_AMD:
-			if (cpu_feature_enabled(X86_FEATURE_K6_MTRR)) {
-				/* Pre-Athlon (K6) AMD CPU MTRRs */
+			/* Pre-Athlon (K6) AMD CPU MTRRs */
+			if (cpu_feature_enabled(X86_FEATURE_K6_MTRR))
 				mtrr_if = &amd_mtrr_ops;
-				size_or_mask = SIZE_OR_MASK_BITS(32);
-				size_and_mask = 0;
-			}
 			break;
 		case X86_VENDOR_CENTAUR:
-			if (cpu_feature_enabled(X86_FEATURE_CENTAUR_MCR)) {
+			if (cpu_feature_enabled(X86_FEATURE_CENTAUR_MCR))
 				mtrr_if = &centaur_mtrr_ops;
-				size_or_mask = SIZE_OR_MASK_BITS(32);
-				size_and_mask = 0;
-			}
 			break;
 		case X86_VENDOR_CYRIX:
-			if (cpu_feature_enabled(X86_FEATURE_CYRIX_ARR)) {
+			if (cpu_feature_enabled(X86_FEATURE_CYRIX_ARR))
 				mtrr_if = &cyrix_mtrr_ops;
-				size_or_mask = SIZE_OR_MASK_BITS(32);
-				size_and_mask = 0;
-			}
 			break;
 		default:
 			break;
-- 
2.35.3

[PATCH v4 02/12] x86/mtrr: optimize mtrr_calc_physbits()

[Juergen Gross]

Optimize mtrr_calc_physbits() for better readability.

Drop a stale comment, as reality has made it obsolete.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V3:
- new patch, split off from previous patch (Boris Petkov)
---
 arch/x86/kernel/cpu/mtrr/mtrr.c | 19 +++----------------
 1 file changed, 3 insertions(+), 16 deletions(-)

diff --git a/arch/x86/kernel/cpu/mtrr/mtrr.c b/arch/x86/kernel/cpu/mtrr/mtrr.c
index 8310bdb111d0..7596ebeab929 100644
--- a/arch/x86/kernel/cpu/mtrr/mtrr.c
+++ b/arch/x86/kernel/cpu/mtrr/mtrr.c
@@ -619,8 +619,6 @@ static struct syscore_ops mtrr_syscore_ops = {
 
 int __initdata changed_by_mtrr_cleanup;
 
-#define SIZE_OR_MASK_BITS(n)  (~((1ULL << ((n) - PAGE_SHIFT)) - 1))
-
 static unsigned int __init mtrr_calc_physbits(bool generic)
 {
 	unsigned int phys_addr;
@@ -628,15 +626,8 @@ static unsigned int __init mtrr_calc_physbits(bool generic)
 	phys_addr = 32;
 
 	if (generic) {
-		size_or_mask = SIZE_OR_MASK_BITS(36);
-		size_and_mask = 0x00f00000;
 		phys_addr = 36;
 
-		/*
-		 * This is an AMD specific MSR, but we assume(hope?) that
-		 * Intel will implement it too when they extend the address
-		 * bus of the Xeon.
-		 */
 		if (cpuid_eax(0x80000000) >= 0x80000008) {
 			phys_addr = cpuid_eax(0x80000008) & 0xff;
 			/* CPUID workaround for Intel 0F33/0F34 CPU */
@@ -647,23 +638,19 @@ static unsigned int __init mtrr_calc_physbits(bool generic)
 			     boot_cpu_data.x86_stepping == 0x4))
 				phys_addr = 36;
 
-			size_or_mask = SIZE_OR_MASK_BITS(phys_addr);
-			size_and_mask = ~size_or_mask & 0xfffff00000ULL;
 		} else if (boot_cpu_data.x86_vendor == X86_VENDOR_CENTAUR &&
 			   boot_cpu_data.x86 == 6) {
 			/*
 			 * VIA C* family have Intel style MTRRs,
 			 * but don't support PAE
 			 */
-			size_or_mask = SIZE_OR_MASK_BITS(32);
-			size_and_mask = 0;
 			phys_addr = 32;
 		}
-	} else {
-		size_or_mask = SIZE_OR_MASK_BITS(32);
-		size_and_mask = 0;
 	}
 
+	size_or_mask = ~((1ULL << (phys_addr - PAGE_SHIFT)) - 1);
+	size_and_mask = ~size_or_mask & 0xfffff00000ULL;
+
 	return phys_addr;
 }
 
-- 
2.35.3

Re: [PATCH v4 02/12] x86/mtrr: optimize mtrr_calc_physbits()

[Borislav Petkov]

On Mon, Mar 06, 2023 at 05:34:15PM +0100, Juergen Gross wrote:
> Optimize mtrr_calc_physbits() for better readability.
> 
> Drop a stale comment, as reality has made it obsolete.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> V3:
> - new patch, split off from previous patch (Boris Petkov)
> ---
>  arch/x86/kernel/cpu/mtrr/mtrr.c | 19 +++----------------
>  1 file changed, 3 insertions(+), 16 deletions(-)

Optimize some more:

---
From: Juergen Gross <jgross@suse.com>
Date: Mon, 6 Mar 2023 17:34:15 +0100
Subject: [PATCH] x86/mtrr: Optimize mtrr_calc_physbits()

Optimize mtrr_calc_physbits() for better readability.

Drop a stale comment, as reality has made it obsolete.

  [ bp:
   - s/mtrr/MTRR/
   - s/boot_cpu_has/cpu_feature_enabled/
   - use GENMASK_ULL
   - simplify. ]

Signed-off-by: Juergen Gross <jgross@suse.com>
Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de>
Link: https://lore.kernel.org/r/20230306163425.8324-3-jgross@suse.com
---
 arch/x86/kernel/cpu/mtrr/mtrr.c | 27 +++++++--------------------
 1 file changed, 7 insertions(+), 20 deletions(-)

diff --git a/arch/x86/kernel/cpu/mtrr/mtrr.c b/arch/x86/kernel/cpu/mtrr/mtrr.c
index 8310bdb111d0..deb22e989105 100644
--- a/arch/x86/kernel/cpu/mtrr/mtrr.c
+++ b/arch/x86/kernel/cpu/mtrr/mtrr.c
@@ -619,8 +619,6 @@ static struct syscore_ops mtrr_syscore_ops = {
 
 int __initdata changed_by_mtrr_cleanup;
 
-#define SIZE_OR_MASK_BITS(n)  (~((1ULL << ((n) - PAGE_SHIFT)) - 1))
-
 static unsigned int __init mtrr_calc_physbits(bool generic)
 {
 	unsigned int phys_addr;
@@ -628,15 +626,8 @@ static unsigned int __init mtrr_calc_physbits(bool generic)
 	phys_addr = 32;
 
 	if (generic) {
-		size_or_mask = SIZE_OR_MASK_BITS(36);
-		size_and_mask = 0x00f00000;
 		phys_addr = 36;
 
-		/*
-		 * This is an AMD specific MSR, but we assume(hope?) that
-		 * Intel will implement it too when they extend the address
-		 * bus of the Xeon.
-		 */
 		if (cpuid_eax(0x80000000) >= 0x80000008) {
 			phys_addr = cpuid_eax(0x80000008) & 0xff;
 			/* CPUID workaround for Intel 0F33/0F34 CPU */
@@ -647,41 +638,37 @@ static unsigned int __init mtrr_calc_physbits(bool generic)
 			     boot_cpu_data.x86_stepping == 0x4))
 				phys_addr = 36;
 
-			size_or_mask = SIZE_OR_MASK_BITS(phys_addr);
-			size_and_mask = ~size_or_mask & 0xfffff00000ULL;
 		} else if (boot_cpu_data.x86_vendor == X86_VENDOR_CENTAUR &&
 			   boot_cpu_data.x86 == 6) {
 			/*
 			 * VIA C* family have Intel style MTRRs,
 			 * but don't support PAE
 			 */
-			size_or_mask = SIZE_OR_MASK_BITS(32);
-			size_and_mask = 0;
 			phys_addr = 32;
 		}
-	} else {
-		size_or_mask = SIZE_OR_MASK_BITS(32);
-		size_and_mask = 0;
 	}
 
+	size_or_mask  = ~GENMASK_ULL(phys_addr - PAGE_SHIFT, 0);
+	size_and_mask = ~size_or_mask & GENMASK_ULL(39, 20);
+
 	return phys_addr;
 }
 
 /**
- * mtrr_bp_init - initialize mtrrs on the boot CPU
+ * mtrr_bp_init - initialize MTRRs on the boot CPU
  *
  * This needs to be called early; before any of the other CPUs are
  * initialized (i.e. before smp_init()).
- *
  */
 void __init mtrr_bp_init(void)
 {
+	bool generic_mtrrs = cpu_feature_enabled(X86_FEATURE_MTRR);
 	const char *why = "(not available)";
 	unsigned int phys_addr;
 
-	phys_addr = mtrr_calc_physbits(boot_cpu_has(X86_FEATURE_MTRR));
+	phys_addr = mtrr_calc_physbits(generic_mtrrs);
 
-	if (boot_cpu_has(X86_FEATURE_MTRR)) {
+	if (generic_mtrrs) {
 		mtrr_if = &generic_mtrr_ops;
 	} else {
 		switch (boot_cpu_data.x86_vendor) {
-- 
2.35.1

-- 
Regards/Gruss,
    Boris.

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

[PATCH v4 03/12] x86/mtrr: support setting MTRR state for software defined MTRRs

[Juergen Gross]

When running virtualized, MTRR access can be reduced (e.g. in Xen PV
guests or when running as a SEV-SNP guest under Hyper-V). Typically
the hypervisor will reset the MTRR feature in CPUID data, resulting
in no MTRR memory type information being available for the kernel.

This has turned out to result in problems:

- Hyper-V SEV-SNP guests using uncached mappings where they shouldn't
- Xen PV dom0 mapping memory as WB which should be UC- instead

Solve those problems by supporting to set a static MTRR state,
overwriting the empty state used today. In case such a state has been
set, don't call get_mtrr_state() in mtrr_bp_init(). The set state
will only be used by mtrr_type_lookup(), as in all other cases
mtrr_enabled() is being checked, which will return false. Accept the
overwrite call only for selected cases when running as a guest.
Disable X86_FEATURE_MTRR in order to avoid any MTRR modifications by
just refusing them.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- new patch
V3:
- omit fixed MTRRs, as those are currently not needed
- disable X86_FEATURE_MTRR instead of testing it
- provide a stub for !CONFIG_MTRR (Michael Kelley)
- use cpu_feature_enabled() (Boris Petkov)
- add tests for mtrr_overwrite_state() being allowed (Boris Petkov)
V4:
- add test for hv_is_isolation_supported() (Michael Kelley)
---
 arch/x86/include/asm/mtrr.h        |  8 ++++++
 arch/x86/kernel/cpu/mtrr/generic.c | 46 +++++++++++++++++++++++++++++-
 arch/x86/kernel/cpu/mtrr/mtrr.c    |  9 ++++++
 arch/x86/kernel/setup.c            |  2 ++
 4 files changed, 64 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/mtrr.h b/arch/x86/include/asm/mtrr.h
index f0eeaf6e5f5f..f1cb81330a64 100644
--- a/arch/x86/include/asm/mtrr.h
+++ b/arch/x86/include/asm/mtrr.h
@@ -31,6 +31,8 @@
  */
 # ifdef CONFIG_MTRR
 void mtrr_bp_init(void);
+void mtrr_overwrite_state(struct mtrr_var_range *var, unsigned int num_var,
+			  mtrr_type def_type);
 extern u8 mtrr_type_lookup(u64 addr, u64 end, u8 *uniform);
 extern void mtrr_save_fixed_ranges(void *);
 extern void mtrr_save_state(void);
@@ -48,6 +50,12 @@ void mtrr_disable(void);
 void mtrr_enable(void);
 void mtrr_generic_set_state(void);
 #  else
+static inline void mtrr_overwrite_state(struct mtrr_var_range *var,
+					unsigned int num_var,
+					mtrr_type def_type)
+{
+}
+
 static inline u8 mtrr_type_lookup(u64 addr, u64 end, u8 *uniform)
 {
 	/*
diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
index ee09d359e08f..49b4cc923312 100644
--- a/arch/x86/kernel/cpu/mtrr/generic.c
+++ b/arch/x86/kernel/cpu/mtrr/generic.c
@@ -8,10 +8,12 @@
 #include <linux/init.h>
 #include <linux/io.h>
 #include <linux/mm.h>
-
+#include <linux/cc_platform.h>
 #include <asm/processor-flags.h>
 #include <asm/cacheinfo.h>
 #include <asm/cpufeature.h>
+#include <asm/hypervisor.h>
+#include <asm/mshyperv.h>
 #include <asm/tlbflush.h>
 #include <asm/mtrr.h>
 #include <asm/msr.h>
@@ -240,6 +242,48 @@ static u8 mtrr_type_lookup_variable(u64 start, u64 end, u64 *partial_end,
 	return mtrr_state.def_type;
 }
 
+/**
+ * mtrr_overwrite_state - set static MTRR state
+ *
+ * Used to set MTRR state via different means (e.g. with data obtained from
+ * a hypervisor).
+ * Is allowed only for special cases when running virtualized. Must be called
+ * from the x86_init.hyper.init_platform() hook. X86_FEATURE_MTRR must be off.
+ */
+void mtrr_overwrite_state(struct mtrr_var_range *var, unsigned int num_var,
+			  mtrr_type def_type)
+{
+	unsigned int i;
+
+	if (WARN_ON(mtrr_state_set ||
+		    hypervisor_is_type(X86_HYPER_NATIVE) ||
+		    !cpu_feature_enabled(X86_FEATURE_HYPERVISOR) ||
+		    (!cc_platform_has(CC_ATTR_GUEST_SEV_SNP) &&
+		     !hv_is_isolation_supported() &&
+		     !cpu_feature_enabled(X86_FEATURE_XENPV) &&
+		     !cpu_feature_enabled(X86_FEATURE_TDX_GUEST))))
+		return;
+
+	/* Disable MTRR in order to disable MTRR modifications. */
+	setup_clear_cpu_cap(X86_FEATURE_MTRR);
+
+	if (var) {
+		if (num_var > MTRR_MAX_VAR_RANGES) {
+			pr_warn("Trying to overwrite MTRR state with %u variable entries\n",
+				num_var);
+			num_var = MTRR_MAX_VAR_RANGES;
+		}
+		for (i = 0; i < num_var; i++)
+			mtrr_state.var_ranges[i] = var[i];
+		num_var_ranges = num_var;
+	}
+
+	mtrr_state.def_type = def_type;
+	mtrr_state.enabled |= MTRR_STATE_MTRR_ENABLED;
+
+	mtrr_state_set = 1;
+}
+
 /**
  * mtrr_type_lookup - look up memory type in MTRR
  *
diff --git a/arch/x86/kernel/cpu/mtrr/mtrr.c b/arch/x86/kernel/cpu/mtrr/mtrr.c
index 7596ebeab929..5fe62ee0361b 100644
--- a/arch/x86/kernel/cpu/mtrr/mtrr.c
+++ b/arch/x86/kernel/cpu/mtrr/mtrr.c
@@ -666,6 +666,15 @@ void __init mtrr_bp_init(void)
 	const char *why = "(not available)";
 	unsigned int phys_addr;
 
+	if (mtrr_state.enabled) {
+		/* Software overwrite of MTRR state, only for generic case. */
+		mtrr_calc_physbits(true);
+		init_table();
+		pr_info("MTRRs set to read-only\n");
+
+		return;
+	}
+
 	phys_addr = mtrr_calc_physbits(boot_cpu_has(X86_FEATURE_MTRR));
 
 	if (boot_cpu_has(X86_FEATURE_MTRR)) {
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 16babff771bd..0cccfeb67c3a 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -1037,6 +1037,8 @@ void __init setup_arch(char **cmdline_p)
 	/*
 	 * VMware detection requires dmi to be available, so this
 	 * needs to be done after dmi_setup(), for the boot CPU.
+	 * For some guest types (Xen PV, SEV-SNP, TDX) it is required to be
+	 * called before cache_bp_init() for setting up MTRR state.
 	 */
 	init_hypervisor_platform();
 
-- 
2.35.3

Re: [PATCH v4 03/12] x86/mtrr: support setting MTRR state for software defined MTRRs

[Huang, Kai]

On Mon, 2023-03-06 at 17:34 +0100, Juergen Gross wrote:
> When running virtualized, MTRR access can be reduced (e.g. in Xen PV
> guests or when running as a SEV-SNP guest under Hyper-V). Typically
> the hypervisor will reset the MTRR feature in CPUID data, resulting
> in no MTRR memory type information being available for the kernel.
> 
> This has turned out to result in problems:
> 
> - Hyper-V SEV-SNP guests using uncached mappings where they shouldn't
> - Xen PV dom0 mapping memory as WB which should be UC- instead
> 
> Solve those problems by supporting to set a static MTRR state,
> overwriting the empty state used today. In case such a state has been
> set, don't call get_mtrr_state() in mtrr_bp_init(). The set state
> will only be used by mtrr_type_lookup(), as in all other cases
> mtrr_enabled() is being checked, which will return false. Accept the
> overwrite call only for selected cases when running as a guest.
> Disable X86_FEATURE_MTRR in order to avoid any MTRR modifications by
> just refusing them.
> 
> 
[...]

> diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
> index ee09d359e08f..49b4cc923312 100644
> --- a/arch/x86/kernel/cpu/mtrr/generic.c
> +++ b/arch/x86/kernel/cpu/mtrr/generic.c
> @@ -8,10 +8,12 @@
>  #include <linux/init.h>
>  #include <linux/io.h>
>  #include <linux/mm.h>
> -
> +#include <linux/cc_platform.h>
>  #include <asm/processor-flags.h>
>  #include <asm/cacheinfo.h>
>  #include <asm/cpufeature.h>
> +#include <asm/hypervisor.h>
> +#include <asm/mshyperv.h>

Is <asm/mshyperv.h> needed here?

>  #include <asm/tlbflush.h>
>  #include <asm/mtrr.h>
>  #include <asm/msr.h>
> @@ -240,6 +242,48 @@ static u8 mtrr_type_lookup_variable(u64 start, u64 end, u64 *partial_end,
>  	return mtrr_state.def_type;
>  }
>  
> +/**
> + * mtrr_overwrite_state - set static MTRR state
> + *
> + * Used to set MTRR state via different means (e.g. with data obtained from
> + * a hypervisor).

+KVM list and KVM maintainers,

IIUC in the next patch, SEV-SNP guest only sets a synthetic MTRR w/o telling the
hypervisor (hyperv).  I think this works for SEV-SNP running on top of hyperv
because they have mutual understanding?

What about the SNP guest running on other hypervisors such as KVM?

Since this code covers TDX guest too, I think eventually it makes sense for TDX
guest to use this function too (to avoid #VE IIUC).  If want to do that, then I
think TDX guest should have the same mutual understanding with *ALL* hypervisor,
as I am not sure what's the point of making the TDX guest's MTRR behaviour
depending on specific hypervisor.

For now I don't see there's any use case for TDX guest to use non-WB memory type
(in fact, KVM always maps guest memory as WB if there's no non-coherent DMA to
the guest memory), so to me it seems it's OK to make a universal mutual
understanding that TDX guest will always have WB memory type for all memory.

But, I am not sure whether it's better to have a standard hypercall between
guest & hypervisor for this purpose so things can be more flexible?

> + * Is allowed only for special cases when running virtualized. Must be called
> + * from the x86_init.hyper.init_platform() hook. X86_FEATURE_MTRR must be off.
> + */
> +void mtrr_overwrite_state(struct mtrr_var_range *var, unsigned int num_var,
> +			  mtrr_type def_type)
> +{
> +	unsigned int i;
> +
> +	if (WARN_ON(mtrr_state_set ||
> +		    hypervisor_is_type(X86_HYPER_NATIVE) ||
> +		    !cpu_feature_enabled(X86_FEATURE_HYPERVISOR) ||
> +		    (!cc_platform_has(CC_ATTR_GUEST_SEV_SNP) &&
> +		     !hv_is_isolation_supported() &&
> +		     !cpu_feature_enabled(X86_FEATURE_XENPV) &&
> +		     !cpu_feature_enabled(X86_FEATURE_TDX_GUEST))))
> +		return;
> +
> +	/* Disable MTRR in order to disable MTRR modifications. */
> +	setup_clear_cpu_cap(X86_FEATURE_MTRR);
> +
> +	if (var) {
> +		if (num_var > MTRR_MAX_VAR_RANGES) {
> +			pr_warn("Trying to overwrite MTRR state with %u variable entries\n",
> +				num_var);
> +			num_var = MTRR_MAX_VAR_RANGES;
> +		}
> +		for (i = 0; i < num_var; i++)
> +			mtrr_state.var_ranges[i] = var[i];
> +		num_var_ranges = num_var;
> +	}
> +
> +	mtrr_state.def_type = def_type;
> +	mtrr_state.enabled |= MTRR_STATE_MTRR_ENABLED;
> +
> +	mtrr_state_set = 1;
> +}
> +
>  /**
>   * mtrr_type_lookup - look up memory type in MTRR
>   *
> diff --git a/arch/x86/kernel/cpu/mtrr/mtrr.c b/arch/x86/kernel/cpu/mtrr/mtrr.c
> index 7596ebeab929..5fe62ee0361b 100644
> --- a/arch/x86/kernel/cpu/mtrr/mtrr.c
> +++ b/arch/x86/kernel/cpu/mtrr/mtrr.c
> @@ -666,6 +666,15 @@ void __init mtrr_bp_init(void)
>  	const char *why = "(not available)";
>  	unsigned int phys_addr;
>  
> +	if (mtrr_state.enabled) {
> +		/* Software overwrite of MTRR state, only for generic case. */
> +		mtrr_calc_physbits(true);
> +		init_table();
> +		pr_info("MTRRs set to read-only\n");
> +
> +		return;
> +	}
> +
>  	phys_addr = mtrr_calc_physbits(boot_cpu_has(X86_FEATURE_MTRR));
>  
>  	if (boot_cpu_has(X86_FEATURE_MTRR)) {
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index 16babff771bd..0cccfeb67c3a 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -1037,6 +1037,8 @@ void __init setup_arch(char **cmdline_p)
>  	/*
>  	 * VMware detection requires dmi to be available, so this
>  	 * needs to be done after dmi_setup(), for the boot CPU.
> +	 * For some guest types (Xen PV, SEV-SNP, TDX) it is required to be
> +	 * called before cache_bp_init() for setting up MTRR state.
>  	 */
>  	init_hypervisor_platform();
>  

Re: [PATCH v4 03/12] x86/mtrr: support setting MTRR state for software defined MTRRs

[Juergen Gross]

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

On 20.03.23 13:59, Huang, Kai wrote:
> On Mon, 2023-03-06 at 17:34 +0100, Juergen Gross wrote:
>> When running virtualized, MTRR access can be reduced (e.g. in Xen PV
>> guests or when running as a SEV-SNP guest under Hyper-V). Typically
>> the hypervisor will reset the MTRR feature in CPUID data, resulting
>> in no MTRR memory type information being available for the kernel.
>>
>> This has turned out to result in problems:
>>
>> - Hyper-V SEV-SNP guests using uncached mappings where they shouldn't
>> - Xen PV dom0 mapping memory as WB which should be UC- instead
>>
>> Solve those problems by supporting to set a static MTRR state,
>> overwriting the empty state used today. In case such a state has been
>> set, don't call get_mtrr_state() in mtrr_bp_init(). The set state
>> will only be used by mtrr_type_lookup(), as in all other cases
>> mtrr_enabled() is being checked, which will return false. Accept the
>> overwrite call only for selected cases when running as a guest.
>> Disable X86_FEATURE_MTRR in order to avoid any MTRR modifications by
>> just refusing them.
>>
>>
> [...]
> 
>> diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
>> index ee09d359e08f..49b4cc923312 100644
>> --- a/arch/x86/kernel/cpu/mtrr/generic.c
>> +++ b/arch/x86/kernel/cpu/mtrr/generic.c
>> @@ -8,10 +8,12 @@
>>   #include <linux/init.h>
>>   #include <linux/io.h>
>>   #include <linux/mm.h>
>> -
>> +#include <linux/cc_platform.h>
>>   #include <asm/processor-flags.h>
>>   #include <asm/cacheinfo.h>
>>   #include <asm/cpufeature.h>
>> +#include <asm/hypervisor.h>
>> +#include <asm/mshyperv.h>
> 
> Is <asm/mshyperv.h> needed here?

Yes, for hv_is_isolation_supported().

> 
>>   #include <asm/tlbflush.h>
>>   #include <asm/mtrr.h>
>>   #include <asm/msr.h>
>> @@ -240,6 +242,48 @@ static u8 mtrr_type_lookup_variable(u64 start, u64 end, u64 *partial_end,
>>   	return mtrr_state.def_type;
>>   }
>>   
>> +/**
>> + * mtrr_overwrite_state - set static MTRR state
>> + *
>> + * Used to set MTRR state via different means (e.g. with data obtained from
>> + * a hypervisor).
> 
> +KVM list and KVM maintainers,
> 
> IIUC in the next patch, SEV-SNP guest only sets a synthetic MTRR w/o telling the
> hypervisor (hyperv).  I think this works for SEV-SNP running on top of hyperv
> because they have mutual understanding?
> 
> What about the SNP guest running on other hypervisors such as KVM?
> 
> Since this code covers TDX guest too, I think eventually it makes sense for TDX
> guest to use this function too (to avoid #VE IIUC).  If want to do that, then I
> think TDX guest should have the same mutual understanding with *ALL* hypervisor,
> as I am not sure what's the point of making the TDX guest's MTRR behaviour
> depending on specific hypervisor.

This series tries to fix the current fallout.

Boris Petkov asked for the hypervisor specific tests to be added, so I've
added them after discussing the topic with him (he is the maintainer of
this code after all).

> For now I don't see there's any use case for TDX guest to use non-WB memory type
> (in fact, KVM always maps guest memory as WB if there's no non-coherent DMA to
> the guest memory), so to me it seems it's OK to make a universal mutual
> understanding that TDX guest will always have WB memory type for all memory.

I agree.

> But, I am not sure whether it's better to have a standard hypercall between
> guest & hypervisor for this purpose so things can be more flexible?

Maybe. But for now we need to handle the current situation.


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

Re: [PATCH v4 03/12] x86/mtrr: support setting MTRR state for software defined MTRRs

[Huang, Kai]

On Mon, 2023-03-20 at 14:47 +0100, Juergen Gross wrote:
> On 20.03.23 13:59, Huang, Kai wrote:
> > On Mon, 2023-03-06 at 17:34 +0100, Juergen Gross wrote:
> > > When running virtualized, MTRR access can be reduced (e.g. in Xen PV
> > > guests or when running as a SEV-SNP guest under Hyper-V). Typically
> > > the hypervisor will reset the MTRR feature in CPUID data, resulting
> > > in no MTRR memory type information being available for the kernel.
> > > 
> > > This has turned out to result in problems:
> > > 
> > > - Hyper-V SEV-SNP guests using uncached mappings where they shouldn't
> > > - Xen PV dom0 mapping memory as WB which should be UC- instead
> > > 
> > > Solve those problems by supporting to set a static MTRR state,
> > > overwriting the empty state used today. In case such a state has been
> > > set, don't call get_mtrr_state() in mtrr_bp_init(). The set state
> > > will only be used by mtrr_type_lookup(), as in all other cases
> > > mtrr_enabled() is being checked, which will return false. Accept the
> > > overwrite call only for selected cases when running as a guest.
> > > Disable X86_FEATURE_MTRR in order to avoid any MTRR modifications by
> > > just refusing them.
> > > 
> > > 
> > [...]
> > 
> > > diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
> > > index ee09d359e08f..49b4cc923312 100644
> > > --- a/arch/x86/kernel/cpu/mtrr/generic.c
> > > +++ b/arch/x86/kernel/cpu/mtrr/generic.c
> > > @@ -8,10 +8,12 @@
> > >   #include <linux/init.h>
> > >   #include <linux/io.h>
> > >   #include <linux/mm.h>
> > > -
> > > +#include <linux/cc_platform.h>
> > >   #include <asm/processor-flags.h>
> > >   #include <asm/cacheinfo.h>
> > >   #include <asm/cpufeature.h>
> > > +#include <asm/hypervisor.h>
> > > +#include <asm/mshyperv.h>
> > 
> > Is <asm/mshyperv.h> needed here?
> 
> Yes, for hv_is_isolation_supported().
> 
> > 
> > >   #include <asm/tlbflush.h>
> > >   #include <asm/mtrr.h>
> > >   #include <asm/msr.h>
> > > @@ -240,6 +242,48 @@ static u8 mtrr_type_lookup_variable(u64 start, u64 end, u64 *partial_end,
> > >   	return mtrr_state.def_type;
> > >   }
> > >   
> > > +/**
> > > + * mtrr_overwrite_state - set static MTRR state
> > > + *
> > > + * Used to set MTRR state via different means (e.g. with data obtained from
> > > + * a hypervisor).
> > 
> > +KVM list and KVM maintainers,
> > 
> > IIUC in the next patch, SEV-SNP guest only sets a synthetic MTRR w/o telling the
> > hypervisor (hyperv).  I think this works for SEV-SNP running on top of hyperv
> > because they have mutual understanding?
> > 
> > What about the SNP guest running on other hypervisors such as KVM?
> > 
> > Since this code covers TDX guest too, I think eventually it makes sense for TDX
> > guest to use this function too (to avoid #VE IIUC).  If want to do that, then I
> > think TDX guest should have the same mutual understanding with *ALL* hypervisor,
> > as I am not sure what's the point of making the TDX guest's MTRR behaviour
> > depending on specific hypervisor.
> 
> This series tries to fix the current fallout.
> 
> Boris Petkov asked for the hypervisor specific tests to be added, so I've
> added them after discussing the topic with him (he is the maintainer of
> this code after all).
> 
> > For now I don't see there's any use case for TDX guest to use non-WB memory type
> > (in fact, KVM always maps guest memory as WB if there's no non-coherent DMA to
> > the guest memory), so to me it seems it's OK to make a universal mutual
> > understanding that TDX guest will always have WB memory type for all memory.
> 
> I agree.
> 
> > But, I am not sure whether it's better to have a standard hypercall between
> > guest & hypervisor for this purpose so things can be more flexible?
> 
> Maybe. But for now we need to handle the current situation.
> 
> 

Agreed.  Thanks for explaining.

Re: [PATCH v4 03/12] x86/mtrr: support setting MTRR state for software defined MTRRs

[Borislav Petkov]

On Mon, Mar 20, 2023 at 02:47:30PM +0100, Juergen Gross wrote:
> > Since this code covers TDX guest too, I think eventually it makes sense for TDX
> > guest to use this function too (to avoid #VE IIUC).  If want to do that, then I
> > think TDX guest should have the same mutual understanding with *ALL* hypervisor,
> > as I am not sure what's the point of making the TDX guest's MTRR behaviour
> > depending on specific hypervisor.
> 
> This series tries to fix the current fallout.

We can relax the check so that it runs on TDX too. Along with a comment
above it why it needs to run on TDX.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v4 03/12] x86/mtrr: support setting MTRR state for software defined MTRRs

[Juergen Gross]

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

On 20.03.23 23:42, Borislav Petkov wrote:
> On Mon, Mar 20, 2023 at 02:47:30PM +0100, Juergen Gross wrote:
>>> Since this code covers TDX guest too, I think eventually it makes sense for TDX
>>> guest to use this function too (to avoid #VE IIUC).  If want to do that, then I
>>> think TDX guest should have the same mutual understanding with *ALL* hypervisor,
>>> as I am not sure what's the point of making the TDX guest's MTRR behaviour
>>> depending on specific hypervisor.
>>
>> This series tries to fix the current fallout.
> 
> We can relax the check so that it runs on TDX too. Along with a comment
> above it why it needs to run on TDX.
> 

Okay, fine with me.


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

Re: [PATCH v4 03/12] x86/mtrr: support setting MTRR state for software defined MTRRs

[Borislav Petkov]

On Mon, Mar 06, 2023 at 05:34:16PM +0100, Juergen Gross wrote:
> +/**
> + * mtrr_overwrite_state - set static MTRR state
> + *
> + * Used to set MTRR state via different means (e.g. with data obtained from
> + * a hypervisor).
> + * Is allowed only for special cases when running virtualized. Must be called
> + * from the x86_init.hyper.init_platform() hook. X86_FEATURE_MTRR must be off.
> + */
> +void mtrr_overwrite_state(struct mtrr_var_range *var, unsigned int num_var,
> +			  mtrr_type def_type)
> +{
> +	unsigned int i;
> +
> +	if (WARN_ON(mtrr_state_set ||
> +		    hypervisor_is_type(X86_HYPER_NATIVE) ||

Why that check?

> +		    !cpu_feature_enabled(X86_FEATURE_HYPERVISOR) ||
> +		    (!cc_platform_has(CC_ATTR_GUEST_SEV_SNP) &&
> +		     !hv_is_isolation_supported() &&
> +		     !cpu_feature_enabled(X86_FEATURE_XENPV) &&
> +		     !cpu_feature_enabled(X86_FEATURE_TDX_GUEST))))

This is unparseable. Please split it into separate checks:

	if (WARN_ON(mtrr_state_set))
		return;

	if (WARN_ON(!cpu_feature_enabled(X86_FEATURE_HYPERVISOR)))
		return;

	...

and add comments above each one why we're testing this.


> +		return;
> +
> +	/* Disable MTRR in order to disable MTRR modifications. */
> +	setup_clear_cpu_cap(X86_FEATURE_MTRR);
> +
> +	if (var) {
> +		if (num_var > MTRR_MAX_VAR_RANGES) {
> +			pr_warn("Trying to overwrite MTRR state with %u variable entries\n",
> +				num_var);
> +			num_var = MTRR_MAX_VAR_RANGES;
> +		}
> +		for (i = 0; i < num_var; i++)
> +			mtrr_state.var_ranges[i] = var[i];
> +		num_var_ranges = num_var;
> +	}
> +
> +	mtrr_state.def_type = def_type;
> +	mtrr_state.enabled |= MTRR_STATE_MTRR_ENABLED;
> +
> +	mtrr_state_set = 1;
> +}
> +
>  /**
>   * mtrr_type_lookup - look up memory type in MTRR
>   *
> diff --git a/arch/x86/kernel/cpu/mtrr/mtrr.c b/arch/x86/kernel/cpu/mtrr/mtrr.c
> index 7596ebeab929..5fe62ee0361b 100644
> --- a/arch/x86/kernel/cpu/mtrr/mtrr.c
> +++ b/arch/x86/kernel/cpu/mtrr/mtrr.c
> @@ -666,6 +666,15 @@ void __init mtrr_bp_init(void)
>  	const char *why = "(not available)";
>  	unsigned int phys_addr;
>  
> +	if (mtrr_state.enabled) {

I'm guessing the proper detection of that weird state should be:

	/*
	 * Check for the software overwrite of MTRR state, only for generic case.
	 * See mtrr_overwrite_state().
	 */
	if (!cpu_feature_enabled(X86_FEATURE_MTRR) &&
	    mtrr_state.enabled) {
		...


> +		/* Software overwrite of MTRR state, only for generic case. */
> +		mtrr_calc_physbits(true);
> +		init_table();
> +		pr_info("MTRRs set to read-only\n");
> +
> +		return;
> +	}
> +

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v4 03/12] x86/mtrr: support setting MTRR state for software defined MTRRs

[Juergen Gross]

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

On 20.03.23 20:05, Borislav Petkov wrote:
> On Mon, Mar 06, 2023 at 05:34:16PM +0100, Juergen Gross wrote:
>> +/**
>> + * mtrr_overwrite_state - set static MTRR state
>> + *
>> + * Used to set MTRR state via different means (e.g. with data obtained from
>> + * a hypervisor).
>> + * Is allowed only for special cases when running virtualized. Must be called
>> + * from the x86_init.hyper.init_platform() hook. X86_FEATURE_MTRR must be off.
>> + */
>> +void mtrr_overwrite_state(struct mtrr_var_range *var, unsigned int num_var,
>> +			  mtrr_type def_type)
>> +{
>> +	unsigned int i;
>> +
>> +	if (WARN_ON(mtrr_state_set ||
>> +		    hypervisor_is_type(X86_HYPER_NATIVE) ||
> 
> Why that check?

I guess you are asking because the next test seems to catch the same case?

I think it doesn't, e.g. for the case of unknown hypervisors (which shows that
X86_HYPER_NATIVE in theory should be named X86_HYPER_NATIVE_OR_UNKNOWN, or it
should be split into X86_HYPER_NATIVE and X86_HYPER_UNKNOWN).

> 
>> +		    !cpu_feature_enabled(X86_FEATURE_HYPERVISOR) ||
>> +		    (!cc_platform_has(CC_ATTR_GUEST_SEV_SNP) &&
>> +		     !hv_is_isolation_supported() &&
>> +		     !cpu_feature_enabled(X86_FEATURE_XENPV) &&
>> +		     !cpu_feature_enabled(X86_FEATURE_TDX_GUEST))))
> 
> This is unparseable. Please split it into separate checks:
> 
> 	if (WARN_ON(mtrr_state_set))
> 		return;
> 
> 	if (WARN_ON(!cpu_feature_enabled(X86_FEATURE_HYPERVISOR)))
> 		return;
> 
> 	...
> 
> and add comments above each one why we're testing this.

Okay.

> 
> 
>> +		return;
>> +
>> +	/* Disable MTRR in order to disable MTRR modifications. */
>> +	setup_clear_cpu_cap(X86_FEATURE_MTRR);
>> +
>> +	if (var) {
>> +		if (num_var > MTRR_MAX_VAR_RANGES) {
>> +			pr_warn("Trying to overwrite MTRR state with %u variable entries\n",
>> +				num_var);
>> +			num_var = MTRR_MAX_VAR_RANGES;
>> +		}
>> +		for (i = 0; i < num_var; i++)
>> +			mtrr_state.var_ranges[i] = var[i];
>> +		num_var_ranges = num_var;
>> +	}
>> +
>> +	mtrr_state.def_type = def_type;
>> +	mtrr_state.enabled |= MTRR_STATE_MTRR_ENABLED;
>> +
>> +	mtrr_state_set = 1;
>> +}
>> +
>>   /**
>>    * mtrr_type_lookup - look up memory type in MTRR
>>    *
>> diff --git a/arch/x86/kernel/cpu/mtrr/mtrr.c b/arch/x86/kernel/cpu/mtrr/mtrr.c
>> index 7596ebeab929..5fe62ee0361b 100644
>> --- a/arch/x86/kernel/cpu/mtrr/mtrr.c
>> +++ b/arch/x86/kernel/cpu/mtrr/mtrr.c
>> @@ -666,6 +666,15 @@ void __init mtrr_bp_init(void)
>>   	const char *why = "(not available)";
>>   	unsigned int phys_addr;
>>   
>> +	if (mtrr_state.enabled) {
> 
> I'm guessing the proper detection of that weird state should be:
> 
> 	/*
> 	 * Check for the software overwrite of MTRR state, only for generic case.
> 	 * See mtrr_overwrite_state().
> 	 */
> 	if (!cpu_feature_enabled(X86_FEATURE_MTRR) &&
> 	    mtrr_state.enabled) {
> 		...

It basically doesn't matter.

The only possibility of mtrr_state.enabled to be set at this point is a
previous call of mtrr_overwrite_state().


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

Re: [PATCH v4 03/12] x86/mtrr: support setting MTRR state for software defined MTRRs

[Borislav Petkov]

On Tue, Mar 21, 2023 at 07:00:58AM +0100, Juergen Gross wrote:
> I guess you are asking because the next test seems to catch the same case?
> 
> I think it doesn't, e.g. for the case of unknown hypervisors (which shows that
> X86_HYPER_NATIVE in theory should be named X86_HYPER_NATIVE_OR_UNKNOWN, or it
> should be split into X86_HYPER_NATIVE and X86_HYPER_UNKNOWN).

Yeah, we don't care about unknown hypervisors. They'll crash'n'burn
anyway.

My intent is to have every case properly documented with a comment above it
instead of one huge compound conditional.

> It basically doesn't matter.

It doesn't matter now. Until someone decides to "redefine" how MTRRs
should be done again because the next representative from the virt zoo
decided to do magic pink ponies.

I'm not taking any chances anymore judging by the amount of crap that
gets sent into arch/x86/ to support some weird guest contraption.

> The only possibility of mtrr_state.enabled to be set at this point is a
> previous call of mtrr_overwrite_state().

Sure, pls make it explicit and defensive so that it is perfectly clear
what's going on.

Thx.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v4 03/12] x86/mtrr: support setting MTRR state for software defined MTRRs

[Juergen Gross]

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

On 21.03.23 11:30, Borislav Petkov wrote:
> On Tue, Mar 21, 2023 at 07:00:58AM +0100, Juergen Gross wrote:
>> I guess you are asking because the next test seems to catch the same case?
>>
>> I think it doesn't, e.g. for the case of unknown hypervisors (which shows that
>> X86_HYPER_NATIVE in theory should be named X86_HYPER_NATIVE_OR_UNKNOWN, or it
>> should be split into X86_HYPER_NATIVE and X86_HYPER_UNKNOWN).
> 
> Yeah, we don't care about unknown hypervisors. They'll crash'n'burn
> anyway.

Okay, I'll drop that test.

> My intent is to have every case properly documented with a comment above it
> instead of one huge compound conditional.
> 
>> It basically doesn't matter.
> 
> It doesn't matter now. Until someone decides to "redefine" how MTRRs
> should be done again because the next representative from the virt zoo
> decided to do magic pink ponies.
> 
> I'm not taking any chances anymore judging by the amount of crap that
> gets sent into arch/x86/ to support some weird guest contraption.
> 
>> The only possibility of mtrr_state.enabled to be set at this point is a
>> previous call of mtrr_overwrite_state().
> 
> Sure, pls make it explicit and defensive so that it is perfectly clear
> what's going on.

Okay, will do the modification you were suggesting.


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

[PATCH v4 04/12] x86/hyperv: set MTRR state when running as SEV-SNP Hyper-V guest

[Juergen Gross]

In order to avoid mappings using the UC- cache attribute, set the
MTRR state to use WB caching as the default.

This is needed in order to cope with the fact that PAT is enabled,
while MTRRs are not supported by the hypervisor.

Fixes: 90b926e68f50 ("x86/pat: Fix pat_x_mtrr_type() for MTRR disabled case")
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- new patch
---
 arch/x86/kernel/cpu/mshyperv.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/arch/x86/kernel/cpu/mshyperv.c b/arch/x86/kernel/cpu/mshyperv.c
index f36dc2f796c5..0a6cc3cf8919 100644
--- a/arch/x86/kernel/cpu/mshyperv.c
+++ b/arch/x86/kernel/cpu/mshyperv.c
@@ -34,6 +34,7 @@
 #include <clocksource/hyperv_timer.h>
 #include <asm/numa.h>
 #include <asm/coco.h>
+#include <asm/mtrr.h>
 
 /* Is Linux running as the root partition? */
 bool hv_root_partition;
@@ -408,6 +409,9 @@ static void __init ms_hyperv_init_platform(void)
 #ifdef CONFIG_SWIOTLB
 			swiotlb_unencrypted_base = ms_hyperv.shared_gpa_boundary;
 #endif
+
+			/* Set WB as the default cache mode. */
+			mtrr_overwrite_state(NULL, 0, MTRR_TYPE_WRBACK);
 		}
 		/* Isolation VMs are unenlightened SEV-based VMs, thus this check: */
 		if (IS_ENABLED(CONFIG_AMD_MEM_ENCRYPT)) {
-- 
2.35.3

[PATCH v4 05/12] x86/xen: set MTRR state when running as Xen PV initial domain

[Juergen Gross]

When running as Xen PV initial domain (aka dom0), MTRRs are disabled
by the hypervisor, but the system should nevertheless use correct
cache memory types. This has always kind of worked, as disabled MTRRs
resulted in disabled PAT, too, so that the kernel avoided code paths
resulting in inconsistencies. This bypassed all of the sanity checks
the kernel is doing with enabled MTRRs in order to avoid memory
mappings with conflicting memory types.

This has been changed recently, leading to PAT being accepted to be
enabled, while MTRRs stayed disabled. The result is that
mtrr_type_lookup() no longer is accepting all memory type requests,
but started to return WB even if UC- was requested. This led to
driver failures during initialization of some devices.

In reality MTRRs are still in effect, but they are under complete
control of the Xen hypervisor. It is possible, however, to retrieve
the MTRR settings from the hypervisor.

In order to fix those problems, overwrite the MTRR state via
mtrr_overwrite_state() with the MTRR data from the hypervisor, if the
system is running as a Xen dom0.

Fixes: 72cbc8f04fe2 ("x86/PAT: Have pat_enabled() properly reflect state when running on Xen")
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- new patch
V3:
- move the call of mtrr_overwrite_state() to xen_pv_init_platform()
V4:
- only call mtrr_overwrite_state() if any MTRR got from Xen
  (Boris Ostrovsky)
---
 arch/x86/xen/enlighten_pv.c | 52 +++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
index bb59cc6ddb2d..12e6b6845870 100644
--- a/arch/x86/xen/enlighten_pv.c
+++ b/arch/x86/xen/enlighten_pv.c
@@ -68,6 +68,7 @@
 #include <asm/reboot.h>
 #include <asm/hypervisor.h>
 #include <asm/mach_traps.h>
+#include <asm/mtrr.h>
 #include <asm/mwait.h>
 #include <asm/pci_x86.h>
 #include <asm/cpu.h>
@@ -119,6 +120,54 @@ static int __init parse_xen_msr_safe(char *str)
 }
 early_param("xen_msr_safe", parse_xen_msr_safe);
 
+/* Get MTRR settings from Xen and put them into mtrr_state. */
+static void __init xen_set_mtrr_data(void)
+{
+#ifdef CONFIG_MTRR
+	struct xen_platform_op op = {
+		.cmd = XENPF_read_memtype,
+		.interface_version = XENPF_INTERFACE_VERSION,
+	};
+	unsigned int reg;
+	unsigned long mask;
+	uint32_t eax, width;
+	static struct mtrr_var_range var[MTRR_MAX_VAR_RANGES] __initdata;
+
+	/* Get physical address width (only 64-bit cpus supported). */
+	width = 36;
+	eax = cpuid_eax(0x80000000);
+	if ((eax >> 16) == 0x8000 && eax >= 0x80000008) {
+		eax = cpuid_eax(0x80000008);
+		width = eax & 0xff;
+	}
+
+	for (reg = 0; reg < MTRR_MAX_VAR_RANGES; reg++) {
+		op.u.read_memtype.reg = reg;
+		if (HYPERVISOR_platform_op(&op))
+			break;
+
+		/*
+		 * Only called in dom0, which has all RAM PFNs mapped at
+		 * RAM MFNs, and all PCI space etc. is identity mapped.
+		 * This means we can treat MFN == PFN regarding MTTR settings.
+		 */
+		var[reg].base_lo = op.u.read_memtype.type;
+		var[reg].base_lo |= op.u.read_memtype.mfn << PAGE_SHIFT;
+		var[reg].base_hi = op.u.read_memtype.mfn >> (32 - PAGE_SHIFT);
+		mask = ~((op.u.read_memtype.nr_mfns << PAGE_SHIFT) - 1);
+		mask &= (1UL << width) - 1;
+		if (mask)
+			mask |= 1 << 11;
+		var[reg].mask_lo = mask;
+		var[reg].mask_hi = mask >> 32;
+	}
+
+	/* Only overwrite MTRR state if any MTRR could be got from Xen. */
+	if (reg)
+		mtrr_overwrite_state(var, reg, MTRR_TYPE_UNCACHABLE);
+#endif
+}
+
 static void __init xen_pv_init_platform(void)
 {
 	/* PV guests can't operate virtio devices without grants. */
@@ -135,6 +184,9 @@ static void __init xen_pv_init_platform(void)
 
 	/* pvclock is in shared info area */
 	xen_init_time_ops();
+
+	if (xen_initial_domain())
+		xen_set_mtrr_data();
 }
 
 static void __init xen_pv_guest_late_init(void)
-- 
2.35.3

Re: [PATCH v4 05/12] x86/xen: set MTRR state when running as Xen PV initial domain

[Boris Ostrovsky]

On 3/6/23 11:34 AM, Juergen Gross wrote:
> When running as Xen PV initial domain (aka dom0), MTRRs are disabled
> by the hypervisor, but the system should nevertheless use correct
> cache memory types. This has always kind of worked, as disabled MTRRs
> resulted in disabled PAT, too, so that the kernel avoided code paths
> resulting in inconsistencies. This bypassed all of the sanity checks
> the kernel is doing with enabled MTRRs in order to avoid memory
> mappings with conflicting memory types.
> 
> This has been changed recently, leading to PAT being accepted to be
> enabled, while MTRRs stayed disabled. The result is that
> mtrr_type_lookup() no longer is accepting all memory type requests,
> but started to return WB even if UC- was requested. This led to
> driver failures during initialization of some devices.
> 
> In reality MTRRs are still in effect, but they are under complete
> control of the Xen hypervisor. It is possible, however, to retrieve
> the MTRR settings from the hypervisor.
> 
> In order to fix those problems, overwrite the MTRR state via
> mtrr_overwrite_state() with the MTRR data from the hypervisor, if the
> system is running as a Xen dom0.
> 
> Fixes: 72cbc8f04fe2 ("x86/PAT: Have pat_enabled() properly reflect state when running on Xen")
> Signed-off-by: Juergen Gross <jgross@suse.com>


Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>

Re: [PATCH v4 05/12] x86/xen: set MTRR state when running as Xen PV initial domain

[Borislav Petkov]

On Mon, Mar 06, 2023 at 05:34:18PM +0100, Juergen Gross wrote:
> +	for (reg = 0; reg < MTRR_MAX_VAR_RANGES; reg++) {
> +		op.u.read_memtype.reg = reg;
> +		if (HYPERVISOR_platform_op(&op))
> +			break;
> +
> +		/*
> +		 * Only called in dom0, which has all RAM PFNs mapped at
> +		 * RAM MFNs, and all PCI space etc. is identity mapped.
> +		 * This means we can treat MFN == PFN regarding MTTR settings.
								^^^^

"MTRR"

-- 
Regards/Gruss,
    Boris.

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

[PATCH v4 06/12] x86/mtrr: replace vendor tests in MTRR code

[Juergen Gross]

Modern CPUs all share the same MTRR interface implemented via
generic_mtrr_ops.

At several places in MTRR code this generic interface is deduced via
is_cpu(INTEL) tests, which is only working due to X86_VENDOR_INTEL
being 0 (the is_cpu() macro is testing mtrr_if->vendor, which isn't
explicitly set in generic_mtrr_ops).

Fix that by replacing the is_cpu(INTEL) tests with testing for mtrr_if
to be &generic_mtrr_ops.

The only other place where the .vendor member of struct mtrr_ops is
being used is in set_num_var_ranges(), where depending on the vendor
the number of MTRR registers is determined. This can easily be changed
by replacing .vendor with the static number of MTRR registers.

It should be noted that the test "is_cpu(HYGON)" wasn't ever returning
true, as there is no struct mtrr_ops with that vendor information.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V3:
- new patch
---
 arch/x86/kernel/cpu/mtrr/amd.c     | 2 +-
 arch/x86/kernel/cpu/mtrr/centaur.c | 2 +-
 arch/x86/kernel/cpu/mtrr/cleanup.c | 4 ++--
 arch/x86/kernel/cpu/mtrr/cyrix.c   | 2 +-
 arch/x86/kernel/cpu/mtrr/generic.c | 2 +-
 arch/x86/kernel/cpu/mtrr/mtrr.c    | 8 +++-----
 arch/x86/kernel/cpu/mtrr/mtrr.h    | 4 +---
 7 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/arch/x86/kernel/cpu/mtrr/amd.c b/arch/x86/kernel/cpu/mtrr/amd.c
index eff6ac62c0ff..ef3e8e42b782 100644
--- a/arch/x86/kernel/cpu/mtrr/amd.c
+++ b/arch/x86/kernel/cpu/mtrr/amd.c
@@ -110,7 +110,7 @@ amd_validate_add_page(unsigned long base, unsigned long size, unsigned int type)
 }
 
 const struct mtrr_ops amd_mtrr_ops = {
-	.vendor            = X86_VENDOR_AMD,
+	.var_regs          = 2,
 	.set               = amd_set_mtrr,
 	.get               = amd_get_mtrr,
 	.get_free_region   = generic_get_free_region,
diff --git a/arch/x86/kernel/cpu/mtrr/centaur.c b/arch/x86/kernel/cpu/mtrr/centaur.c
index b8a74eddde83..4466ddeb0125 100644
--- a/arch/x86/kernel/cpu/mtrr/centaur.c
+++ b/arch/x86/kernel/cpu/mtrr/centaur.c
@@ -112,7 +112,7 @@ centaur_validate_add_page(unsigned long base, unsigned long size, unsigned int t
 }
 
 const struct mtrr_ops centaur_mtrr_ops = {
-	.vendor            = X86_VENDOR_CENTAUR,
+	.var_regs          = 8,
 	.set               = centaur_set_mcr,
 	.get               = centaur_get_mcr,
 	.get_free_region   = centaur_get_free_region,
diff --git a/arch/x86/kernel/cpu/mtrr/cleanup.c b/arch/x86/kernel/cpu/mtrr/cleanup.c
index b5f43049fa5f..1c2c0c252fa5 100644
--- a/arch/x86/kernel/cpu/mtrr/cleanup.c
+++ b/arch/x86/kernel/cpu/mtrr/cleanup.c
@@ -689,7 +689,7 @@ int __init mtrr_cleanup(unsigned address_bits)
 	int index_good;
 	int i;
 
-	if (!is_cpu(INTEL) || enable_mtrr_cleanup < 1)
+	if (mtrr_if != &generic_mtrr_ops || enable_mtrr_cleanup < 1)
 		return 0;
 
 	rdmsr(MSR_MTRRdefType, def, dummy);
@@ -886,7 +886,7 @@ int __init mtrr_trim_uncached_memory(unsigned long end_pfn)
 	 * Make sure we only trim uncachable memory on machines that
 	 * support the Intel MTRR architecture:
 	 */
-	if (!is_cpu(INTEL) || disable_mtrr_trim)
+	if (mtrr_if != &generic_mtrr_ops || disable_mtrr_trim)
 		return 0;
 
 	rdmsr(MSR_MTRRdefType, def, dummy);
diff --git a/arch/x86/kernel/cpu/mtrr/cyrix.c b/arch/x86/kernel/cpu/mtrr/cyrix.c
index 173b9e01e623..238dad57d4d6 100644
--- a/arch/x86/kernel/cpu/mtrr/cyrix.c
+++ b/arch/x86/kernel/cpu/mtrr/cyrix.c
@@ -235,7 +235,7 @@ static void cyrix_set_arr(unsigned int reg, unsigned long base,
 }
 
 const struct mtrr_ops cyrix_mtrr_ops = {
-	.vendor            = X86_VENDOR_CYRIX,
+	.var_regs          = 8,
 	.set               = cyrix_set_arr,
 	.get               = cyrix_get_arr,
 	.get_free_region   = cyrix_get_free_region,
diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
index 49b4cc923312..e25a44c2c950 100644
--- a/arch/x86/kernel/cpu/mtrr/generic.c
+++ b/arch/x86/kernel/cpu/mtrr/generic.c
@@ -827,7 +827,7 @@ int generic_validate_add_page(unsigned long base, unsigned long size,
 	 * For Intel PPro stepping <= 7
 	 * must be 4 MiB aligned and not touch 0x70000000 -> 0x7003FFFF
 	 */
-	if (is_cpu(INTEL) && boot_cpu_data.x86 == 6 &&
+	if (mtrr_if == &generic_mtrr_ops && boot_cpu_data.x86 == 6 &&
 	    boot_cpu_data.x86_model == 1 &&
 	    boot_cpu_data.x86_stepping <= 7) {
 		if (base & ((1 << (22 - PAGE_SHIFT)) - 1)) {
diff --git a/arch/x86/kernel/cpu/mtrr/mtrr.c b/arch/x86/kernel/cpu/mtrr/mtrr.c
index 5fe62ee0361b..0c83990501f5 100644
--- a/arch/x86/kernel/cpu/mtrr/mtrr.c
+++ b/arch/x86/kernel/cpu/mtrr/mtrr.c
@@ -108,14 +108,12 @@ static int have_wrcomb(void)
 /*  This function returns the number of variable MTRRs  */
 static void __init set_num_var_ranges(bool use_generic)
 {
-	unsigned long config = 0, dummy;
+	unsigned long config, dummy;
 
 	if (use_generic)
 		rdmsr(MSR_MTRRcap, config, dummy);
-	else if (is_cpu(AMD) || is_cpu(HYGON))
-		config = 2;
-	else if (is_cpu(CYRIX) || is_cpu(CENTAUR))
-		config = 8;
+	else
+		config = mtrr_if->var_regs;
 
 	num_var_ranges = config & 0xff;
 }
diff --git a/arch/x86/kernel/cpu/mtrr/mtrr.h b/arch/x86/kernel/cpu/mtrr/mtrr.h
index 02eb5871492d..a3c362d3d5bf 100644
--- a/arch/x86/kernel/cpu/mtrr/mtrr.h
+++ b/arch/x86/kernel/cpu/mtrr/mtrr.h
@@ -13,7 +13,7 @@
 extern unsigned int mtrr_usage_table[MTRR_MAX_VAR_RANGES];
 
 struct mtrr_ops {
-	u32	vendor;
+	u32	var_regs;
 	void	(*set)(unsigned int reg, unsigned long base,
 		       unsigned long size, mtrr_type type);
 	void	(*get)(unsigned int reg, unsigned long *base,
@@ -54,8 +54,6 @@ bool get_mtrr_state(void);
 extern u64 size_or_mask, size_and_mask;
 extern const struct mtrr_ops *mtrr_if;
 
-#define is_cpu(vnd)	(mtrr_if && mtrr_if->vendor == X86_VENDOR_##vnd)
-
 extern unsigned int num_var_ranges;
 extern u64 mtrr_tom2;
 extern struct mtrr_state_type mtrr_state;
-- 
2.35.3

Re: [PATCH v4 06/12] x86/mtrr: replace vendor tests in MTRR code

[Borislav Petkov]

On Mon, Mar 06, 2023 at 05:34:19PM +0100, Juergen Gross wrote:
> Modern CPUs all share the same MTRR interface implemented via
> generic_mtrr_ops.
> 
> At several places in MTRR code this generic interface is deduced via
> is_cpu(INTEL) tests, which is only working due to X86_VENDOR_INTEL
> being 0 (the is_cpu() macro is testing mtrr_if->vendor, which isn't
> explicitly set in generic_mtrr_ops).
> 
> Fix that by replacing the is_cpu(INTEL) tests with testing for mtrr_if
> to be &generic_mtrr_ops.

Two things:

* is_cpu() checks also whether mtrr_if is set. And we don't set it for
all vendors. I wanted to replace that thing with a vendor check recently
but there's that little issue.

I guess for the cases where we have the generic MTRR implementation, we
can safely assume that mtrr_if is set. Which leads me to the second
thing:

* If you're going to test for &generic_mtrr_ops, then you can just as
well do

	cpu_feature_enabled(X86_FEATURE_MTRR)

which is a lot more telling.

> diff --git a/arch/x86/kernel/cpu/mtrr/mtrr.c b/arch/x86/kernel/cpu/mtrr/mtrr.c
> index 5fe62ee0361b..0c83990501f5 100644
> --- a/arch/x86/kernel/cpu/mtrr/mtrr.c
> +++ b/arch/x86/kernel/cpu/mtrr/mtrr.c
> @@ -108,14 +108,12 @@ static int have_wrcomb(void)
>  /*  This function returns the number of variable MTRRs  */
>  static void __init set_num_var_ranges(bool use_generic)
>  {
> -	unsigned long config = 0, dummy;
> +	unsigned long config, dummy;
>  
>  	if (use_generic)
>  		rdmsr(MSR_MTRRcap, config, dummy);
> -	else if (is_cpu(AMD) || is_cpu(HYGON))
> -		config = 2;
> -	else if (is_cpu(CYRIX) || is_cpu(CENTAUR))
> -		config = 8;
> +	else
> +		config = mtrr_if->var_regs;
>  
>  	num_var_ranges = config & 0xff;
>  }

Since you're touching this function, you might simply expand its body in
its only call site in mtrr_bp_init(), put a comment above the expanded
code and remove that function.

That is, if we're going to do the ->var_regs thing.

Thx.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v4 06/12] x86/mtrr: replace vendor tests in MTRR code

[Juergen Gross]

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

On 24.03.23 17:56, Borislav Petkov wrote:
> On Mon, Mar 06, 2023 at 05:34:19PM +0100, Juergen Gross wrote:
>> Modern CPUs all share the same MTRR interface implemented via
>> generic_mtrr_ops.
>>
>> At several places in MTRR code this generic interface is deduced via
>> is_cpu(INTEL) tests, which is only working due to X86_VENDOR_INTEL
>> being 0 (the is_cpu() macro is testing mtrr_if->vendor, which isn't
>> explicitly set in generic_mtrr_ops).
>>
>> Fix that by replacing the is_cpu(INTEL) tests with testing for mtrr_if
>> to be &generic_mtrr_ops.
> 
> Two things:
> 
> * is_cpu() checks also whether mtrr_if is set. And we don't set it for
> all vendors. I wanted to replace that thing with a vendor check recently
> but there's that little issue.

The is_cpu() checks are either in functions reachable only with mtrr_if being
set, or are testing for INTEL, which is replaced by the test of mtrr_if being
&generic_mtrr_ops as written in the commit message.

> I guess for the cases where we have the generic MTRR implementation, we
> can safely assume that mtrr_if is set. Which leads me to the second
> thing:
> 
> * If you're going to test for &generic_mtrr_ops, then you can just as
> well do
> 
> 	cpu_feature_enabled(X86_FEATURE_MTRR)
> 
> which is a lot more telling.

Yes, I think this is true.

> 
>> diff --git a/arch/x86/kernel/cpu/mtrr/mtrr.c b/arch/x86/kernel/cpu/mtrr/mtrr.c
>> index 5fe62ee0361b..0c83990501f5 100644
>> --- a/arch/x86/kernel/cpu/mtrr/mtrr.c
>> +++ b/arch/x86/kernel/cpu/mtrr/mtrr.c
>> @@ -108,14 +108,12 @@ static int have_wrcomb(void)
>>   /*  This function returns the number of variable MTRRs  */
>>   static void __init set_num_var_ranges(bool use_generic)
>>   {
>> -	unsigned long config = 0, dummy;
>> +	unsigned long config, dummy;
>>   
>>   	if (use_generic)
>>   		rdmsr(MSR_MTRRcap, config, dummy);
>> -	else if (is_cpu(AMD) || is_cpu(HYGON))
>> -		config = 2;
>> -	else if (is_cpu(CYRIX) || is_cpu(CENTAUR))
>> -		config = 8;
>> +	else
>> +		config = mtrr_if->var_regs;
>>   
>>   	num_var_ranges = config & 0xff;
>>   }
> 
> Since you're touching this function, you might simply expand its body in
> its only call site in mtrr_bp_init(), put a comment above the expanded
> code and remove that function.

Okay.


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

Re: [PATCH v4 06/12] x86/mtrr: replace vendor tests in MTRR code

[Borislav Petkov]

On Mon, Mar 27, 2023 at 07:43:58AM +0200, Juergen Gross wrote:
> The is_cpu() checks are either in functions reachable only with mtrr_if being
> set, or are testing for INTEL, which is replaced by the test of mtrr_if being
> &generic_mtrr_ops as written in the commit message.

I went through all call sites... it seems like what you're saying should
work. I guess this mtrr_if check was added as a precaution in 2002 as
part of a cleanup:

commit 8fbdcb188e31ac901e216b466b97e90e8b057daa
Author: Dave Jones <davej@suse.de>
Date:   Wed Aug 14 21:14:22 2002 -0700

    [PATCH] Modular x86 MTRR driver.
    
    This patch from Pat Mochel cleans up the hell that was mtrr.c
    into something a lot more modular and easy to understand, by
    doing the implementation-per-file as has been done to various
    other things by Pat and myself over the last months.
    
    It's functionally identical from a kernel internal point of view,
    and a userspace point of view, and is basically just a very large
    code clean up.

So let's see what catches fire...

Thx.

-- 
Regards/Gruss,
    Boris.

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

[PATCH v4 07/12] x86/mtrr: allocate mtrr_value array dynamically

[Juergen Gross]

The mtrr_value[] array is a static variable, which is used only in a
few configurations. Consuming 6kB is ridiculous for this case,
especially as the array doesn't need to be that large and it can easily
be allocated dynamically.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 arch/x86/kernel/cpu/mtrr/mtrr.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/mtrr/mtrr.c b/arch/x86/kernel/cpu/mtrr/mtrr.c
index 0c83990501f5..50cd2287b6e1 100644
--- a/arch/x86/kernel/cpu/mtrr/mtrr.c
+++ b/arch/x86/kernel/cpu/mtrr/mtrr.c
@@ -581,7 +581,7 @@ struct mtrr_value {
 	unsigned long	lsize;
 };
 
-static struct mtrr_value mtrr_value[MTRR_MAX_VAR_RANGES];
+static struct mtrr_value *mtrr_value;
 
 static int mtrr_save(void)
 {
@@ -750,6 +750,7 @@ static int __init mtrr_init_finialize(void)
 	 * TBD: is there any system with such CPU which supports
 	 * suspend/resume? If no, we should remove the code.
 	 */
+	mtrr_value = kcalloc(num_var_ranges, sizeof(*mtrr_value), GFP_KERNEL);
 	register_syscore_ops(&mtrr_syscore_ops);
 
 	return 0;
-- 
2.35.3

Re: [PATCH v4 07/12] x86/mtrr: allocate mtrr_value array dynamically

[Huang, Kai]

On Mon, 2023-03-06 at 17:34 +0100, Juergen Gross wrote:
> The mtrr_value[] array is a static variable, which is used only in a
> few configurations. Consuming 6kB is ridiculous for this case,
> especially as the array doesn't need to be that large and it can easily
> be allocated dynamically.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  arch/x86/kernel/cpu/mtrr/mtrr.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/mtrr/mtrr.c b/arch/x86/kernel/cpu/mtrr/mtrr.c
> index 0c83990501f5..50cd2287b6e1 100644
> --- a/arch/x86/kernel/cpu/mtrr/mtrr.c
> +++ b/arch/x86/kernel/cpu/mtrr/mtrr.c
> @@ -581,7 +581,7 @@ struct mtrr_value {
>  	unsigned long	lsize;
>  };
>  
> -static struct mtrr_value mtrr_value[MTRR_MAX_VAR_RANGES];
> +static struct mtrr_value *mtrr_value;
>  
>  static int mtrr_save(void)
>  {
> @@ -750,6 +750,7 @@ static int __init mtrr_init_finialize(void)
>  	 * TBD: is there any system with such CPU which supports
>  	 * suspend/resume? If no, we should remove the code.
>  	 */
> +	mtrr_value = kcalloc(num_var_ranges, sizeof(*mtrr_value), GFP_KERNEL);

Theoretically dynamic allocation can fail, although it should not happen as this
happens during kernel boot and the size is small.  Maybe a WARN()?

>  	register_syscore_ops(&mtrr_syscore_ops);
>  
>  	return 0;

Re: [PATCH v4 07/12] x86/mtrr: allocate mtrr_value array dynamically

[Juergen Gross]

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

On 20.03.23 13:25, Huang, Kai wrote:
> On Mon, 2023-03-06 at 17:34 +0100, Juergen Gross wrote:
>> The mtrr_value[] array is a static variable, which is used only in a
>> few configurations. Consuming 6kB is ridiculous for this case,
>> especially as the array doesn't need to be that large and it can easily
>> be allocated dynamically.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>   arch/x86/kernel/cpu/mtrr/mtrr.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kernel/cpu/mtrr/mtrr.c b/arch/x86/kernel/cpu/mtrr/mtrr.c
>> index 0c83990501f5..50cd2287b6e1 100644
>> --- a/arch/x86/kernel/cpu/mtrr/mtrr.c
>> +++ b/arch/x86/kernel/cpu/mtrr/mtrr.c
>> @@ -581,7 +581,7 @@ struct mtrr_value {
>>   	unsigned long	lsize;
>>   };
>>   
>> -static struct mtrr_value mtrr_value[MTRR_MAX_VAR_RANGES];
>> +static struct mtrr_value *mtrr_value;
>>   
>>   static int mtrr_save(void)
>>   {
>> @@ -750,6 +750,7 @@ static int __init mtrr_init_finialize(void)
>>   	 * TBD: is there any system with such CPU which supports
>>   	 * suspend/resume? If no, we should remove the code.
>>   	 */
>> +	mtrr_value = kcalloc(num_var_ranges, sizeof(*mtrr_value), GFP_KERNEL);
> 
> Theoretically dynamic allocation can fail, although it should not happen as this
> happens during kernel boot and the size is small.  Maybe a WARN()?

Fine with me.


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

Re: [PATCH v4 07/12] x86/mtrr: allocate mtrr_value array dynamically

[Dave Hansen]

On 3/20/23 06:49, Juergen Gross wrote:
>>>
>>> @@ -750,6 +750,7 @@ static int __init mtrr_init_finialize(void)
>>>        * TBD: is there any system with such CPU which supports
>>>        * suspend/resume? If no, we should remove the code.
>>>        */
>>> +    mtrr_value = kcalloc(num_var_ranges, sizeof(*mtrr_value),
>>> GFP_KERNEL);
>>
>> Theoretically dynamic allocation can fail, although it should not
>> happen as this
>> happens during kernel boot and the size is small.  Maybe a WARN()?
> 
> Fine with me.

What *actually* happens if the system is running out of memory and this
is the _first_ failure?  Does a WARN_ON() here help someone debug what
is going on?

Re: [PATCH v4 07/12] x86/mtrr: allocate mtrr_value array dynamically

[Juergen Gross]

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

On 20.03.23 16:31, Dave Hansen wrote:
> On 3/20/23 06:49, Juergen Gross wrote:
>>>>
>>>> @@ -750,6 +750,7 @@ static int __init mtrr_init_finialize(void)
>>>>         * TBD: is there any system with such CPU which supports
>>>>         * suspend/resume? If no, we should remove the code.
>>>>         */
>>>> +    mtrr_value = kcalloc(num_var_ranges, sizeof(*mtrr_value),
>>>> GFP_KERNEL);
>>>
>>> Theoretically dynamic allocation can fail, although it should not
>>> happen as this
>>> happens during kernel boot and the size is small.  Maybe a WARN()?
>>
>> Fine with me.
> 
> What *actually* happens if the system is running out of memory and this
> is the _first_ failure?  Does a WARN_ON() here help someone debug what
> is going on?

Good question. Especially as we don't set __GFP_NOWARN here.


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

Re: [PATCH v4 07/12] x86/mtrr: allocate mtrr_value array dynamically

[Borislav Petkov]

On Mon, Mar 06, 2023 at 05:34:20PM +0100, Juergen Gross wrote:
> The mtrr_value[] array is a static variable, which is used only in a
> few configurations. Consuming 6kB is ridiculous for this case,

Ah, that struct mtrr_value is of size 24 due to that first member
mtrr_type getting padded even if it is a u8.

> especially as the array doesn't need to be that large and it can easily
> be allocated dynamically.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
>  arch/x86/kernel/cpu/mtrr/mtrr.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/mtrr/mtrr.c b/arch/x86/kernel/cpu/mtrr/mtrr.c
> index 0c83990501f5..50cd2287b6e1 100644
> --- a/arch/x86/kernel/cpu/mtrr/mtrr.c
> +++ b/arch/x86/kernel/cpu/mtrr/mtrr.c
> @@ -581,7 +581,7 @@ struct mtrr_value {
>  	unsigned long	lsize;
>  };
>  
> -static struct mtrr_value mtrr_value[MTRR_MAX_VAR_RANGES];
> +static struct mtrr_value *mtrr_value;
>  
>  static int mtrr_save(void)
>  {
> @@ -750,6 +750,7 @@ static int __init mtrr_init_finialize(void)
>  	 * TBD: is there any system with such CPU which supports
>  	 * suspend/resume? If no, we should remove the code.
>  	 */
> +	mtrr_value = kcalloc(num_var_ranges, sizeof(*mtrr_value), GFP_KERNEL);

Pls put that over the comment.

Also, you need to handle kcalloc() returning an error.

-- 
Regards/Gruss,
    Boris.

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

Re: [PATCH v4 07/12] x86/mtrr: allocate mtrr_value array dynamically

[Juergen Gross]

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

On 27.03.23 00:05, Borislav Petkov wrote:
> On Mon, Mar 06, 2023 at 05:34:20PM +0100, Juergen Gross wrote:
>> The mtrr_value[] array is a static variable, which is used only in a
>> few configurations. Consuming 6kB is ridiculous for this case,
> 
> Ah, that struct mtrr_value is of size 24 due to that first member
> mtrr_type getting padded even if it is a u8.
> 
>> especially as the array doesn't need to be that large and it can easily
>> be allocated dynamically.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>>   arch/x86/kernel/cpu/mtrr/mtrr.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kernel/cpu/mtrr/mtrr.c b/arch/x86/kernel/cpu/mtrr/mtrr.c
>> index 0c83990501f5..50cd2287b6e1 100644
>> --- a/arch/x86/kernel/cpu/mtrr/mtrr.c
>> +++ b/arch/x86/kernel/cpu/mtrr/mtrr.c
>> @@ -581,7 +581,7 @@ struct mtrr_value {
>>   	unsigned long	lsize;
>>   };
>>   
>> -static struct mtrr_value mtrr_value[MTRR_MAX_VAR_RANGES];
>> +static struct mtrr_value *mtrr_value;
>>   
>>   static int mtrr_save(void)
>>   {
>> @@ -750,6 +750,7 @@ static int __init mtrr_init_finialize(void)
>>   	 * TBD: is there any system with such CPU which supports
>>   	 * suspend/resume? If no, we should remove the code.
>>   	 */
>> +	mtrr_value = kcalloc(num_var_ranges, sizeof(*mtrr_value), GFP_KERNEL);
> 
> Pls put that over the comment.
> 
> Also, you need to handle kcalloc() returning an error.

Okay.


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

[PATCH v4 08/12] x86/mtrr: add get_effective_type() service function

[Juergen Gross]

Add a service function for obtaining the effective cache mode of
overlapping MTRR registers.

Make use of that function in check_type_overlap().

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V3:
- new patch
---
 arch/x86/kernel/cpu/mtrr/generic.c | 39 +++++++++++++++---------------
 1 file changed, 19 insertions(+), 20 deletions(-)

diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
index e25a44c2c950..d271e0c73775 100644
--- a/arch/x86/kernel/cpu/mtrr/generic.c
+++ b/arch/x86/kernel/cpu/mtrr/generic.c
@@ -78,31 +78,30 @@ static u64 get_mtrr_size(u64 mask)
 	return size;
 }
 
+static u8 get_effective_type(u8 type1, u8 type2)
+{
+	if (type1 == MTRR_TYPE_UNCACHABLE || type2 == MTRR_TYPE_UNCACHABLE)
+		return MTRR_TYPE_UNCACHABLE;
+
+	if ((type1 == MTRR_TYPE_WRBACK && type2 == MTRR_TYPE_WRTHROUGH) ||
+	    (type1 == MTRR_TYPE_WRTHROUGH && type2 == MTRR_TYPE_WRBACK))
+		return MTRR_TYPE_WRTHROUGH;
+
+	if (type1 != type2)
+		return MTRR_TYPE_UNCACHABLE;
+
+	return type1;
+}
+
 /*
  * Check and return the effective type for MTRR-MTRR type overlap.
- * Returns 1 if the effective type is UNCACHEABLE, else returns 0
+ * Returns true if the effective type is UNCACHEABLE, else returns false
  */
-static int check_type_overlap(u8 *prev, u8 *curr)
+static bool check_type_overlap(u8 *prev, u8 *curr)
 {
-	if (*prev == MTRR_TYPE_UNCACHABLE || *curr == MTRR_TYPE_UNCACHABLE) {
-		*prev = MTRR_TYPE_UNCACHABLE;
-		*curr = MTRR_TYPE_UNCACHABLE;
-		return 1;
-	}
-
-	if ((*prev == MTRR_TYPE_WRBACK && *curr == MTRR_TYPE_WRTHROUGH) ||
-	    (*prev == MTRR_TYPE_WRTHROUGH && *curr == MTRR_TYPE_WRBACK)) {
-		*prev = MTRR_TYPE_WRTHROUGH;
-		*curr = MTRR_TYPE_WRTHROUGH;
-	}
+	*prev = *curr = get_effective_type(*curr, *prev);
 
-	if (*prev != *curr) {
-		*prev = MTRR_TYPE_UNCACHABLE;
-		*curr = MTRR_TYPE_UNCACHABLE;
-		return 1;
-	}
-
-	return 0;
+	return *prev == MTRR_TYPE_UNCACHABLE;
 }
 
 /**
-- 
2.35.3

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

[Juergen Gross]

After MTRR initialization construct a memory map with cache modes from
MTRR values. This will speed up lookups via mtrr_lookup_type()
especially in case of overlapping MTRRs.

This will be needed when switching the semantics of the "uniform"
parameter of mtrr_lookup_type() from "only covered by one MTRR" to
"memory range has a uniform cache mode", which is the data the callers
really want to know. Today this information is not easily available,
in case MTRRs are not well sorted regarding base address.

The map will be built in __initdata. When memory management is up, the
map will be moved to dynamically allocated memory, in order to avoid
the need of an overly large array. The size of this array is calculated
using the number of variable MTRR registers and the needed size for
fixed entries.

Only add the map creation and expansion for now. The lookup will be
added later.

When writing new MTRR entries in the running system rebuild the map
inside the call from mtrr_rendezvous_handler() in order to avoid nasty
race conditions with concurrent lookups.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V3:
- new patch
---
 arch/x86/kernel/cpu/mtrr/generic.c | 254 +++++++++++++++++++++++++++++
 arch/x86/kernel/cpu/mtrr/mtrr.c    |   6 +-
 arch/x86/kernel/cpu/mtrr/mtrr.h    |   3 +
 3 files changed, 262 insertions(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
index d271e0c73775..13bc637a50e9 100644
--- a/arch/x86/kernel/cpu/mtrr/generic.c
+++ b/arch/x86/kernel/cpu/mtrr/generic.c
@@ -33,6 +33,37 @@ static struct fixed_range_block fixed_range_blocks[] = {
 	{}
 };
 
+struct cache_map {
+	u64 start;
+	u64 end;
+	u8 type;
+	bool fixed;
+};
+
+/*
+ * CACHE_MAP_MAX is the maximum number of memory ranges in cache_map, where
+ * no 2 adjacent ranges have the same cache mode (those would be merged).
+ * The number is based on the worst case:
+ * - no two adjacent fixed MTRRs share the same cache mode
+ * - one variable MTRR is spanning a huge area with mode WB
+ * - 255 variable MTRRs with mode UC all overlap with the WB MTRR, creating 2
+ *   additional ranges each (result like "ababababa...aba" with a = WB, b = UC),
+ *   accounting for MTRR_MAX_VAR_RANGES * 2 - 1 range entries
+ * - a TOM2 area (even with overlapping an UC MTRR can't add 2 range entries
+ *   to the possible maximum, as it always starts at 4GB, thus it can't be in
+ *   the middle of that MTRR, unless that MTRR starts at 0, which would remove
+ *   the initial "a" from the "abababa" pattern above)
+ * The map won't contain ranges with no matching MTRR (those fall back to the
+ * default cache mode).
+ */
+#define CACHE_MAP_MAX	(MTRR_NUM_FIXED_RANGES + MTRR_MAX_VAR_RANGES * 2)
+
+static struct cache_map init_cache_map[CACHE_MAP_MAX] __initdata;
+static struct cache_map *cache_map __refdata = init_cache_map;
+static unsigned int cache_map_size = CACHE_MAP_MAX;
+static unsigned int cache_map_n;
+static unsigned int cache_map_fixed;
+
 static unsigned long smp_changes_mask;
 static int mtrr_state_set;
 u64 mtrr_tom2;
@@ -78,6 +109,20 @@ static u64 get_mtrr_size(u64 mask)
 	return size;
 }
 
+static u8 get_var_mtrr_state(unsigned int reg, u64 *start, u64 *size)
+{
+	struct mtrr_var_range *mtrr = mtrr_state.var_ranges + reg;
+
+	if (!(mtrr->mask_lo & (1 << 11)))
+		return MTRR_TYPE_INVALID;
+
+	*start = (((u64)mtrr->base_hi) << 32) + (mtrr->base_lo & PAGE_MASK);
+	*size = get_mtrr_size((((u64)mtrr->mask_hi) << 32) +
+			      (mtrr->mask_lo & PAGE_MASK));
+
+	return mtrr->base_lo & 0xff;
+}
+
 static u8 get_effective_type(u8 type1, u8 type2)
 {
 	if (type1 == MTRR_TYPE_UNCACHABLE || type2 == MTRR_TYPE_UNCACHABLE)
@@ -241,6 +286,211 @@ static u8 mtrr_type_lookup_variable(u64 start, u64 end, u64 *partial_end,
 	return mtrr_state.def_type;
 }
 
+static void rm_map_entry_at(int idx)
+{
+	int i;
+
+	for (i = idx; i < cache_map_n - 1; i++)
+		cache_map[i] = cache_map[i + 1];
+
+	cache_map_n--;
+}
+
+/*
+ * Add an entry into cache_map at a specific index.
+ * Merges adjacent entries if appropriate.
+ * Return the number of merges for correcting the scan index.
+ */
+static int add_map_entry_at(u64 start, u64 end, u8 type, int idx)
+{
+	bool merge_prev, merge_next;
+	int i;
+
+	if (start >= end)
+		return 0;
+
+	merge_prev = (idx > 0 && !cache_map[idx - 1].fixed &&
+		      start == cache_map[idx - 1].end &&
+		      type == cache_map[idx - 1].type);
+	merge_next = (idx < cache_map_n && !cache_map[idx].fixed &&
+		      end == cache_map[idx].start &&
+		      type == cache_map[idx].type);
+
+	if (merge_prev && merge_next) {
+		cache_map[idx - 1].end = cache_map[idx].end;
+		rm_map_entry_at(idx);
+		return 2;
+	}
+	if (merge_prev) {
+		cache_map[idx - 1].end = end;
+		return 1;
+	}
+	if (merge_next) {
+		cache_map[idx].start = start;
+		return 1;
+	}
+
+	/* Sanity check: the array should NEVER be too small! */
+	if (cache_map_n == cache_map_size) {
+		WARN(1, "MTRR cache mode memory map exhausted!\n");
+		cache_map_n = cache_map_fixed;
+		return 0;
+	}
+
+	for (i = cache_map_n; i > idx; i--)
+		cache_map[i] = cache_map[i - 1];
+
+	cache_map[idx].start = start;
+	cache_map[idx].end = end;
+	cache_map[idx].type = type;
+	cache_map[idx].fixed = false;
+	cache_map_n++;
+
+	return 0;
+}
+
+/* Clear a part of an entry. Return 1 if start of entry is still valid. */
+static int clr_map_range_at(u64 start, u64 end, int idx)
+{
+	int ret = start != cache_map[idx].start;
+	u64 tmp;
+
+	if (start == cache_map[idx].start && end == cache_map[idx].end) {
+		rm_map_entry_at(idx);
+	} else if (start == cache_map[idx].start) {
+		cache_map[idx].start = end;
+	} else if (end == cache_map[idx].end) {
+		cache_map[idx].end = start;
+	} else {
+		tmp = cache_map[idx].end;
+		cache_map[idx].end = start;
+		add_map_entry_at(end, tmp, cache_map[idx].type, idx + 1);
+	}
+
+	return ret;
+}
+
+static void add_map_entry(u64 start, u64 end, u8 type)
+{
+	int i;
+	u8 new_type, old_type;
+	u64 tmp;
+
+	for (i = 0; i < cache_map_n && start < end; i++) {
+		if (start >= cache_map[i].end)
+			continue;
+
+		if (start < cache_map[i].start) {
+			/* Region start has no overlap. */
+			tmp = min(end, cache_map[i].start);
+			i -= add_map_entry_at(start, tmp,  type, i);
+			start = tmp;
+			continue;
+		}
+
+		new_type = get_effective_type(type, cache_map[i].type);
+		old_type = cache_map[i].type;
+
+		if (cache_map[i].fixed || new_type == old_type) {
+			/* Cut off start of new entry. */
+			start = cache_map[i].end;
+			continue;
+		}
+
+		tmp = min(end, cache_map[i].end);
+		i += clr_map_range_at(start, tmp, i);
+		i -= add_map_entry_at(start, tmp, new_type, i);
+		start = tmp;
+	}
+
+	add_map_entry_at(start, end, type, i);
+}
+
+/* Add variable MTRRs to cache map. */
+static void map_add_var(void)
+{
+	unsigned int i;
+	u64 start, size;
+	u8 type;
+
+	/* Add AMD magic MTRR. */
+	if (mtrr_tom2) {
+		add_map_entry(1ULL << 32, mtrr_tom2 - 1, MTRR_TYPE_WRBACK);
+		cache_map[cache_map_n - 1].fixed = true;
+	}
+
+	for (i = 0; i < num_var_ranges; i++) {
+		type = get_var_mtrr_state(i, &start, &size);
+		if (type != MTRR_TYPE_INVALID)
+			add_map_entry(start, start + size, type);
+	}
+}
+
+/* Rebuild map by replacing variable entries. */
+static void rebuild_map(void)
+{
+	cache_map_n = cache_map_fixed;
+
+	map_add_var();
+}
+
+/* Build the cache_map containing the cache modes per memory range. */
+void mtrr_build_map(void)
+{
+	unsigned int i;
+	u64 start, end, size;
+	u8 type;
+
+	if (!mtrr_state.enabled)
+		return;
+
+	/* Add fixed MTRRs, optimize for adjacent entries with same type. */
+	if (mtrr_state.enabled & MTRR_STATE_MTRR_FIXED_ENABLED) {
+		start = 0;
+		end = size = 0x10000;
+		type = mtrr_state.fixed_ranges[0];
+
+		for (i = 1; i < MTRR_NUM_FIXED_RANGES; i++) {
+			if (i == 8 || i == 24)
+				size >>= 2;
+
+			if (mtrr_state.fixed_ranges[i] != type) {
+				add_map_entry(start, end, type);
+				start = end;
+				type = mtrr_state.fixed_ranges[i];
+			}
+			end += size;
+		}
+		add_map_entry(start, end, type);
+	}
+
+	/* Mark fixed and magic MTRR as fixed, they take precedence. */
+	for (i = 0; i < cache_map_n; i++)
+		cache_map[i].fixed = true;
+	cache_map_fixed = cache_map_n;
+
+	map_add_var();
+}
+
+/* Copy the cache_map from __initdata memory to dynamically allocated one. */
+void __init mtrr_copy_map(void)
+{
+	unsigned int new_size = cache_map_fixed + 2 * num_var_ranges;
+
+	if (!mtrr_state.enabled || !new_size) {
+		cache_map = NULL;
+		return;
+	}
+
+	mutex_lock(&mtrr_mutex);
+
+	cache_map = kcalloc(new_size, sizeof(*cache_map), GFP_KERNEL);
+	memmove(cache_map, init_cache_map, cache_map_n * sizeof(*cache_map));
+	cache_map_size = new_size;
+
+	mutex_unlock(&mtrr_mutex);
+}
+
 /**
  * mtrr_overwrite_state - set static MTRR state
  *
@@ -815,6 +1065,10 @@ static void generic_set_mtrr(unsigned int reg, unsigned long base,
 
 	cache_enable();
 	local_irq_restore(flags);
+
+	/* On the first cpu rebuild the cache mode memory map. */
+	if (smp_processor_id() == cpumask_first(cpu_online_mask))
+		rebuild_map();
 }
 
 int generic_validate_add_page(unsigned long base, unsigned long size,
diff --git a/arch/x86/kernel/cpu/mtrr/mtrr.c b/arch/x86/kernel/cpu/mtrr/mtrr.c
index 50cd2287b6e1..1dbb9fdfd87b 100644
--- a/arch/x86/kernel/cpu/mtrr/mtrr.c
+++ b/arch/x86/kernel/cpu/mtrr/mtrr.c
@@ -65,7 +65,7 @@ static bool mtrr_enabled(void)
 }
 
 unsigned int mtrr_usage_table[MTRR_MAX_VAR_RANGES];
-static DEFINE_MUTEX(mtrr_mutex);
+DEFINE_MUTEX(mtrr_mutex);
 
 u64 size_or_mask, size_and_mask;
 
@@ -668,6 +668,7 @@ void __init mtrr_bp_init(void)
 		/* Software overwrite of MTRR state, only for generic case. */
 		mtrr_calc_physbits(true);
 		init_table();
+		mtrr_build_map();
 		pr_info("MTRRs set to read-only\n");
 
 		return;
@@ -705,6 +706,7 @@ void __init mtrr_bp_init(void)
 			if (get_mtrr_state()) {
 				memory_caching_control |= CACHE_MTRR;
 				changed_by_mtrr_cleanup = mtrr_cleanup(phys_addr);
+				mtrr_build_map();
 			} else {
 				mtrr_if = NULL;
 				why = "by BIOS";
@@ -733,6 +735,8 @@ void mtrr_save_state(void)
 
 static int __init mtrr_init_finialize(void)
 {
+	mtrr_copy_map();
+
 	if (!mtrr_enabled())
 		return 0;
 
diff --git a/arch/x86/kernel/cpu/mtrr/mtrr.h b/arch/x86/kernel/cpu/mtrr/mtrr.h
index a3c362d3d5bf..6246a1d8650b 100644
--- a/arch/x86/kernel/cpu/mtrr/mtrr.h
+++ b/arch/x86/kernel/cpu/mtrr/mtrr.h
@@ -53,6 +53,7 @@ bool get_mtrr_state(void);
 
 extern u64 size_or_mask, size_and_mask;
 extern const struct mtrr_ops *mtrr_if;
+extern struct mutex mtrr_mutex;
 
 extern unsigned int num_var_ranges;
 extern u64 mtrr_tom2;
@@ -61,6 +62,8 @@ extern struct mtrr_state_type mtrr_state;
 void mtrr_state_warn(void);
 const char *mtrr_attrib_to_str(int x);
 void mtrr_wrmsr(unsigned, unsigned, unsigned);
+void mtrr_build_map(void);
+void mtrr_copy_map(void);
 
 /* CPU specific mtrr_ops vectors. */
 extern const struct mtrr_ops amd_mtrr_ops;
-- 
2.35.3

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

[Borislav Petkov]

On Mon, Mar 06, 2023 at 05:34:22PM +0100, Juergen Gross wrote:
> +struct cache_map {
> +	u64 start;
> +	u64 end;
> +	u8 type;
> +	bool fixed;

Can those last two be a single

	u64 flags;

which contains the type and fixed and later perhaps other things so that we can
have those elements aligned and we don't waste space unnecessarily by having
a bool for a single bit of information?

> +};
> +
> +/*
> + * CACHE_MAP_MAX is the maximum number of memory ranges in cache_map, where
> + * no 2 adjacent ranges have the same cache mode (those would be merged).
> + * The number is based on the worst case:
> + * - no two adjacent fixed MTRRs share the same cache mode
> + * - one variable MTRR is spanning a huge area with mode WB
> + * - 255 variable MTRRs with mode UC all overlap with the WB MTRR, creating 2
> + *   additional ranges each (result like "ababababa...aba" with a = WB, b = UC),
> + *   accounting for MTRR_MAX_VAR_RANGES * 2 - 1 range entries
> + * - a TOM2 area (even with overlapping an UC MTRR can't add 2 range entries
> + *   to the possible maximum, as it always starts at 4GB, thus it can't be in
> + *   the middle of that MTRR, unless that MTRR starts at 0, which would remove
> + *   the initial "a" from the "abababa" pattern above)
> + * The map won't contain ranges with no matching MTRR (those fall back to the
> + * default cache mode).
> + */

Nice.

> +#define CACHE_MAP_MAX	(MTRR_NUM_FIXED_RANGES + MTRR_MAX_VAR_RANGES * 2)
> +
> +static struct cache_map init_cache_map[CACHE_MAP_MAX] __initdata;
> +static struct cache_map *cache_map __refdata = init_cache_map;
> +static unsigned int cache_map_size = CACHE_MAP_MAX;
> +static unsigned int cache_map_n;
> +static unsigned int cache_map_fixed;
> +
>  static unsigned long smp_changes_mask;
>  static int mtrr_state_set;
>  u64 mtrr_tom2;
> @@ -78,6 +109,20 @@ static u64 get_mtrr_size(u64 mask)
>  	return size;
>  }
>  
> +static u8 get_var_mtrr_state(unsigned int reg, u64 *start, u64 *size)
> +{
> +	struct mtrr_var_range *mtrr = mtrr_state.var_ranges + reg;
> +
> +	if (!(mtrr->mask_lo & (1 << 11)))

I'm guessing that's the

	MTRR Pair Enable

bit?

Use a macro with a proper name pls.

> +		return MTRR_TYPE_INVALID;
> +
> +	*start = (((u64)mtrr->base_hi) << 32) + (mtrr->base_lo & PAGE_MASK);
> +	*size = get_mtrr_size((((u64)mtrr->mask_hi) << 32) +
> +			      (mtrr->mask_lo & PAGE_MASK));
> +
> +	return mtrr->base_lo & 0xff;
> +}
> +
>  static u8 get_effective_type(u8 type1, u8 type2)
>  {
>  	if (type1 == MTRR_TYPE_UNCACHABLE || type2 == MTRR_TYPE_UNCACHABLE)
> @@ -241,6 +286,211 @@ static u8 mtrr_type_lookup_variable(u64 start, u64 end, u64 *partial_end,
>  	return mtrr_state.def_type;
>  }
>  
> +static void rm_map_entry_at(int idx)
> +{
> +	int i;
> +
> +	for (i = idx; i < cache_map_n - 1; i++)
> +		cache_map[i] = cache_map[i + 1];

That can be a memmove, I think. Something like

	memmove((void *)&cache_map[i],
		(void *)&cache_map[i + 1], 
		(cache_map_n - idx) * sizeof(struct cache_map));


> +
> +	cache_map_n--;
> +}
> +
> +/*
> + * Add an entry into cache_map at a specific index.
> + * Merges adjacent entries if appropriate.
> + * Return the number of merges for correcting the scan index.

Make that a block comment:

* Add an entry into cache_map at a specific index.  Merges adjacent entries if
* appropriate.  Return the number of merges for correcting the scan index.

vim, for example, has the cool "gq}" command for that.

> + */
> +static int add_map_entry_at(u64 start, u64 end, u8 type, int idx)
> +{
> +	bool merge_prev, merge_next;
> +	int i;
> +
> +	if (start >= end)
> +		return 0;
> +
> +	merge_prev = (idx > 0 && !cache_map[idx - 1].fixed &&
> +		      start == cache_map[idx - 1].end &&
> +		      type == cache_map[idx - 1].type);
> +	merge_next = (idx < cache_map_n && !cache_map[idx].fixed &&
> +		      end == cache_map[idx].start &&
> +		      type == cache_map[idx].type);

Uuh, head hurts. How about:

	bool merge_prev = false, merge_next = false;

	...

	if (idx > 0) {
		struct cache_map *prev = &cache_map[idx - 1];

		if (!prev->fixed &&
		     prev->end  == start &&
		     prev->type == type)
			merge_prev = true;
	}

Untested ofc, but you get the idea. It is a lot more readable this way. And the
same with merge_next.

> +
> +	if (merge_prev && merge_next) {
> +		cache_map[idx - 1].end = cache_map[idx].end;
> +		rm_map_entry_at(idx);
> +		return 2;
> +	}
> +	if (merge_prev) {
> +		cache_map[idx - 1].end = end;
> +		return 1;
> +	}
> +	if (merge_next) {
> +		cache_map[idx].start = start;
> +		return 1;
> +	}
> +
> +	/* Sanity check: the array should NEVER be too small! */
> +	if (cache_map_n == cache_map_size) {
> +		WARN(1, "MTRR cache mode memory map exhausted!\n");
> +		cache_map_n = cache_map_fixed;
> +		return 0;
> +	}
> +
> +	for (i = cache_map_n; i > idx; i--)
> +		cache_map[i] = cache_map[i - 1];

memmove as above.

> +
> +	cache_map[idx].start = start;
> +	cache_map[idx].end = end;
> +	cache_map[idx].type = type;
> +	cache_map[idx].fixed = false;
> +	cache_map_n++;
> +
> +	return 0;
> +}
> +
> +/* Clear a part of an entry. Return 1 if start of entry is still valid. */
> +static int clr_map_range_at(u64 start, u64 end, int idx)
> +{
> +	int ret = start != cache_map[idx].start;
> +	u64 tmp;
> +
> +	if (start == cache_map[idx].start && end == cache_map[idx].end) {
> +		rm_map_entry_at(idx);
> +	} else if (start == cache_map[idx].start) {
> +		cache_map[idx].start = end;
> +	} else if (end == cache_map[idx].end) {
> +		cache_map[idx].end = start;
> +	} else {
> +		tmp = cache_map[idx].end;
> +		cache_map[idx].end = start;
> +		add_map_entry_at(end, tmp, cache_map[idx].type, idx + 1);
> +	}
> +
> +	return ret;
> +}
> +
> +static void add_map_entry(u64 start, u64 end, u8 type)
> +{
> +	int i;
> +	u8 new_type, old_type;
> +	u64 tmp;

"int i;" goes here.

> +
> +	for (i = 0; i < cache_map_n && start < end; i++) {
> +		if (start >= cache_map[i].end)
> +			continue;
> +
> +		if (start < cache_map[i].start) {
> +			/* Region start has no overlap. */
> +			tmp = min(end, cache_map[i].start);
> +			i -= add_map_entry_at(start, tmp,  type, i);

Uff, what happens if i == 0 and becomes negative here?

Can that even happen?

This feels weird: using function retvals as index var decrements. I have
only a vague idea what you're doing in this function but it feels weird.
Maybe I need to play through an example to grok it better...

> +			start = tmp;
> +			continue;
> +		}
> +
> +		new_type = get_effective_type(type, cache_map[i].type);
> +		old_type = cache_map[i].type;
> +
> +		if (cache_map[i].fixed || new_type == old_type) {
> +			/* Cut off start of new entry. */
> +			start = cache_map[i].end;
> +			continue;
> +		}
> +
> +		tmp = min(end, cache_map[i].end);
> +		i += clr_map_range_at(start, tmp, i);
> +		i -= add_map_entry_at(start, tmp, new_type, i);
> +		start = tmp;
> +	}
> +
> +	add_map_entry_at(start, end, type, i);
> +}
> +
> +/* Add variable MTRRs to cache map. */
> +static void map_add_var(void)
> +{
> +	unsigned int i;
> +	u64 start, size;
> +	u8 type;
> +
> +	/* Add AMD magic MTRR. */

Why magic?

> +	if (mtrr_tom2) {
> +		add_map_entry(1ULL << 32, mtrr_tom2 - 1, MTRR_TYPE_WRBACK);

BIT_ULL(32)

> +		cache_map[cache_map_n - 1].fixed = true;
> +	}
> +
> +	for (i = 0; i < num_var_ranges; i++) {
> +		type = get_var_mtrr_state(i, &start, &size);
> +		if (type != MTRR_TYPE_INVALID)
> +			add_map_entry(start, start + size, type);
> +	}
> +}
> +
> +/* Rebuild map by replacing variable entries. */
> +static void rebuild_map(void)
> +{
> +	cache_map_n = cache_map_fixed;
> +
> +	map_add_var();
> +}
> +
> +/* Build the cache_map containing the cache modes per memory range. */
> +void mtrr_build_map(void)
> +{
> +	unsigned int i;
> +	u64 start, end, size;
> +	u8 type;
> +
> +	if (!mtrr_state.enabled)
> +		return;
> +
> +	/* Add fixed MTRRs, optimize for adjacent entries with same type. */
> +	if (mtrr_state.enabled & MTRR_STATE_MTRR_FIXED_ENABLED) {
> +		start = 0;
> +		end = size = 0x10000;
> +		type = mtrr_state.fixed_ranges[0];
> +
> +		for (i = 1; i < MTRR_NUM_FIXED_RANGES; i++) {
> +			if (i == 8 || i == 24)
> +				size >>= 2;
> +
> +			if (mtrr_state.fixed_ranges[i] != type) {
> +				add_map_entry(start, end, type);
> +				start = end;
> +				type = mtrr_state.fixed_ranges[i];
> +			}
> +			end += size;
> +		}
> +		add_map_entry(start, end, type);
> +	}
> +
> +	/* Mark fixed and magic MTRR as fixed, they take precedence. */
> +	for (i = 0; i < cache_map_n; i++)
> +		cache_map[i].fixed = true;
> +	cache_map_fixed = cache_map_n;
> +
> +	map_add_var();

I think it would be useful to issue some stats here:

	pr_info("MTRR map: ... size: ..., ... entries: ..., memory used: ....");

and so on so that we can get some feedback when that happens. We can always
drop it later but for the initial runs it would be prudent to have that.

> +/* Copy the cache_map from __initdata memory to dynamically allocated one. */
> +void __init mtrr_copy_map(void)
> +{
> +	unsigned int new_size = cache_map_fixed + 2 * num_var_ranges;
> +
> +	if (!mtrr_state.enabled || !new_size) {
> +		cache_map = NULL;
> +		return;
> +	}
> +
> +	mutex_lock(&mtrr_mutex);
> +
> +	cache_map = kcalloc(new_size, sizeof(*cache_map), GFP_KERNEL);
> +	memmove(cache_map, init_cache_map, cache_map_n * sizeof(*cache_map));
> +	cache_map_size = new_size;
> +
> +	mutex_unlock(&mtrr_mutex);
> +}
> +
>  /**
>   * mtrr_overwrite_state - set static MTRR state
>   *
> @@ -815,6 +1065,10 @@ static void generic_set_mtrr(unsigned int reg, unsigned long base,
>  
>  	cache_enable();
>  	local_irq_restore(flags);
> +
> +	/* On the first cpu rebuild the cache mode memory map. */

s/cpu/CPU/

> +	if (smp_processor_id() == cpumask_first(cpu_online_mask))

Why not in mtrr_bp_init()? That is the first CPU.

> +		rebuild_map();
>  }
>  
>  int generic_validate_add_page(unsigned long base, unsigned long size,
> diff --git a/arch/x86/kernel/cpu/mtrr/mtrr.c b/arch/x86/kernel/cpu/mtrr/mtrr.c
> index 50cd2287b6e1..1dbb9fdfd87b 100644
> --- a/arch/x86/kernel/cpu/mtrr/mtrr.c
> +++ b/arch/x86/kernel/cpu/mtrr/mtrr.c
> @@ -65,7 +65,7 @@ static bool mtrr_enabled(void)
>  }
>  
>  unsigned int mtrr_usage_table[MTRR_MAX_VAR_RANGES];
> -static DEFINE_MUTEX(mtrr_mutex);
> +DEFINE_MUTEX(mtrr_mutex);
>  
>  u64 size_or_mask, size_and_mask;
>  
> @@ -668,6 +668,7 @@ void __init mtrr_bp_init(void)
>  		/* Software overwrite of MTRR state, only for generic case. */
>  		mtrr_calc_physbits(true);
>  		init_table();
> +		mtrr_build_map();
>  		pr_info("MTRRs set to read-only\n");
>  
>  		return;
> @@ -705,6 +706,7 @@ void __init mtrr_bp_init(void)
>  			if (get_mtrr_state()) {
>  				memory_caching_control |= CACHE_MTRR;
>  				changed_by_mtrr_cleanup = mtrr_cleanup(phys_addr);
> +				mtrr_build_map();
>  			} else {
>  				mtrr_if = NULL;
>  				why = "by BIOS";
> @@ -733,6 +735,8 @@ void mtrr_save_state(void)
>  
>  static int __init mtrr_init_finialize(void)
>  {
> +	mtrr_copy_map();

Move that...

> +
>  	if (!mtrr_enabled())
>  		return 0;

... here.

So yeah, I like the general direction but this is a complex patch. Lemme
add some printks to it in order to get a better idea of what happens.

Thx.

-- 
Regards/Gruss,
    Boris.

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

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

[Juergen Gross]

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

On 29.03.23 14:51, Borislav Petkov wrote:
> On Mon, Mar 06, 2023 at 05:34:22PM +0100, Juergen Gross wrote:
>> +struct cache_map {
>> +	u64 start;
>> +	u64 end;
>> +	u8 type;
>> +	bool fixed;
> 
> Can those last two be a single
> 
> 	u64 flags;
> 
> which contains the type and fixed and later perhaps other things so that we can
> have those elements aligned and we don't waste space unnecessarily by having
> a bool for a single bit of information?

Yes, of course.

> 
>> +};
>> +
>> +/*
>> + * CACHE_MAP_MAX is the maximum number of memory ranges in cache_map, where
>> + * no 2 adjacent ranges have the same cache mode (those would be merged).
>> + * The number is based on the worst case:
>> + * - no two adjacent fixed MTRRs share the same cache mode
>> + * - one variable MTRR is spanning a huge area with mode WB
>> + * - 255 variable MTRRs with mode UC all overlap with the WB MTRR, creating 2
>> + *   additional ranges each (result like "ababababa...aba" with a = WB, b = UC),
>> + *   accounting for MTRR_MAX_VAR_RANGES * 2 - 1 range entries
>> + * - a TOM2 area (even with overlapping an UC MTRR can't add 2 range entries
>> + *   to the possible maximum, as it always starts at 4GB, thus it can't be in
>> + *   the middle of that MTRR, unless that MTRR starts at 0, which would remove
>> + *   the initial "a" from the "abababa" pattern above)
>> + * The map won't contain ranges with no matching MTRR (those fall back to the
>> + * default cache mode).
>> + */
> 
> Nice.
> 
>> +#define CACHE_MAP_MAX	(MTRR_NUM_FIXED_RANGES + MTRR_MAX_VAR_RANGES * 2)
>> +
>> +static struct cache_map init_cache_map[CACHE_MAP_MAX] __initdata;
>> +static struct cache_map *cache_map __refdata = init_cache_map;
>> +static unsigned int cache_map_size = CACHE_MAP_MAX;
>> +static unsigned int cache_map_n;
>> +static unsigned int cache_map_fixed;
>> +
>>   static unsigned long smp_changes_mask;
>>   static int mtrr_state_set;
>>   u64 mtrr_tom2;
>> @@ -78,6 +109,20 @@ static u64 get_mtrr_size(u64 mask)
>>   	return size;
>>   }
>>   
>> +static u8 get_var_mtrr_state(unsigned int reg, u64 *start, u64 *size)
>> +{
>> +	struct mtrr_var_range *mtrr = mtrr_state.var_ranges + reg;
>> +
>> +	if (!(mtrr->mask_lo & (1 << 11)))
> 
> I'm guessing that's the
> 
> 	MTRR Pair Enable
> 
> bit?
> 
> Use a macro with a proper name pls.

Okay.

> 
>> +		return MTRR_TYPE_INVALID;
>> +
>> +	*start = (((u64)mtrr->base_hi) << 32) + (mtrr->base_lo & PAGE_MASK);
>> +	*size = get_mtrr_size((((u64)mtrr->mask_hi) << 32) +
>> +			      (mtrr->mask_lo & PAGE_MASK));
>> +
>> +	return mtrr->base_lo & 0xff;
>> +}
>> +
>>   static u8 get_effective_type(u8 type1, u8 type2)
>>   {
>>   	if (type1 == MTRR_TYPE_UNCACHABLE || type2 == MTRR_TYPE_UNCACHABLE)
>> @@ -241,6 +286,211 @@ static u8 mtrr_type_lookup_variable(u64 start, u64 end, u64 *partial_end,
>>   	return mtrr_state.def_type;
>>   }
>>   
>> +static void rm_map_entry_at(int idx)
>> +{
>> +	int i;
>> +
>> +	for (i = idx; i < cache_map_n - 1; i++)
>> +		cache_map[i] = cache_map[i + 1];
> 
> That can be a memmove, I think. Something like
> 
> 	memmove((void *)&cache_map[i],
> 		(void *)&cache_map[i + 1],
> 		(cache_map_n - idx) * sizeof(struct cache_map));

Okay.

> 
> 
>> +
>> +	cache_map_n--;
>> +}
>> +
>> +/*
>> + * Add an entry into cache_map at a specific index.
>> + * Merges adjacent entries if appropriate.
>> + * Return the number of merges for correcting the scan index.
> 
> Make that a block comment:
> 
> * Add an entry into cache_map at a specific index.  Merges adjacent entries if
> * appropriate.  Return the number of merges for correcting the scan index.

Okay.

> 
> vim, for example, has the cool "gq}" command for that.
> 
>> + */
>> +static int add_map_entry_at(u64 start, u64 end, u8 type, int idx)
>> +{
>> +	bool merge_prev, merge_next;
>> +	int i;
>> +
>> +	if (start >= end)
>> +		return 0;
>> +
>> +	merge_prev = (idx > 0 && !cache_map[idx - 1].fixed &&
>> +		      start == cache_map[idx - 1].end &&
>> +		      type == cache_map[idx - 1].type);
>> +	merge_next = (idx < cache_map_n && !cache_map[idx].fixed &&
>> +		      end == cache_map[idx].start &&
>> +		      type == cache_map[idx].type);
> 
> Uuh, head hurts. How about:
> 
> 	bool merge_prev = false, merge_next = false;
> 
> 	...
> 
> 	if (idx > 0) {
> 		struct cache_map *prev = &cache_map[idx - 1];
> 
> 		if (!prev->fixed &&
> 		     prev->end  == start &&
> 		     prev->type == type)
> 			merge_prev = true;
> 	}
> 
> Untested ofc, but you get the idea. It is a lot more readable this way. And the
> same with merge_next.

Fine with me.

> 
>> +
>> +	if (merge_prev && merge_next) {
>> +		cache_map[idx - 1].end = cache_map[idx].end;
>> +		rm_map_entry_at(idx);
>> +		return 2;
>> +	}
>> +	if (merge_prev) {
>> +		cache_map[idx - 1].end = end;
>> +		return 1;
>> +	}
>> +	if (merge_next) {
>> +		cache_map[idx].start = start;
>> +		return 1;
>> +	}
>> +
>> +	/* Sanity check: the array should NEVER be too small! */
>> +	if (cache_map_n == cache_map_size) {
>> +		WARN(1, "MTRR cache mode memory map exhausted!\n");
>> +		cache_map_n = cache_map_fixed;
>> +		return 0;
>> +	}
>> +
>> +	for (i = cache_map_n; i > idx; i--)
>> +		cache_map[i] = cache_map[i - 1];
> 
> memmove as above.

Okay.

> 
>> +
>> +	cache_map[idx].start = start;
>> +	cache_map[idx].end = end;
>> +	cache_map[idx].type = type;
>> +	cache_map[idx].fixed = false;
>> +	cache_map_n++;
>> +
>> +	return 0;
>> +}
>> +
>> +/* Clear a part of an entry. Return 1 if start of entry is still valid. */
>> +static int clr_map_range_at(u64 start, u64 end, int idx)
>> +{
>> +	int ret = start != cache_map[idx].start;
>> +	u64 tmp;
>> +
>> +	if (start == cache_map[idx].start && end == cache_map[idx].end) {
>> +		rm_map_entry_at(idx);
>> +	} else if (start == cache_map[idx].start) {
>> +		cache_map[idx].start = end;
>> +	} else if (end == cache_map[idx].end) {
>> +		cache_map[idx].end = start;
>> +	} else {
>> +		tmp = cache_map[idx].end;
>> +		cache_map[idx].end = start;
>> +		add_map_entry_at(end, tmp, cache_map[idx].type, idx + 1);
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>> +static void add_map_entry(u64 start, u64 end, u8 type)
>> +{
>> +	int i;
>> +	u8 new_type, old_type;
>> +	u64 tmp;
> 
> "int i;" goes here.

Okay.

> 
>> +
>> +	for (i = 0; i < cache_map_n && start < end; i++) {
>> +		if (start >= cache_map[i].end)
>> +			continue;
>> +
>> +		if (start < cache_map[i].start) {
>> +			/* Region start has no overlap. */
>> +			tmp = min(end, cache_map[i].start);
>> +			i -= add_map_entry_at(start, tmp,  type, i);
> 
> Uff, what happens if i == 0 and becomes negative here?
> 
> Can that even happen?

No. :-)

> 
> This feels weird: using function retvals as index var decrements. I have
> only a vague idea what you're doing in this function but it feels weird.
> Maybe I need to play through an example to grok it better...

The final form of the code is the result of an iterative process. :-)

It looked much worse in the beginning.

> 
>> +			start = tmp;
>> +			continue;
>> +		}
>> +
>> +		new_type = get_effective_type(type, cache_map[i].type);
>> +		old_type = cache_map[i].type;
>> +
>> +		if (cache_map[i].fixed || new_type == old_type) {
>> +			/* Cut off start of new entry. */
>> +			start = cache_map[i].end;
>> +			continue;
>> +		}
>> +
>> +		tmp = min(end, cache_map[i].end);
>> +		i += clr_map_range_at(start, tmp, i);
>> +		i -= add_map_entry_at(start, tmp, new_type, i);
>> +		start = tmp;
>> +	}
>> +
>> +	add_map_entry_at(start, end, type, i);
>> +}
>> +
>> +/* Add variable MTRRs to cache map. */
>> +static void map_add_var(void)
>> +{
>> +	unsigned int i;
>> +	u64 start, size;
>> +	u8 type;
>> +
>> +	/* Add AMD magic MTRR. */
> 
> Why magic?

I've reused the wording from cleanup.c (just above amd_special_default_mtrr()).

> 
>> +	if (mtrr_tom2) {
>> +		add_map_entry(1ULL << 32, mtrr_tom2 - 1, MTRR_TYPE_WRBACK);
> 
> BIT_ULL(32)

Okay.

> 
>> +		cache_map[cache_map_n - 1].fixed = true;
>> +	}
>> +
>> +	for (i = 0; i < num_var_ranges; i++) {
>> +		type = get_var_mtrr_state(i, &start, &size);
>> +		if (type != MTRR_TYPE_INVALID)
>> +			add_map_entry(start, start + size, type);
>> +	}
>> +}
>> +
>> +/* Rebuild map by replacing variable entries. */
>> +static void rebuild_map(void)
>> +{
>> +	cache_map_n = cache_map_fixed;
>> +
>> +	map_add_var();
>> +}
>> +
>> +/* Build the cache_map containing the cache modes per memory range. */
>> +void mtrr_build_map(void)
>> +{
>> +	unsigned int i;
>> +	u64 start, end, size;
>> +	u8 type;
>> +
>> +	if (!mtrr_state.enabled)
>> +		return;
>> +
>> +	/* Add fixed MTRRs, optimize for adjacent entries with same type. */
>> +	if (mtrr_state.enabled & MTRR_STATE_MTRR_FIXED_ENABLED) {
>> +		start = 0;
>> +		end = size = 0x10000;
>> +		type = mtrr_state.fixed_ranges[0];
>> +
>> +		for (i = 1; i < MTRR_NUM_FIXED_RANGES; i++) {
>> +			if (i == 8 || i == 24)
>> +				size >>= 2;
>> +
>> +			if (mtrr_state.fixed_ranges[i] != type) {
>> +				add_map_entry(start, end, type);
>> +				start = end;
>> +				type = mtrr_state.fixed_ranges[i];
>> +			}
>> +			end += size;
>> +		}
>> +		add_map_entry(start, end, type);
>> +	}
>> +
>> +	/* Mark fixed and magic MTRR as fixed, they take precedence. */
>> +	for (i = 0; i < cache_map_n; i++)
>> +		cache_map[i].fixed = true;
>> +	cache_map_fixed = cache_map_n;
>> +
>> +	map_add_var();
> 
> I think it would be useful to issue some stats here:
> 
> 	pr_info("MTRR map: ... size: ..., ... entries: ..., memory used: ....");
> 
> and so on so that we can get some feedback when that happens. We can always
> drop it later but for the initial runs it would be prudent to have that.

Fine with me.

> 
>> +/* Copy the cache_map from __initdata memory to dynamically allocated one. */
>> +void __init mtrr_copy_map(void)
>> +{
>> +	unsigned int new_size = cache_map_fixed + 2 * num_var_ranges;
>> +
>> +	if (!mtrr_state.enabled || !new_size) {
>> +		cache_map = NULL;
>> +		return;
>> +	}
>> +
>> +	mutex_lock(&mtrr_mutex);
>> +
>> +	cache_map = kcalloc(new_size, sizeof(*cache_map), GFP_KERNEL);
>> +	memmove(cache_map, init_cache_map, cache_map_n * sizeof(*cache_map));
>> +	cache_map_size = new_size;
>> +
>> +	mutex_unlock(&mtrr_mutex);
>> +}
>> +
>>   /**
>>    * mtrr_overwrite_state - set static MTRR state
>>    *
>> @@ -815,6 +1065,10 @@ static void generic_set_mtrr(unsigned int reg, unsigned long base,
>>   
>>   	cache_enable();
>>   	local_irq_restore(flags);
>> +
>> +	/* On the first cpu rebuild the cache mode memory map. */
> 
> s/cpu/CPU/
> 
>> +	if (smp_processor_id() == cpumask_first(cpu_online_mask))
> 
> Why not in mtrr_bp_init()? That is the first CPU.

Yeah, but generic_set_mtrr() can be called after boot, too.

> 
>> +		rebuild_map();
>>   }
>>   
>>   int generic_validate_add_page(unsigned long base, unsigned long size,
>> diff --git a/arch/x86/kernel/cpu/mtrr/mtrr.c b/arch/x86/kernel/cpu/mtrr/mtrr.c
>> index 50cd2287b6e1..1dbb9fdfd87b 100644
>> --- a/arch/x86/kernel/cpu/mtrr/mtrr.c
>> +++ b/arch/x86/kernel/cpu/mtrr/mtrr.c
>> @@ -65,7 +65,7 @@ static bool mtrr_enabled(void)
>>   }
>>   
>>   unsigned int mtrr_usage_table[MTRR_MAX_VAR_RANGES];
>> -static DEFINE_MUTEX(mtrr_mutex);
>> +DEFINE_MUTEX(mtrr_mutex);
>>   
>>   u64 size_or_mask, size_and_mask;
>>   
>> @@ -668,6 +668,7 @@ void __init mtrr_bp_init(void)
>>   		/* Software overwrite of MTRR state, only for generic case. */
>>   		mtrr_calc_physbits(true);
>>   		init_table();
>> +		mtrr_build_map();
>>   		pr_info("MTRRs set to read-only\n");
>>   
>>   		return;
>> @@ -705,6 +706,7 @@ void __init mtrr_bp_init(void)
>>   			if (get_mtrr_state()) {
>>   				memory_caching_control |= CACHE_MTRR;
>>   				changed_by_mtrr_cleanup = mtrr_cleanup(phys_addr);
>> +				mtrr_build_map();
>>   			} else {
>>   				mtrr_if = NULL;
>>   				why = "by BIOS";
>> @@ -733,6 +735,8 @@ void mtrr_save_state(void)
>>   
>>   static int __init mtrr_init_finialize(void)
>>   {
>> +	mtrr_copy_map();
> 
> Move that...
> 
>> +
>>   	if (!mtrr_enabled())
>>   		return 0;
> 
> ... here.

Umm, not really. I want to do the copy even in the Xen PV case.

> 
> So yeah, I like the general direction but this is a complex patch. Lemme
> add some printks to it in order to get a better idea of what happens.

Yeah, those were part of my iterative process mentioned above.


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

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

[Borislav Petkov]

On Wed, Mar 29, 2023 at 03:39:35PM +0200, Juergen Gross wrote:
> No. :-)

Because?

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

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.

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

> 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?!

;-\

-- 
Regards/Gruss,
    Boris.

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

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

[Juergen Gross]

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

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

[Borislav Petkov]

On Fri, Mar 31, 2023 at 03:23:13PM +0200, Juergen Gross wrote:
> 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.

Yeah, in the meantime I did add some debug printks in order to find my
way around that code...

> How should it be named? AMD TOP_MEM2 MSR?

It is already called that way - see "git grep TOP_MEM2" output.

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

Pls put that blurb over the function: needs to be called when MTRRs get
modified so that the map is kept valid, yadda yadda...

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

Ack.

> Okay, I'll add some more comments.

Thx.

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

I'm not kidding - it is not super easy.

:-)

-- 
Regards/Gruss,
    Boris.

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

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

[Juergen Gross]

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

On 01.04.23 16:24, Borislav Petkov wrote:
> On Fri, Mar 31, 2023 at 03:23:13PM +0200, Juergen Gross wrote:
>> 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.
> 
> Yeah, in the meantime I did add some debug printks in order to find my
> way around that code...
> 
>> How should it be named? AMD TOP_MEM2 MSR?
> 
> It is already called that way - see "git grep TOP_MEM2" output.
> 
>> 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.
> 
> Pls put that blurb over the function: needs to be called when MTRRs get
> modified so that the map is kept valid, yadda yadda...

Okay.


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

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

[Borislav Petkov]

On Mon, Mar 06, 2023 at 05:34:22PM +0100, Juergen Gross wrote:
> +/* Build the cache_map containing the cache modes per memory range. */
> +void mtrr_build_map(void)
> +{
> +	unsigned int i;
> +	u64 start, end, size;
> +	u8 type;
> +
> +	if (!mtrr_state.enabled)
	     ^^^^^^^^^^^^^^^^^^^

> +		return;
> +
> +	/* Add fixed MTRRs, optimize for adjacent entries with same type. */
> +	if (mtrr_state.enabled & MTRR_STATE_MTRR_FIXED_ENABLED) {
	    ^^^^^^^^^^^^^^^^^^

First check can go.

> +		start = 0;
> +		end = size = 0x10000;
> +		type = mtrr_state.fixed_ranges[0];
> +
> +		for (i = 1; i < MTRR_NUM_FIXED_RANGES; i++) {

Loops starts at 1. Comment explaining why pls.

> +			if (i == 8 || i == 24)

Magic numbers.

Ok, I'll stop here.

Please send a new version with the review comments incorporated. I need
to look more at that map thing more.

Thx.

-- 
Regards/Gruss,
    Boris.

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

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

[Juergen Gross]

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

On 31.03.23 14:57, Borislav Petkov wrote:
> On Mon, Mar 06, 2023 at 05:34:22PM +0100, Juergen Gross wrote:
>> +/* Build the cache_map containing the cache modes per memory range. */
>> +void mtrr_build_map(void)
>> +{
>> +	unsigned int i;
>> +	u64 start, end, size;
>> +	u8 type;
>> +
>> +	if (!mtrr_state.enabled)
> 	     ^^^^^^^^^^^^^^^^^^^
> 
>> +		return;
>> +
>> +	/* Add fixed MTRRs, optimize for adjacent entries with same type. */
>> +	if (mtrr_state.enabled & MTRR_STATE_MTRR_FIXED_ENABLED) {
> 	    ^^^^^^^^^^^^^^^^^^
> 
> First check can go.

Hmm, this makes it much harder to see that the code really does nothing
if enabled isn't set.

> 
>> +		start = 0;
>> +		end = size = 0x10000;
>> +		type = mtrr_state.fixed_ranges[0];
>> +
>> +		for (i = 1; i < MTRR_NUM_FIXED_RANGES; i++) {
> 
> Loops starts at 1. Comment explaining why pls.

Okay.

> 
>> +			if (i == 8 || i == 24)
> 
> Magic numbers.

I'll add more comments.

> 
> Ok, I'll stop here.
> 
> Please send a new version with the review comments incorporated. I need
> to look more at that map thing more.

Will do.


Thanks,

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

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

[Borislav Petkov]

On Fri, Mar 31, 2023 at 03:35:07PM +0200, Juergen Gross wrote:
> Hmm, this makes it much harder to see that the code really does nothing
> if enabled isn't set.

Both callsites in mtrr_bp_init() are behind such a check already.

Thx.

-- 
Regards/Gruss,
    Boris.

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

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

[Juergen Gross]

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

On 01.04.23 16:26, Borislav Petkov wrote:
> On Fri, Mar 31, 2023 at 03:35:07PM +0200, Juergen Gross wrote:
>> Hmm, this makes it much harder to see that the code really does nothing
>> if enabled isn't set.
> 
> Both callsites in mtrr_bp_init() are behind such a check already.

Oh, indeed they are.

I'll remove the check.


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

[PATCH v4 10/12] x86/mtrr: use new cache_map in mtrr_type_lookup()

[Juergen Gross]

Instead of crawling through the MTRR register state, use the new
cache_map for looking up the cache type(s) of a memory region.

This allows now to set the uniform parameter according to the
uniformity of the cache mode of the region, instead of setting it
only if the complete region is mapped by a single MTRR. This now
includes even the region covered by the fixed MTRR registers.

Make sure uniform is always set.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
V3:
- new patch
V3.1:
- fix type_merge() (Michael Kelley)
V4:
- fix type_merge() again (Michael Kelley)
---
 arch/x86/kernel/cpu/mtrr/generic.c | 227 ++++-------------------------
 1 file changed, 32 insertions(+), 195 deletions(-)

diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
index 13bc637a50e9..de6d74d6e3a0 100644
--- a/arch/x86/kernel/cpu/mtrr/generic.c
+++ b/arch/x86/kernel/cpu/mtrr/generic.c
@@ -138,154 +138,6 @@ static u8 get_effective_type(u8 type1, u8 type2)
 	return type1;
 }
 
-/*
- * Check and return the effective type for MTRR-MTRR type overlap.
- * Returns true if the effective type is UNCACHEABLE, else returns false
- */
-static bool check_type_overlap(u8 *prev, u8 *curr)
-{
-	*prev = *curr = get_effective_type(*curr, *prev);
-
-	return *prev == MTRR_TYPE_UNCACHABLE;
-}
-
-/**
- * mtrr_type_lookup_fixed - look up memory type in MTRR fixed entries
- *
- * Return the MTRR fixed memory type of 'start'.
- *
- * MTRR fixed entries are divided into the following ways:
- *  0x00000 - 0x7FFFF : This range is divided into eight 64KB sub-ranges
- *  0x80000 - 0xBFFFF : This range is divided into sixteen 16KB sub-ranges
- *  0xC0000 - 0xFFFFF : This range is divided into sixty-four 4KB sub-ranges
- *
- * Return Values:
- * MTRR_TYPE_(type)  - Matched memory type
- * MTRR_TYPE_INVALID - Unmatched
- */
-static u8 mtrr_type_lookup_fixed(u64 start, u64 end)
-{
-	int idx;
-
-	if (start >= 0x100000)
-		return MTRR_TYPE_INVALID;
-
-	/* 0x0 - 0x7FFFF */
-	if (start < 0x80000) {
-		idx = 0;
-		idx += (start >> 16);
-		return mtrr_state.fixed_ranges[idx];
-	/* 0x80000 - 0xBFFFF */
-	} else if (start < 0xC0000) {
-		idx = 1 * 8;
-		idx += ((start - 0x80000) >> 14);
-		return mtrr_state.fixed_ranges[idx];
-	}
-
-	/* 0xC0000 - 0xFFFFF */
-	idx = 3 * 8;
-	idx += ((start - 0xC0000) >> 12);
-	return mtrr_state.fixed_ranges[idx];
-}
-
-/**
- * mtrr_type_lookup_variable - look up memory type in MTRR variable entries
- *
- * Return Value:
- * MTRR_TYPE_(type) - Matched memory type or default memory type (unmatched)
- *
- * Output Arguments:
- * repeat - Set to 1 when [start:end] spanned across MTRR range and type
- *	    returned corresponds only to [start:*partial_end].  Caller has
- *	    to lookup again for [*partial_end:end].
- *
- * uniform - Set to 1 when an MTRR covers the region uniformly, i.e. the
- *	     region is fully covered by a single MTRR entry or the default
- *	     type.
- */
-static u8 mtrr_type_lookup_variable(u64 start, u64 end, u64 *partial_end,
-				    int *repeat, u8 *uniform)
-{
-	int i;
-	u64 base, mask;
-	u8 prev_match, curr_match;
-
-	*repeat = 0;
-	*uniform = 1;
-
-	prev_match = MTRR_TYPE_INVALID;
-	for (i = 0; i < num_var_ranges; ++i) {
-		unsigned short start_state, end_state, inclusive;
-
-		if (!(mtrr_state.var_ranges[i].mask_lo & (1 << 11)))
-			continue;
-
-		base = (((u64)mtrr_state.var_ranges[i].base_hi) << 32) +
-		       (mtrr_state.var_ranges[i].base_lo & PAGE_MASK);
-		mask = (((u64)mtrr_state.var_ranges[i].mask_hi) << 32) +
-		       (mtrr_state.var_ranges[i].mask_lo & PAGE_MASK);
-
-		start_state = ((start & mask) == (base & mask));
-		end_state = ((end & mask) == (base & mask));
-		inclusive = ((start < base) && (end > base));
-
-		if ((start_state != end_state) || inclusive) {
-			/*
-			 * We have start:end spanning across an MTRR.
-			 * We split the region into either
-			 *
-			 * - start_state:1
-			 * (start:mtrr_end)(mtrr_end:end)
-			 * - end_state:1
-			 * (start:mtrr_start)(mtrr_start:end)
-			 * - inclusive:1
-			 * (start:mtrr_start)(mtrr_start:mtrr_end)(mtrr_end:end)
-			 *
-			 * depending on kind of overlap.
-			 *
-			 * Return the type of the first region and a pointer
-			 * to the start of next region so that caller will be
-			 * advised to lookup again after having adjusted start
-			 * and end.
-			 *
-			 * Note: This way we handle overlaps with multiple
-			 * entries and the default type properly.
-			 */
-			if (start_state)
-				*partial_end = base + get_mtrr_size(mask);
-			else
-				*partial_end = base;
-
-			if (unlikely(*partial_end <= start)) {
-				WARN_ON(1);
-				*partial_end = start + PAGE_SIZE;
-			}
-
-			end = *partial_end - 1; /* end is inclusive */
-			*repeat = 1;
-			*uniform = 0;
-		}
-
-		if ((start & mask) != (base & mask))
-			continue;
-
-		curr_match = mtrr_state.var_ranges[i].base_lo & 0xff;
-		if (prev_match == MTRR_TYPE_INVALID) {
-			prev_match = curr_match;
-			continue;
-		}
-
-		*uniform = 0;
-		if (check_type_overlap(&prev_match, &curr_match))
-			return curr_match;
-	}
-
-	if (prev_match != MTRR_TYPE_INVALID)
-		return prev_match;
-
-	return mtrr_state.def_type;
-}
-
 static void rm_map_entry_at(int idx)
 {
 	int i;
@@ -533,6 +385,20 @@ void mtrr_overwrite_state(struct mtrr_var_range *var, unsigned int num_var,
 	mtrr_state_set = 1;
 }
 
+static u8 type_merge(u8 type, u8 new_type, u8 *uniform)
+{
+	u8 effective_type;
+
+	if (type == MTRR_TYPE_INVALID)
+		return new_type;
+
+	effective_type = get_effective_type(type, new_type);
+	if (type != effective_type)
+		*uniform = 0;
+
+	return effective_type;
+}
+
 /**
  * mtrr_type_lookup - look up memory type in MTRR
  *
@@ -541,66 +407,37 @@ void mtrr_overwrite_state(struct mtrr_var_range *var, unsigned int num_var,
  * MTRR_TYPE_INVALID - MTRR is disabled
  *
  * Output Argument:
- * uniform - Set to 1 when an MTRR covers the region uniformly, i.e. the
- *	     region is fully covered by a single MTRR entry or the default
- *	     type.
+ * uniform - Set to 1 when the returned MTRR type is valid for the whole
+ *	     region, set to 0 else.
  */
 u8 mtrr_type_lookup(u64 start, u64 end, u8 *uniform)
 {
-	u8 type, prev_type, is_uniform = 1, dummy;
-	int repeat;
-	u64 partial_end;
-
-	/* Make end inclusive instead of exclusive */
-	end--;
+	u8 type = MTRR_TYPE_INVALID;
+	unsigned int i;
 
-	if (!mtrr_state_set)
+	if (!mtrr_state_set) {
+		*uniform = 0;	/* Uniformity is unknown. */
 		return MTRR_TYPE_INVALID;
+	}
+
+	*uniform = 1;
 
 	if (!(mtrr_state.enabled & MTRR_STATE_MTRR_ENABLED))
 		return MTRR_TYPE_INVALID;
 
-	/*
-	 * Look up the fixed ranges first, which take priority over
-	 * the variable ranges.
-	 */
-	if ((start < 0x100000) &&
-	    (mtrr_state.have_fixed) &&
-	    (mtrr_state.enabled & MTRR_STATE_MTRR_FIXED_ENABLED)) {
-		is_uniform = 0;
-		type = mtrr_type_lookup_fixed(start, end);
-		goto out;
-	}
-
-	/*
-	 * Look up the variable ranges.  Look of multiple ranges matching
-	 * this address and pick type as per MTRR precedence.
-	 */
-	type = mtrr_type_lookup_variable(start, end, &partial_end,
-					 &repeat, &is_uniform);
+	for (i = 0; i < cache_map_n && start < end; i++) {
+		if (start >= cache_map[i].end)
+			continue;
+		if (start < cache_map[i].start)
+			type = type_merge(type, mtrr_state.def_type, uniform);
+		type = type_merge(type, cache_map[i].type, uniform);
 
-	/*
-	 * Common path is with repeat = 0.
-	 * However, we can have cases where [start:end] spans across some
-	 * MTRR ranges and/or the default type.  Do repeated lookups for
-	 * that case here.
-	 */
-	while (repeat) {
-		prev_type = type;
-		start = partial_end;
-		is_uniform = 0;
-		type = mtrr_type_lookup_variable(start, end, &partial_end,
-						 &repeat, &dummy);
-
-		if (check_type_overlap(&prev_type, &type))
-			goto out;
+		start = cache_map[i].end;
 	}
 
-	if (mtrr_tom2 && (start >= (1ULL<<32)) && (end < mtrr_tom2))
-		type = MTRR_TYPE_WRBACK;
+	if (start < end)
+		type = type_merge(type, mtrr_state.def_type, uniform);
 
-out:
-	*uniform = is_uniform;
 	return type;
 }
 
-- 
2.35.3

[PATCH v4 11/12] x86/mtrr: don't let mtrr_type_lookup() return MTRR_TYPE_INVALID

[Juergen Gross]

mtrr_type_lookup() should always return a valid memory type. In case
there is no information available, it should return the default UC.

This will remove the last case where mtrr_type_lookup() can return
MTRR_TYPE_INVALID, so adjust the comment in include/uapi/asm/mtrr.h.

Note that removing the MTRR_TYPE_INVALID #define from that header
could break user code, so it has to stay.

At the same time the mtrr_type_lookup() stub for the !CONFIG_MTRR
case should set uniform to 1, as if the memory range would be
covered by no MTRR at all.

Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V2:
- always set uniform
- set uniform to 1 in case of disabled MTRRs (Linus Torvalds)
V3:
- adjust include/uapi/asm/mtrr.h comment
---
 arch/x86/include/asm/mtrr.h        | 7 +++++--
 arch/x86/include/uapi/asm/mtrr.h   | 6 +++---
 arch/x86/kernel/cpu/mtrr/generic.c | 4 ++--
 3 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/arch/x86/include/asm/mtrr.h b/arch/x86/include/asm/mtrr.h
index f1cb81330a64..0d7ea8a54d81 100644
--- a/arch/x86/include/asm/mtrr.h
+++ b/arch/x86/include/asm/mtrr.h
@@ -59,9 +59,12 @@ static inline void mtrr_overwrite_state(struct mtrr_var_range *var,
 static inline u8 mtrr_type_lookup(u64 addr, u64 end, u8 *uniform)
 {
 	/*
-	 * Return no-MTRRs:
+	 * Return the default MTRR type, without any known other types in
+	 * that range.
 	 */
-	return MTRR_TYPE_INVALID;
+	*uniform = 1;
+
+	return MTRR_TYPE_UNCACHABLE;
 }
 #define mtrr_save_fixed_ranges(arg) do {} while (0)
 #define mtrr_save_state() do {} while (0)
diff --git a/arch/x86/include/uapi/asm/mtrr.h b/arch/x86/include/uapi/asm/mtrr.h
index 376563f2bac1..4aa05c2ffa78 100644
--- a/arch/x86/include/uapi/asm/mtrr.h
+++ b/arch/x86/include/uapi/asm/mtrr.h
@@ -115,9 +115,9 @@ struct mtrr_state_type {
 #define MTRR_NUM_TYPES       7
 
 /*
- * Invalid MTRR memory type.  mtrr_type_lookup() returns this value when
- * MTRRs are disabled.  Note, this value is allocated from the reserved
- * values (0x7-0xff) of the MTRR memory types.
+ * Invalid MTRR memory type.  No longer used outside of MTRR code.
+ * Note, this value is allocated from the reserved values (0x7-0xff) of
+ * the MTRR memory types.
  */
 #define MTRR_TYPE_INVALID    0xff
 
diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
index de6d74d6e3a0..3d48e9d06bfb 100644
--- a/arch/x86/kernel/cpu/mtrr/generic.c
+++ b/arch/x86/kernel/cpu/mtrr/generic.c
@@ -417,13 +417,13 @@ u8 mtrr_type_lookup(u64 start, u64 end, u8 *uniform)
 
 	if (!mtrr_state_set) {
 		*uniform = 0;	/* Uniformity is unknown. */
-		return MTRR_TYPE_INVALID;
+		return MTRR_TYPE_UNCACHABLE;
 	}
 
 	*uniform = 1;
 
 	if (!(mtrr_state.enabled & MTRR_STATE_MTRR_ENABLED))
-		return MTRR_TYPE_INVALID;
+		return MTRR_TYPE_UNCACHABLE;
 
 	for (i = 0; i < cache_map_n && start < end; i++) {
 		if (start >= cache_map[i].end)
-- 
2.35.3

[PATCH v4 12/12] x86/mm: only check uniform after calling mtrr_type_lookup()

[Juergen Gross]

Today pud_set_huge() and pmd_set_huge() test for the MTRR type to be
WB or INVALID after calling mtrr_type_lookup(). Those tests can be
dropped, as the only reason to not use a large mapping would be
uniform being 0. Any MTRR type can be accepted as long as it applies
to the whole memory range covered by the mapping, as the alternative
would only be to map the same region with smaller pages instead, using
the same PAT type as for the large mapping.

Suggested-by: Linus Torvalds <torvalds@linux-foundation.org>
Signed-off-by: Juergen Gross <jgross@suse.com>
---
V3:
- adapt comment for pud_set_huge()
---
 arch/x86/mm/pgtable.c | 24 ++++++++----------------
 1 file changed, 8 insertions(+), 16 deletions(-)

diff --git a/arch/x86/mm/pgtable.c b/arch/x86/mm/pgtable.c
index e4f499eb0f29..15a8009a4480 100644
--- a/arch/x86/mm/pgtable.c
+++ b/arch/x86/mm/pgtable.c
@@ -702,14 +702,8 @@ void p4d_clear_huge(p4d_t *p4d)
  * pud_set_huge - setup kernel PUD mapping
  *
  * MTRRs can override PAT memory types with 4KiB granularity. Therefore, this
- * function sets up a huge page only if any of the following conditions are met:
- *
- * - MTRRs are disabled, or
- *
- * - MTRRs are enabled and the range is completely covered by a single MTRR, or
- *
- * - MTRRs are enabled and the corresponding MTRR memory type is WB, which
- *   has no effect on the requested PAT memory type.
+ * function sets up a huge page only if the complete range has the same MTRR
+ * caching mode.
  *
  * Callers should try to decrease page size (1GB -> 2MB -> 4K) if the bigger
  * page mapping attempt fails.
@@ -718,11 +712,10 @@ void p4d_clear_huge(p4d_t *p4d)
  */
 int pud_set_huge(pud_t *pud, phys_addr_t addr, pgprot_t prot)
 {
-	u8 mtrr, uniform;
+	u8 uniform;
 
-	mtrr = mtrr_type_lookup(addr, addr + PUD_SIZE, &uniform);
-	if ((mtrr != MTRR_TYPE_INVALID) && (!uniform) &&
-	    (mtrr != MTRR_TYPE_WRBACK))
+	mtrr_type_lookup(addr, addr + PUD_SIZE, &uniform);
+	if (!uniform)
 		return 0;
 
 	/* Bail out if we are we on a populated non-leaf entry: */
@@ -745,11 +738,10 @@ int pud_set_huge(pud_t *pud, phys_addr_t addr, pgprot_t prot)
  */
 int pmd_set_huge(pmd_t *pmd, phys_addr_t addr, pgprot_t prot)
 {
-	u8 mtrr, uniform;
+	u8 uniform;
 
-	mtrr = mtrr_type_lookup(addr, addr + PMD_SIZE, &uniform);
-	if ((mtrr != MTRR_TYPE_INVALID) && (!uniform) &&
-	    (mtrr != MTRR_TYPE_WRBACK)) {
+	mtrr_type_lookup(addr, addr + PMD_SIZE, &uniform);
+	if (!uniform) {
 		pr_warn_once("%s: Cannot satisfy [mem %#010llx-%#010llx] with a huge-page mapping due to MTRR override.\n",
 			     __func__, addr, addr + PMD_SIZE);
 		return 0;
-- 
2.35.3

RE: [PATCH v4 00/12] x86/mtrr: fix handling with PAT but without MTRR

[Michael Kelley (LINUX)]

From: Juergen Gross <jgross@suse.com> Sent: Monday, March 6, 2023 8:34 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
> 
> Juergen Gross (12):
>   x86/mtrr: split off physical address size calculation
>   x86/mtrr: optimize mtrr_calc_physbits()
>   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: allocate mtrr_value array dynamically
>   x86/mtrr: add get_effective_type() service function
>   x86/mtrr: construct a memory map with cache modes
>   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()
> 
>  arch/x86/include/asm/mtrr.h        |  15 +-
>  arch/x86/include/uapi/asm/mtrr.h   |   6 +-
>  arch/x86/kernel/cpu/mshyperv.c     |   4 +
>  arch/x86/kernel/cpu/mtrr/amd.c     |   2 +-
>  arch/x86/kernel/cpu/mtrr/centaur.c |   2 +-
>  arch/x86/kernel/cpu/mtrr/cleanup.c |   4 +-
>  arch/x86/kernel/cpu/mtrr/cyrix.c   |   2 +-
>  arch/x86/kernel/cpu/mtrr/generic.c | 492 ++++++++++++++++++-----------
>  arch/x86/kernel/cpu/mtrr/mtrr.c    |  94 +++---
>  arch/x86/kernel/cpu/mtrr/mtrr.h    |   7 +-
>  arch/x86/kernel/setup.c            |   2 +
>  arch/x86/mm/pgtable.c              |  24 +-
>  arch/x86/xen/enlighten_pv.c        |  52 +++
>  13 files changed, 454 insertions(+), 252 deletions(-)
> 
> --
> 2.35.3

I've tested a Linux 6.2 kernel plus this series in a normal Hyper-V
guest and in a Hyper-V guest using SEV-SNP with vTOM.  MMIO
memory is correctly mapped as WB or UC- depending on the
request, which fixes the original problem introduced for Hyper-V
by the Xen-specific change.

Tested-by: Michael Kelley <mikelley@microsoft.com>