[PATCH v4 0/7] mtrr, mm, x86: Enhance MTRR checks for huge I/O mapping

[PATCH v4 0/7] mtrr, mm, x86: Enhance MTRR checks for huge I/O mapping

[Toshi Kani]

This patchset enhances MTRR checks for the kernel huge I/O mapping,
which was enabled by the patchset below:
  https://lkml.org/lkml/2015/3/3/589

The following functional changes are made in patch 7/7.
 - Allow pud_set_huge() and pmd_set_huge() to create a huge page
   mapping to a range covered by a single MTRR entry of any memory
   type.
 - Log a pr_warn() message when a specified PMD map range spans more
   than a single MTRR entry.  Drivers should make a mapping request
   aligned to a single MTRR entry when the range is covered by MTRRs.

Patch 1/7 addresses other review comments to the mapping funcs for
better code read-ability.  Patch 2/7 - 6/7 are bug fixes and clean up
to mtrr_type_lookup().

The patchset is based on the -mm tree.
---
v4:
 - Update the change logs of patchset. (Ingo Molnar)
 - Add patch 3/7 to make the wrong address fix as a separate patch.
   (Ingo Molnar)
 - Add patch 5/7 to define MTRR_TYPE_INVALID. (Ingo Molnar)
 - Update patch 6/7 to document MTRR fixed ranges. (Ingo Molnar)

v3:
 - Add patch 3/5 to fix a bug in MTRR state checks.
 - Update patch 4/5 to create separate functions for the fixed and
   variable entries. (Ingo Molnar)

v2:
 - Update change logs and comments per review comments.
   (Ingo Molnar)
 - Add patch 3/4 to clean up mtrr_type_lookup(). (Ingo Molnar)

---
Toshi Kani (7):
 1/7 mm, x86: Document return values of mapping funcs
 2/7 mtrr, x86: Fix MTRR lookup to handle inclusive entry
 3/7 mtrr, x86: Remove a wrong address check in __mtrr_type_lookup()
 4/7 mtrr, x86: Fix MTRR state checks in mtrr_type_lookup()
 5/7 mtrr, x86: Define MTRR_TYPE_INVALID for mtrr_type_lookup()
 6/7 mtrr, x86: Clean up mtrr_type_lookup()
 7/7 mtrr, mm, x86: Enhance MTRR checks for KVA huge page mapping

---
 arch/x86/Kconfig                   |   2 +-
 arch/x86/include/asm/mtrr.h        |   7 +-
 arch/x86/include/uapi/asm/mtrr.h   |  12 ++-
 arch/x86/kernel/cpu/mtrr/generic.c | 192 ++++++++++++++++++++++++-------------
 arch/x86/mm/pat.c                  |   4 +-
 arch/x86/mm/pgtable.c              |  53 +++++++---
 6 files changed, 181 insertions(+), 89 deletions(-)

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH v4 1/7] mm, x86: Document return values of mapping funcs

[Toshi Kani]

Document the return values of KVA mapping functions,
pud_set_huge(), pmd_set_huge, pud_clear_huge() and
pmd_clear_huge().

Simplify the conditions to select HAVE_ARCH_HUGE_VMAP
in the Kconfig, since X86_PAE depends on X86_32.

There is no functional change in this patch.

Signed-off-by: Toshi Kani <toshi.kani@hp.com>
---
 arch/x86/Kconfig      |    2 +-
 arch/x86/mm/pgtable.c |   36 ++++++++++++++++++++++++++++--------
 2 files changed, 29 insertions(+), 9 deletions(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index cb23206..2ea27da 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -99,7 +99,7 @@ config X86
 	select IRQ_FORCED_THREADING
 	select HAVE_BPF_JIT if X86_64
 	select HAVE_ARCH_TRANSPARENT_HUGEPAGE
-	select HAVE_ARCH_HUGE_VMAP if X86_64 || (X86_32 && X86_PAE)
+	select HAVE_ARCH_HUGE_VMAP if X86_64 || X86_PAE
 	select ARCH_HAS_SG_CHAIN
 	select CLKEVT_I8253
 	select ARCH_HAVE_NMI_SAFE_CMPXCHG
diff --git a/arch/x86/mm/pgtable.c b/arch/x86/mm/pgtable.c
index 0b97d2c..4891fa1 100644
--- a/arch/x86/mm/pgtable.c
+++ b/arch/x86/mm/pgtable.c
@@ -563,14 +563,19 @@ void native_set_fixmap(enum fixed_addresses idx, phys_addr_t phys,
 }
 
 #ifdef CONFIG_HAVE_ARCH_HUGE_VMAP
+/**
+ * pud_set_huge - setup kernel PUD mapping
+ *
+ * MTRR can override PAT memory types with 4KB granularity.  Therefore,
+ * it does not set up a huge page when the range is covered by a non-WB
+ * type of MTRR.  0xFF indicates that MTRR are disabled.
+ *
+ * Return 1 on success, and 0 when no PUD was set.
+ */
 int pud_set_huge(pud_t *pud, phys_addr_t addr, pgprot_t prot)
 {
 	u8 mtrr;
 
-	/*
-	 * Do not use a huge page when the range is covered by non-WB type
-	 * of MTRRs.
-	 */
 	mtrr = mtrr_type_lookup(addr, addr + PUD_SIZE);
 	if ((mtrr != MTRR_TYPE_WRBACK) && (mtrr != 0xFF))
 		return 0;
@@ -584,14 +589,19 @@ int pud_set_huge(pud_t *pud, phys_addr_t addr, pgprot_t prot)
 	return 1;
 }
 
+/**
+ * pmd_set_huge - setup kernel PMD mapping
+ *
+ * MTRR can override PAT memory types with 4KB granularity.  Therefore,
+ * it does not set up a huge page when the range is covered by a non-WB
+ * type of MTRR.  0xFF indicates that MTRR are disabled.
+ *
+ * Return 1 on success, and 0 when no PMD was set.
+ */
 int pmd_set_huge(pmd_t *pmd, phys_addr_t addr, pgprot_t prot)
 {
 	u8 mtrr;
 
-	/*
-	 * Do not use a huge page when the range is covered by non-WB type
-	 * of MTRRs.
-	 */
 	mtrr = mtrr_type_lookup(addr, addr + PMD_SIZE);
 	if ((mtrr != MTRR_TYPE_WRBACK) && (mtrr != 0xFF))
 		return 0;
@@ -605,6 +615,11 @@ int pmd_set_huge(pmd_t *pmd, phys_addr_t addr, pgprot_t prot)
 	return 1;
 }
 
+/**
+ * pud_clear_huge - clear kernel PUD mapping when it is set
+ *
+ * Return 1 on success, and 0 when no PUD map was found.
+ */
 int pud_clear_huge(pud_t *pud)
 {
 	if (pud_large(*pud)) {
@@ -615,6 +630,11 @@ int pud_clear_huge(pud_t *pud)
 	return 0;
 }
 
+/**
+ * pmd_clear_huge - clear kernel PMD mapping when it is set
+ *
+ * Return 1 on success, and 0 when no PMD map was found.
+ */
 int pmd_clear_huge(pmd_t *pmd)
 {
 	if (pmd_large(*pmd)) {

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH v4 2/7] mtrr, x86: Fix MTRR lookup to handle inclusive entry

[Toshi Kani]

When an MTRR entry is inclusive to a requested range, i.e.
the start and end of the request are not within the MTRR
entry range but the range contains the MTRR entry entirely,
__mtrr_type_lookup() ignores such a case because both
start_state and end_state are set to zero.

This bug can cause the following issues:
1) reserve_memtype() tracks an effective memory type in case
   a request type is WB (ex. /dev/mem blindly uses WB). Missing
   to track with its effective type causes a subsequent request
   to map the same range with the effective type to fail.
2) pud_set_huge() and pmd_set_huge() check if a requested range
   has any overlap with MTRRs. Missing to detect an overlap may
   cause a performance penalty or undefined behavior.

This patch fixes the bug by adding a new flag, 'inclusive',
to detect the inclusive case.  This case is then handled in
the same way as (!start_state && end_state).  With this fix,
__mtrr_type_lookup() handles the inclusive case properly.

Signed-off-by: Toshi Kani <toshi.kani@hp.com>
---
 arch/x86/kernel/cpu/mtrr/generic.c |   17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
index 7d74f7b..a82e370 100644
--- a/arch/x86/kernel/cpu/mtrr/generic.c
+++ b/arch/x86/kernel/cpu/mtrr/generic.c
@@ -154,7 +154,7 @@ static u8 __mtrr_type_lookup(u64 start, u64 end, u64 *partial_end, int *repeat)
 
 	prev_match = 0xFF;
 	for (i = 0; i < num_var_ranges; ++i) {
-		unsigned short start_state, end_state;
+		unsigned short start_state, end_state, inclusive;
 
 		if (!(mtrr_state.var_ranges[i].mask_lo & (1 << 11)))
 			continue;
@@ -166,15 +166,16 @@ static u8 __mtrr_type_lookup(u64 start, u64 end, u64 *partial_end, int *repeat)
 
 		start_state = ((start & mask) == (base & mask));
 		end_state = ((end & mask) == (base & mask));
+		inclusive = ((start < base) && (end > base));
 
-		if (start_state != end_state) {
+		if ((start_state != end_state) || inclusive) {
 			/*
 			 * We have start:end spanning across an MTRR.
-			 * We split the region into
-			 * either
-			 * (start:mtrr_end) (mtrr_end:end)
-			 * or
-			 * (start:mtrr_start) (mtrr_start:end)
+			 * We split the region into either
+			 * - start_state:1
+			 *     (start:mtrr_end) (mtrr_end:end)
+			 * - end_state:1 or inclusive:1
+			 *     (start:mtrr_start) (mtrr_start:end)
 			 * depending on kind of overlap.
 			 * Return the type for first region and a pointer to
 			 * the start of second region so that caller will
@@ -195,7 +196,7 @@ static u8 __mtrr_type_lookup(u64 start, u64 end, u64 *partial_end, int *repeat)
 			*repeat = 1;
 		}
 
-		if ((start & mask) != (base & mask))
+		if (!start_state)
 			continue;
 
 		curr_match = mtrr_state.var_ranges[i].base_lo & 0xff;

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH v4 3/7] mtrr, x86: Remove a wrong address check in __mtrr_type_lookup()

[Toshi Kani]

__mtrr_type_lookup() checks MTRR fixed ranges when
mtrr_state.have_fixed is set and start is less than
0x100000.  However, the 'else if (start < 0x1000000)'
in the code checks with a wrong address as it has
an extra-zero in the address.  The code still runs
correctly as this check is meaningless, though.

This patch replaces the wrong address check with 'else'
with no condition.

Signed-off-by: Toshi Kani <toshi.kani@hp.com>
---
 arch/x86/kernel/cpu/mtrr/generic.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
index a82e370..c5be327 100644
--- a/arch/x86/kernel/cpu/mtrr/generic.c
+++ b/arch/x86/kernel/cpu/mtrr/generic.c
@@ -137,7 +137,7 @@ static u8 __mtrr_type_lookup(u64 start, u64 end, u64 *partial_end, int *repeat)
 			idx = 1 * 8;
 			idx += ((start - 0x80000) >> 14);
 			return mtrr_state.fixed_ranges[idx];
-		} else if (start < 0x1000000) {
+		} else {
 			idx = 3 * 8;
 			idx += ((start - 0xC0000) >> 12);
 			return mtrr_state.fixed_ranges[idx];

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH v4 4/7] mtrr, x86: Fix MTRR state checks in mtrr_type_lookup()

[Toshi Kani]

'mtrr_state.enabled' contains the FE (fixed MTRRs enabled)
and E (MTRRs enabled) flags in MSR_MTRRdefType.  Intel SDM,
section 11.11.2.1, defines these flags as follows:
 - All MTRRs are disabled when the E flag is clear.
   The FE flag has no affect when the E flag is clear.
 - The default type is enabled when the E flag is set.
 - MTRR variable ranges are enabled when the E flag is set.
 - MTRR fixed ranges are enabled when both E and FE flags
   are set.

MTRR state checks in __mtrr_type_lookup() do not match with
SDM.  Hence, this patch makes the following changes:
 - The current code detects MTRRs disabled when both E and
   FE flags are clear in mtrr_state.enabled.  Fix to detect
   MTRRs disabled when the E flag is clear.
 - The current code does not check if the FE bit is set in
   mtrr_state.enabled when looking into the fixed entries.
   Fix to check the FE flag.
 - The current code returns the default type when the E flag
   is clear in mtrr_state.enabled.  However, the default type
   is also disabled when the E flag is clear.  Fix to remove
   the code as this case is handled as MTRR disabled with
   the 1st change.

In addition, this patch defines the E and FE flags in
mtrr_state.enabled as follows.
 - FE flag: MTRR_STATE_MTRR_FIXED_ENABLED
 - E  flag: MTRR_STATE_MTRR_ENABLED

print_mtrr_state() is also updated accordingly.

Signed-off-by: Toshi Kani <toshi.kani@hp.com>
---
 arch/x86/include/uapi/asm/mtrr.h   |    4 ++++
 arch/x86/kernel/cpu/mtrr/generic.c |   15 ++++++++-------
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/arch/x86/include/uapi/asm/mtrr.h b/arch/x86/include/uapi/asm/mtrr.h
index d0acb65..66ba88d 100644
--- a/arch/x86/include/uapi/asm/mtrr.h
+++ b/arch/x86/include/uapi/asm/mtrr.h
@@ -88,6 +88,10 @@ struct mtrr_state_type {
 	mtrr_type def_type;
 };
 
+/* Bit fields for enabled in struct mtrr_state_type */
+#define MTRR_STATE_MTRR_FIXED_ENABLED	0x01
+#define MTRR_STATE_MTRR_ENABLED		0x02
+
 #define MTRRphysBase_MSR(reg) (0x200 + 2 * (reg))
 #define MTRRphysMask_MSR(reg) (0x200 + 2 * (reg) + 1)
 
diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
index c5be327..4bff6db 100644
--- a/arch/x86/kernel/cpu/mtrr/generic.c
+++ b/arch/x86/kernel/cpu/mtrr/generic.c
@@ -119,14 +119,16 @@ static u8 __mtrr_type_lookup(u64 start, u64 end, u64 *partial_end, int *repeat)
 	if (!mtrr_state_set)
 		return 0xFF;
 
-	if (!mtrr_state.enabled)
+	if (!(mtrr_state.enabled & MTRR_STATE_MTRR_ENABLED))
 		return 0xFF;
 
 	/* Make end inclusive end, instead of exclusive */
 	end--;
 
 	/* Look in fixed ranges. Just return the type as per start */
-	if (mtrr_state.have_fixed && (start < 0x100000)) {
+	if ((start < 0x100000) &&
+	    (mtrr_state.have_fixed) &&
+	    (mtrr_state.enabled & MTRR_STATE_MTRR_FIXED_ENABLED)) {
 		int idx;
 
 		if (start < 0x80000) {
@@ -149,9 +151,6 @@ static u8 __mtrr_type_lookup(u64 start, u64 end, u64 *partial_end, int *repeat)
 	 * Look of multiple ranges matching this address and pick type
 	 * as per MTRR precedence
 	 */
-	if (!(mtrr_state.enabled & 2))
-		return mtrr_state.def_type;
-
 	prev_match = 0xFF;
 	for (i = 0; i < num_var_ranges; ++i) {
 		unsigned short start_state, end_state, inclusive;
@@ -348,7 +347,9 @@ static void __init print_mtrr_state(void)
 		 mtrr_attrib_to_str(mtrr_state.def_type));
 	if (mtrr_state.have_fixed) {
 		pr_debug("MTRR fixed ranges %sabled:\n",
-			 mtrr_state.enabled & 1 ? "en" : "dis");
+			((mtrr_state.enabled & MTRR_STATE_MTRR_ENABLED) &&
+			 (mtrr_state.enabled & MTRR_STATE_MTRR_FIXED_ENABLED)) ?
+			 "en" : "dis");
 		print_fixed(0x00000, 0x10000, mtrr_state.fixed_ranges + 0);
 		for (i = 0; i < 2; ++i)
 			print_fixed(0x80000 + i * 0x20000, 0x04000,
@@ -361,7 +362,7 @@ static void __init print_mtrr_state(void)
 		print_fixed_last();
 	}
 	pr_debug("MTRR variable ranges %sabled:\n",
-		 mtrr_state.enabled & 2 ? "en" : "dis");
+		 mtrr_state.enabled & MTRR_STATE_MTRR_ENABLED ? "en" : "dis");
 	high_width = (__ffs64(size_or_mask) - (32 - PAGE_SHIFT) + 3) / 4;
 
 	for (i = 0; i < num_var_ranges; ++i) {

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH v4 5/7] mtrr, x86: Define MTRR_TYPE_INVALID for mtrr_type_lookup()

[Toshi Kani]

mtrr_type_lookup() returns 0xFF when it cannot return a valid
MTRR memory type since MTRRs are disabled.  This patch defines
MTRR_TYPE_INVALID to clarify the meaning of this value, and
documents its usage.

There is no functional change in this patch.

Signed-off-by: Toshi Kani <toshi.kani@hp.com>
---
 arch/x86/include/asm/mtrr.h        |    2 +-
 arch/x86/include/uapi/asm/mtrr.h   |    8 +++++++-
 arch/x86/kernel/cpu/mtrr/generic.c |   14 +++++++-------
 arch/x86/mm/pgtable.c              |    8 ++++----
 4 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/arch/x86/include/asm/mtrr.h b/arch/x86/include/asm/mtrr.h
index f768f62..a174af6 100644
--- a/arch/x86/include/asm/mtrr.h
+++ b/arch/x86/include/asm/mtrr.h
@@ -55,7 +55,7 @@ static inline u8 mtrr_type_lookup(u64 addr, u64 end)
 	/*
 	 * Return no-MTRRs:
 	 */
-	return 0xff;
+	return MTRR_TYPE_INVALID;
 }
 #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 66ba88d..0bc86c6 100644
--- a/arch/x86/include/uapi/asm/mtrr.h
+++ b/arch/x86/include/uapi/asm/mtrr.h
@@ -107,7 +107,7 @@ struct mtrr_state_type {
 #define MTRRIOC_GET_PAGE_ENTRY   _IOWR(MTRR_IOCTL_BASE, 8, struct mtrr_gentry)
 #define MTRRIOC_KILL_PAGE_ENTRY  _IOW(MTRR_IOCTL_BASE,  9, struct mtrr_sentry)
 
-/*  These are the region types  */
+/* MTRR memory types, which are defined in SDM */
 #define MTRR_TYPE_UNCACHABLE 0
 #define MTRR_TYPE_WRCOMB     1
 /*#define MTRR_TYPE_         2*/
@@ -117,5 +117,11 @@ struct mtrr_state_type {
 #define MTRR_TYPE_WRBACK     6
 #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.
+ */
+#define MTRR_TYPE_INVALID    0xff
 
 #endif /* _UAPI_ASM_X86_MTRR_H */
diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
index 4bff6db..8bd1298 100644
--- a/arch/x86/kernel/cpu/mtrr/generic.c
+++ b/arch/x86/kernel/cpu/mtrr/generic.c
@@ -104,7 +104,7 @@ static int check_type_overlap(u8 *prev, u8 *curr)
 
 /*
  * Error/Semi-error returns:
- * 0xFF - when MTRR is not enabled
+ * MTRR_TYPE_INVALID - when MTRR is not enabled
  * *repeat == 1 implies [start:end] spanned across MTRR range and type returned
  *		corresponds only to [start:*partial_end].
  *		Caller has to lookup again for [*partial_end:end].
@@ -117,10 +117,10 @@ static u8 __mtrr_type_lookup(u64 start, u64 end, u64 *partial_end, int *repeat)
 
 	*repeat = 0;
 	if (!mtrr_state_set)
-		return 0xFF;
+		return MTRR_TYPE_INVALID;
 
 	if (!(mtrr_state.enabled & MTRR_STATE_MTRR_ENABLED))
-		return 0xFF;
+		return MTRR_TYPE_INVALID;
 
 	/* Make end inclusive end, instead of exclusive */
 	end--;
@@ -151,7 +151,7 @@ static u8 __mtrr_type_lookup(u64 start, u64 end, u64 *partial_end, int *repeat)
 	 * Look of multiple ranges matching this address and pick type
 	 * as per MTRR precedence
 	 */
-	prev_match = 0xFF;
+	prev_match = MTRR_TYPE_INVALID;
 	for (i = 0; i < num_var_ranges; ++i) {
 		unsigned short start_state, end_state, inclusive;
 
@@ -199,7 +199,7 @@ static u8 __mtrr_type_lookup(u64 start, u64 end, u64 *partial_end, int *repeat)
 			continue;
 
 		curr_match = mtrr_state.var_ranges[i].base_lo & 0xff;
-		if (prev_match == 0xFF) {
+		if (prev_match == MTRR_TYPE_INVALID) {
 			prev_match = curr_match;
 			continue;
 		}
@@ -213,7 +213,7 @@ static u8 __mtrr_type_lookup(u64 start, u64 end, u64 *partial_end, int *repeat)
 			return MTRR_TYPE_WRBACK;
 	}
 
-	if (prev_match != 0xFF)
+	if (prev_match != MTRR_TYPE_INVALID)
 		return prev_match;
 
 	return mtrr_state.def_type;
@@ -222,7 +222,7 @@ static u8 __mtrr_type_lookup(u64 start, u64 end, u64 *partial_end, int *repeat)
 /*
  * Returns the effective MTRR type for the region
  * Error return:
- * 0xFF - when MTRR is not enabled
+ * MTRR_TYPE_INVALID - when MTRR is not enabled
  */
 u8 mtrr_type_lookup(u64 start, u64 end)
 {
diff --git a/arch/x86/mm/pgtable.c b/arch/x86/mm/pgtable.c
index 4891fa1..cfca4cf 100644
--- a/arch/x86/mm/pgtable.c
+++ b/arch/x86/mm/pgtable.c
@@ -568,7 +568,7 @@ void native_set_fixmap(enum fixed_addresses idx, phys_addr_t phys,
  *
  * MTRR can override PAT memory types with 4KB granularity.  Therefore,
  * it does not set up a huge page when the range is covered by a non-WB
- * type of MTRR.  0xFF indicates that MTRR are disabled.
+ * type of MTRR.  MTRR_TYPE_INVALID indicates that MTRR are disabled.
  *
  * Return 1 on success, and 0 when no PUD was set.
  */
@@ -577,7 +577,7 @@ int pud_set_huge(pud_t *pud, phys_addr_t addr, pgprot_t prot)
 	u8 mtrr;
 
 	mtrr = mtrr_type_lookup(addr, addr + PUD_SIZE);
-	if ((mtrr != MTRR_TYPE_WRBACK) && (mtrr != 0xFF))
+	if ((mtrr != MTRR_TYPE_WRBACK) && (mtrr != MTRR_TYPE_INVALID))
 		return 0;
 
 	prot = pgprot_4k_2_large(prot);
@@ -594,7 +594,7 @@ int pud_set_huge(pud_t *pud, phys_addr_t addr, pgprot_t prot)
  *
  * MTRR can override PAT memory types with 4KB granularity.  Therefore,
  * it does not set up a huge page when the range is covered by a non-WB
- * type of MTRR.  0xFF indicates that MTRR are disabled.
+ * type of MTRR.  MTRR_TYPE_INVALID indicates that MTRR are disabled.
  *
  * Return 1 on success, and 0 when no PMD was set.
  */
@@ -603,7 +603,7 @@ int pmd_set_huge(pmd_t *pmd, phys_addr_t addr, pgprot_t prot)
 	u8 mtrr;
 
 	mtrr = mtrr_type_lookup(addr, addr + PMD_SIZE);
-	if ((mtrr != MTRR_TYPE_WRBACK) && (mtrr != 0xFF))
+	if ((mtrr != MTRR_TYPE_WRBACK) && (mtrr != MTRR_TYPE_INVALID))
 		return 0;
 
 	prot = pgprot_4k_2_large(prot);

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH v4 6/7] mtrr, x86: Clean up mtrr_type_lookup()

[Toshi Kani]

MTRRs contain fixed and variable entries.  mtrr_type_lookup()
may repeatedly call __mtrr_type_lookup() to handle a request
that overlaps with variable entries.  However,
__mtrr_type_lookup() also handles the fixed entries, which
do not have to be repeated.  Therefore, this patch creates
separate functions, mtrr_type_lookup_fixed() and
mtrr_type_lookup_variable(), to handle the fixed and variable
ranges respectively.

The patch also updates the function headers to clarify the
return values and output argument.  It updates comments to
clarify that the repeating is necessary to handle overlaps
with the default type, since overlaps with multiple entries
alone can be handled without such repeating.

There is no functional change in this patch.

Signed-off-by: Toshi Kani <toshi.kani@hp.com>
---
 arch/x86/kernel/cpu/mtrr/generic.c |  137 +++++++++++++++++++++++-------------
 1 file changed, 86 insertions(+), 51 deletions(-)

diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
index 8bd1298..3652e2b 100644
--- a/arch/x86/kernel/cpu/mtrr/generic.c
+++ b/arch/x86/kernel/cpu/mtrr/generic.c
@@ -102,55 +102,69 @@ static int check_type_overlap(u8 *prev, u8 *curr)
 	return 0;
 }
 
-/*
- * Error/Semi-error returns:
- * MTRR_TYPE_INVALID - when MTRR is not enabled
- * *repeat == 1 implies [start:end] spanned across MTRR range and type returned
- *		corresponds only to [start:*partial_end].
- *		Caller has to lookup again for [*partial_end:end].
+/**
+ * mtrr_type_lookup_fixed - look up memory type in MTRR fixed entries
+ *
+ * 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 or fixed entries are disabled
  */
-static u8 __mtrr_type_lookup(u64 start, u64 end, u64 *partial_end, int *repeat)
+static u8 mtrr_type_lookup_fixed(u64 start, u64 end)
+{
+	int idx;
+
+	if (start >= 0x100000)
+		return MTRR_TYPE_INVALID;
+
+	if (!(mtrr_state.have_fixed) ||
+	    !(mtrr_state.enabled & MTRR_STATE_MTRR_FIXED_ENABLED))
+		return MTRR_TYPE_INVALID;
+
+	if (start < 0x80000) {		/* 0x0 - 0x7FFFF */
+		idx = 0;
+		idx += (start >> 16);
+		return mtrr_state.fixed_ranges[idx];
+
+	} else if (start < 0xC0000) {	/* 0x80000 - 0xBFFFF */
+		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 Argument:
+ * 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].
+ */
+static u8 mtrr_type_lookup_variable(u64 start, u64 end, u64 *partial_end,
+				    int *repeat)
 {
 	int i;
 	u64 base, mask;
 	u8 prev_match, curr_match;
 
 	*repeat = 0;
-	if (!mtrr_state_set)
-		return MTRR_TYPE_INVALID;
-
-	if (!(mtrr_state.enabled & MTRR_STATE_MTRR_ENABLED))
-		return MTRR_TYPE_INVALID;
 
 	/* Make end inclusive end, instead of exclusive */
 	end--;
 
-	/* Look in fixed ranges. Just return the type as per start */
-	if ((start < 0x100000) &&
-	    (mtrr_state.have_fixed) &&
-	    (mtrr_state.enabled & MTRR_STATE_MTRR_FIXED_ENABLED)) {
-		int idx;
-
-		if (start < 0x80000) {
-			idx = 0;
-			idx += (start >> 16);
-			return mtrr_state.fixed_ranges[idx];
-		} else if (start < 0xC0000) {
-			idx = 1 * 8;
-			idx += ((start - 0x80000) >> 14);
-			return mtrr_state.fixed_ranges[idx];
-		} else {
-			idx = 3 * 8;
-			idx += ((start - 0xC0000) >> 12);
-			return mtrr_state.fixed_ranges[idx];
-		}
-	}
-
-	/*
-	 * Look in variable ranges
-	 * Look of multiple ranges matching this address and pick type
-	 * as per MTRR precedence
-	 */
 	prev_match = MTRR_TYPE_INVALID;
 	for (i = 0; i < num_var_ranges; ++i) {
 		unsigned short start_state, end_state, inclusive;
@@ -179,7 +193,8 @@ static u8 __mtrr_type_lookup(u64 start, u64 end, u64 *partial_end, int *repeat)
 			 * Return the type for first region and a pointer to
 			 * the start of second region so that caller will
 			 * lookup again on the second region.
-			 * Note: This way we handle multiple overlaps as well.
+			 * Note: This way we handle overlaps with multiple
+			 * entries and the default type properly.
 			 */
 			if (start_state)
 				*partial_end = base + get_mtrr_size(mask);
@@ -208,21 +223,18 @@ static u8 __mtrr_type_lookup(u64 start, u64 end, u64 *partial_end, int *repeat)
 			return curr_match;
 	}
 
-	if (mtrr_tom2) {
-		if (start >= (1ULL<<32) && (end < mtrr_tom2))
-			return MTRR_TYPE_WRBACK;
-	}
-
 	if (prev_match != MTRR_TYPE_INVALID)
 		return prev_match;
 
 	return mtrr_state.def_type;
 }
 
-/*
- * Returns the effective MTRR type for the region
- * Error return:
- * MTRR_TYPE_INVALID - when MTRR is not enabled
+/**
+ * mtrr_type_lookup - look up memory type in MTRR
+ *
+ * Return Values:
+ * MTRR_TYPE_(type)  - The effective MTRR type for the region
+ * MTRR_TYPE_INVALID - MTRR is disabled
  */
 u8 mtrr_type_lookup(u64 start, u64 end)
 {
@@ -230,22 +242,45 @@ u8 mtrr_type_lookup(u64 start, u64 end)
 	int repeat;
 	u64 partial_end;
 
-	type = __mtrr_type_lookup(start, end, &partial_end, &repeat);
+	if (!mtrr_state_set)
+		return MTRR_TYPE_INVALID;
+
+	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.
+	 */
+	type = mtrr_type_lookup_fixed(start, end);
+	if (type != MTRR_TYPE_INVALID)
+		return type;
+
+	/*
+	 * 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);
 
 	/*
 	 * Common path is with repeat = 0.
 	 * However, we can have cases where [start:end] spans across some
-	 * MTRR range. Do repeated lookups for that case here.
+	 * MTRR ranges and/or the default type.  Do repeated lookups for
+	 * that case here.
 	 */
 	while (repeat) {
 		prev_type = type;
 		start = partial_end;
-		type = __mtrr_type_lookup(start, end, &partial_end, &repeat);
+		type = mtrr_type_lookup_variable(start, end, &partial_end,
+						 &repeat);
 
 		if (check_type_overlap(&prev_type, &type))
 			return type;
 	}
 
+	if (mtrr_tom2 && (start >= (1ULL<<32)) && (end < mtrr_tom2))
+		return MTRR_TYPE_WRBACK;
+
 	return type;
 }
 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH v4 7/7] mtrr, mm, x86: Enhance MTRR checks for KVA huge page mapping

[Toshi Kani]

This patch adds an additional argument, 'uniform', to
mtrr_type_lookup(), which returns 1 when a given range is
covered uniformly by MTRRs, i.e. the range is fully covered
by a single MTRR entry or the default type.

pud_set_huge() and pmd_set_huge() are changed to check the
new 'uniform' flag to see if it is safe to create a huge page
mapping to the range.  This allows them to create a huge page
mapping to a range covered by a single MTRR entry of any
memory type.  It also detects a non-optimal request properly.
They continue to check with the WB type since the WB type has
no effect even if a request spans multiple MTRR entries.

pmd_set_huge() logs a warning message to a non-optimal request
so that driver writers will be aware of such a case.  Drivers
should make a mapping request aligned to a single MTRR entry
when the range is covered by MTRRs.

Signed-off-by: Toshi Kani <toshi.kani@hp.com>
---
 arch/x86/include/asm/mtrr.h        |    5 +++--
 arch/x86/kernel/cpu/mtrr/generic.c |   35 +++++++++++++++++++++++++++--------
 arch/x86/mm/pat.c                  |    4 ++--
 arch/x86/mm/pgtable.c              |   25 +++++++++++++++----------
 4 files changed, 47 insertions(+), 22 deletions(-)

diff --git a/arch/x86/include/asm/mtrr.h b/arch/x86/include/asm/mtrr.h
index a174af6..da8dff1 100644
--- a/arch/x86/include/asm/mtrr.h
+++ b/arch/x86/include/asm/mtrr.h
@@ -31,7 +31,7 @@
  * arch_phys_wc_add and arch_phys_wc_del.
  */
 # ifdef CONFIG_MTRR
-extern u8 mtrr_type_lookup(u64 addr, u64 end);
+extern u8 mtrr_type_lookup(u64 addr, u64 end, u8 *uniform);
 extern void mtrr_save_fixed_ranges(void *);
 extern void mtrr_save_state(void);
 extern int mtrr_add(unsigned long base, unsigned long size,
@@ -50,11 +50,12 @@ extern int mtrr_trim_uncached_memory(unsigned long end_pfn);
 extern int amd_special_default_mtrr(void);
 extern int phys_wc_to_mtrr_index(int handle);
 #  else
-static inline u8 mtrr_type_lookup(u64 addr, u64 end)
+static inline u8 mtrr_type_lookup(u64 addr, u64 end, u8 *uniform)
 {
 	/*
 	 * Return no-MTRRs:
 	 */
+	*uniform = 1;
 	return MTRR_TYPE_INVALID;
 }
 #define mtrr_save_fixed_ranges(arg) do {} while (0)
diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
index 3652e2b..a83f27a 100644
--- a/arch/x86/kernel/cpu/mtrr/generic.c
+++ b/arch/x86/kernel/cpu/mtrr/generic.c
@@ -148,19 +148,22 @@ static u8 mtrr_type_lookup_fixed(u64 start, u64 end)
  * Return Value:
  * MTRR_TYPE_(type) - Matched memory type or default memory type (unmatched)
  *
- * Output Argument:
+ * 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 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)
+				    int *repeat, u8 *uniform)
 {
 	int i;
 	u64 base, mask;
 	u8 prev_match, curr_match;
 
 	*repeat = 0;
+	*uniform = 1;
 
 	/* Make end inclusive end, instead of exclusive */
 	end--;
@@ -208,6 +211,7 @@ static u8 mtrr_type_lookup_variable(u64 start, u64 end, u64 *partial_end,
 
 			end = *partial_end - 1; /* end is inclusive */
 			*repeat = 1;
+			*uniform = 0;
 		}
 
 		if (!start_state)
@@ -219,6 +223,7 @@ static u8 mtrr_type_lookup_variable(u64 start, u64 end, u64 *partial_end,
 			continue;
 		}
 
+		*uniform = 0;
 		if (check_type_overlap(&prev_match, &curr_match))
 			return curr_match;
 	}
@@ -235,13 +240,19 @@ static u8 mtrr_type_lookup_variable(u64 start, u64 end, u64 *partial_end,
  * Return Values:
  * MTRR_TYPE_(type)  - The effective MTRR type for the region
  * MTRR_TYPE_INVALID - MTRR is disabled
+ *
+ * Output Argument:
+ * uniform - Set to 1 when MTRR covers the region uniformly, i.e. the region
+ *	     is fully covered by a single MTRR entry or the default type.
  */
-u8 mtrr_type_lookup(u64 start, u64 end)
+u8 mtrr_type_lookup(u64 start, u64 end, u8 *uniform)
 {
-	u8 type, prev_type;
+	u8 type, prev_type, is_uniform, dummy;
 	int repeat;
 	u64 partial_end;
 
+	*uniform = 1;
+
 	if (!mtrr_state_set)
 		return MTRR_TYPE_INVALID;
 
@@ -253,14 +264,17 @@ u8 mtrr_type_lookup(u64 start, u64 end)
 	 * the variable ranges.
 	 */
 	type = mtrr_type_lookup_fixed(start, end);
-	if (type != MTRR_TYPE_INVALID)
+	if (type != MTRR_TYPE_INVALID) {
+		*uniform = 0;
 		return type;
+	}
 
 	/*
 	 * 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);
+	type = mtrr_type_lookup_variable(start, end, &partial_end,
+					 &repeat, &is_uniform);
 
 	/*
 	 * Common path is with repeat = 0.
@@ -271,16 +285,21 @@ u8 mtrr_type_lookup(u64 start, u64 end)
 	while (repeat) {
 		prev_type = type;
 		start = partial_end;
+		is_uniform = 0;
+
 		type = mtrr_type_lookup_variable(start, end, &partial_end,
-						 &repeat);
+						 &repeat, &dummy);
 
-		if (check_type_overlap(&prev_type, &type))
+		if (check_type_overlap(&prev_type, &type)) {
+			*uniform = 0;
 			return type;
+		}
 	}
 
 	if (mtrr_tom2 && (start >= (1ULL<<32)) && (end < mtrr_tom2))
 		return MTRR_TYPE_WRBACK;
 
+	*uniform = is_uniform;
 	return type;
 }
 
diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c
index 35af677..372ad42 100644
--- a/arch/x86/mm/pat.c
+++ b/arch/x86/mm/pat.c
@@ -267,9 +267,9 @@ static unsigned long pat_x_mtrr_type(u64 start, u64 end,
 	 * request is for WB.
 	 */
 	if (req_type == _PAGE_CACHE_MODE_WB) {
-		u8 mtrr_type;
+		u8 mtrr_type, uniform;
 
-		mtrr_type = mtrr_type_lookup(start, end);
+		mtrr_type = mtrr_type_lookup(start, end, &uniform);
 		if (mtrr_type != MTRR_TYPE_WRBACK)
 			return _PAGE_CACHE_MODE_UC_MINUS;
 
diff --git a/arch/x86/mm/pgtable.c b/arch/x86/mm/pgtable.c
index cfca4cf..3d6edea 100644
--- a/arch/x86/mm/pgtable.c
+++ b/arch/x86/mm/pgtable.c
@@ -567,17 +567,18 @@ void native_set_fixmap(enum fixed_addresses idx, phys_addr_t phys,
  * pud_set_huge - setup kernel PUD mapping
  *
  * MTRR can override PAT memory types with 4KB granularity.  Therefore,
- * it does not set up a huge page when the range is covered by a non-WB
- * type of MTRR.  MTRR_TYPE_INVALID indicates that MTRR are disabled.
+ * it only sets up a huge page when the range is mapped uniformly by MTRR
+ * (i.e. the range is fully covered by a single MTRR entry or the default
+ * type) or the MTRR memory type is WB.
  *
  * Return 1 on success, and 0 when no PUD was set.
  */
 int pud_set_huge(pud_t *pud, phys_addr_t addr, pgprot_t prot)
 {
-	u8 mtrr;
+	u8 mtrr, uniform;
 
-	mtrr = mtrr_type_lookup(addr, addr + PUD_SIZE);
-	if ((mtrr != MTRR_TYPE_WRBACK) && (mtrr != MTRR_TYPE_INVALID))
+	mtrr = mtrr_type_lookup(addr, addr + PUD_SIZE, &uniform);
+	if ((!uniform) && (mtrr != MTRR_TYPE_WRBACK))
 		return 0;
 
 	prot = pgprot_4k_2_large(prot);
@@ -593,18 +594,22 @@ int pud_set_huge(pud_t *pud, phys_addr_t addr, pgprot_t prot)
  * pmd_set_huge - setup kernel PMD mapping
  *
  * MTRR can override PAT memory types with 4KB granularity.  Therefore,
- * it does not set up a huge page when the range is covered by a non-WB
- * type of MTRR.  MTRR_TYPE_INVALID indicates that MTRR are disabled.
+ * it only sets up a huge page when the range is mapped uniformly by MTRR
+ * (i.e. the range is fully covered by a single MTRR entry or the default
+ * type) or the MTRR memory type is WB.
  *
  * Return 1 on success, and 0 when no PMD was set.
  */
 int pmd_set_huge(pmd_t *pmd, phys_addr_t addr, pgprot_t prot)
 {
-	u8 mtrr;
+	u8 mtrr, uniform;
 
-	mtrr = mtrr_type_lookup(addr, addr + PMD_SIZE);
-	if ((mtrr != MTRR_TYPE_WRBACK) && (mtrr != MTRR_TYPE_INVALID))
+	mtrr = mtrr_type_lookup(addr, addr + PMD_SIZE, &uniform);
+	if ((!uniform) && (mtrr != MTRR_TYPE_WRBACK)) {
+		pr_warn("pmd_set_huge: requesting [mem %#010llx-%#010llx], which spans more than a single MTRR entry\n",
+				addr, addr + PMD_SIZE);
 		return 0;
+	}
 
 	prot = pgprot_4k_2_large(prot);
 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v4 0/7] mtrr, mm, x86: Enhance MTRR checks for huge I/O mapping

[Andrew Morton]

On Tue, 24 Mar 2015 16:08:34 -0600 Toshi Kani <toshi.kani@hp.com> wrote:

> This patchset enhances MTRR checks for the kernel huge I/O mapping,
> which was enabled by the patchset below:
>   https://lkml.org/lkml/2015/3/3/589
> 
> The following functional changes are made in patch 7/7.
>  - Allow pud_set_huge() and pmd_set_huge() to create a huge page
>    mapping to a range covered by a single MTRR entry of any memory
>    type.
>  - Log a pr_warn() message when a specified PMD map range spans more
>    than a single MTRR entry.  Drivers should make a mapping request
>    aligned to a single MTRR entry when the range is covered by MTRRs.
> 

OK, I grabbed these after barely looking at them, to get them a bit of
runtime testing.

I'll await guidance from the x86 maintainers regarding next steps?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v4 0/7] mtrr, mm, x86: Enhance MTRR checks for huge I/O mapping

[Ingo Molnar]

* Andrew Morton <akpm@linux-foundation.org> wrote:

> On Tue, 24 Mar 2015 16:08:34 -0600 Toshi Kani <toshi.kani@hp.com> wrote:
> 
> > This patchset enhances MTRR checks for the kernel huge I/O mapping,
> > which was enabled by the patchset below:
> >   https://lkml.org/lkml/2015/3/3/589
> > 
> > The following functional changes are made in patch 7/7.
> >  - Allow pud_set_huge() and pmd_set_huge() to create a huge page
> >    mapping to a range covered by a single MTRR entry of any memory
> >    type.
> >  - Log a pr_warn() message when a specified PMD map range spans more
> >    than a single MTRR entry.  Drivers should make a mapping request
> >    aligned to a single MTRR entry when the range is covered by MTRRs.
> > 
> 
> OK, I grabbed these after barely looking at them, to get them a bit of
> runtime testing.
> 
> I'll await guidance from the x86 maintainers regarding next steps?

Could you please send the current version of them over to us if your 
testing didn't find any problems?

I'd like to take a final look and have them cook in the x86 tree as 
well for a while and want to preserve your testing effort.

Thanks!

	Ingo

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v4 0/7] mtrr, mm, x86: Enhance MTRR checks for huge I/O mapping

[Toshi Kani]

On Fri, 2015-04-03 at 08:33 +0200, Ingo Molnar wrote:
> * Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > On Tue, 24 Mar 2015 16:08:34 -0600 Toshi Kani <toshi.kani@hp.com> wrote:
> > 
> > > This patchset enhances MTRR checks for the kernel huge I/O mapping,
> > > which was enabled by the patchset below:
> > >   https://lkml.org/lkml/2015/3/3/589
> > > 
> > > The following functional changes are made in patch 7/7.
> > >  - Allow pud_set_huge() and pmd_set_huge() to create a huge page
> > >    mapping to a range covered by a single MTRR entry of any memory
> > >    type.
> > >  - Log a pr_warn() message when a specified PMD map range spans more
> > >    than a single MTRR entry.  Drivers should make a mapping request
> > >    aligned to a single MTRR entry when the range is covered by MTRRs.
> > > 
> > 
> > OK, I grabbed these after barely looking at them, to get them a bit of
> > runtime testing.
> > 
> > I'll await guidance from the x86 maintainers regarding next steps?
> 
> Could you please send the current version of them over to us if your 
> testing didn't find any problems?
> 
> I'd like to take a final look and have them cook in the x86 tree as 
> well for a while and want to preserve your testing effort.

This patchset is on top of the following patches in the -mm tree.
(Patches apply from the bottom to the top.)

2. Build error fixes and cleanups
http://ozlabs.org/~akpm/mmotm/broken-out/x86-mm-support-huge-kva-mappings-on-x86-fix.patch
http://ozlabs.org/~akpm/mmotm/broken-out/mm-change-vunmap-to-tear-down-huge-kva-mappings-fix.patch
http://ozlabs.org/~akpm/mmotm/broken-out/mm-change-ioremap-to-set-up-huge-i-o-mappings-fix.patch
http://ozlabs.org/~akpm/mmotm/broken-out/lib-add-huge-i-o-map-capability-interfaces-fix.patch

1. Kernel huge I/O mapping support
http://ozlabs.org/~akpm/mmotm/broken-out/x86-mm-support-huge-kva-mappings-on-x86.patch
http://ozlabs.org/~akpm/mmotm/broken-out/x86-mm-support-huge-i-o-mapping-capability-i-f.patch
http://ozlabs.org/~akpm/mmotm/broken-out/mm-change-vunmap-to-tear-down-huge-kva-mappings.patch
http://ozlabs.org/~akpm/mmotm/broken-out/mm-change-ioremap-to-set-up-huge-i-o-mappings.patch
http://ozlabs.org/~akpm/mmotm/broken-out/lib-add-huge-i-o-map-capability-interfaces.patch
http://ozlabs.org/~akpm/mmotm/broken-out/mm-change-__get_vm_area_node-to-use-fls_long.patch

Thanks,
-Toshi


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v4 0/7] mtrr, mm, x86: Enhance MTRR checks for huge I/O mapping

[Toshi Kani]

On Fri, 2015-04-03 at 09:22 -0600, Toshi Kani wrote:
> On Fri, 2015-04-03 at 08:33 +0200, Ingo Molnar wrote:
> > * Andrew Morton <akpm@linux-foundation.org> wrote:
> > 
> > > On Tue, 24 Mar 2015 16:08:34 -0600 Toshi Kani <toshi.kani@hp.com> wrote:
> > > 
> > > > This patchset enhances MTRR checks for the kernel huge I/O mapping,
> > > > which was enabled by the patchset below:
> > > >   https://lkml.org/lkml/2015/3/3/589
> > > > 
> > > > The following functional changes are made in patch 7/7.
> > > >  - Allow pud_set_huge() and pmd_set_huge() to create a huge page
> > > >    mapping to a range covered by a single MTRR entry of any memory
> > > >    type.
> > > >  - Log a pr_warn() message when a specified PMD map range spans more
> > > >    than a single MTRR entry.  Drivers should make a mapping request
> > > >    aligned to a single MTRR entry when the range is covered by MTRRs.
> > > > 
> > > 
> > > OK, I grabbed these after barely looking at them, to get them a bit of
> > > runtime testing.
> > > 
> > > I'll await guidance from the x86 maintainers regarding next steps?
> > 
> > Could you please send the current version of them over to us if your 
> > testing didn't find any problems?
> > 
> > I'd like to take a final look and have them cook in the x86 tree as 
> > well for a while and want to preserve your testing effort.
> 
> This patchset is on top of the following patches in the -mm tree.
> (Patches apply from the bottom to the top.)

Ingo,

The following patches (2 got squashed to 1) went to 4.1-rc1, but this
patch-set is still sitting in the -mm tree.  I confirmed that the
patch-set applies cleanly to 4.1-rc1.  Please take a final look and let
me know if you have any comment.

Thanks,
-Toshi


> 2. Build error fixes and cleanups
> http://ozlabs.org/~akpm/mmotm/broken-out/x86-mm-support-huge-kva-mappings-on-x86-fix.patch
> http://ozlabs.org/~akpm/mmotm/broken-out/mm-change-vunmap-to-tear-down-huge-kva-mappings-fix.patch
> http://ozlabs.org/~akpm/mmotm/broken-out/mm-change-ioremap-to-set-up-huge-i-o-mappings-fix.patch
> http://ozlabs.org/~akpm/mmotm/broken-out/lib-add-huge-i-o-map-capability-interfaces-fix.patch
> 
> 1. Kernel huge I/O mapping support
> http://ozlabs.org/~akpm/mmotm/broken-out/x86-mm-support-huge-kva-mappings-on-x86.patch
> http://ozlabs.org/~akpm/mmotm/broken-out/x86-mm-support-huge-i-o-mapping-capability-i-f.patch
> http://ozlabs.org/~akpm/mmotm/broken-out/mm-change-vunmap-to-tear-down-huge-kva-mappings.patch
> http://ozlabs.org/~akpm/mmotm/broken-out/mm-change-ioremap-to-set-up-huge-i-o-mappings.patch
> http://ozlabs.org/~akpm/mmotm/broken-out/lib-add-huge-i-o-map-capability-interfaces.patch
> http://ozlabs.org/~akpm/mmotm/broken-out/mm-change-__get_vm_area_node-to-use-fls_long.patch
> 
> Thanks,
> -Toshi
> 


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v4 1/7] mm, x86: Document return values of mapping funcs

[Borislav Petkov]

On Tue, Mar 24, 2015 at 04:08:35PM -0600, Toshi Kani wrote:
> Document the return values of KVA mapping functions,

KVA?

Please write it out.

> pud_set_huge(), pmd_set_huge, pud_clear_huge() and
> pmd_clear_huge().
> 
> Simplify the conditions to select HAVE_ARCH_HUGE_VMAP
> in the Kconfig, since X86_PAE depends on X86_32.
> 
> There is no functional change in this patch.
> 
> Signed-off-by: Toshi Kani <toshi.kani@hp.com>
> ---
>  arch/x86/Kconfig      |    2 +-
>  arch/x86/mm/pgtable.c |   36 ++++++++++++++++++++++++++++--------
>  2 files changed, 29 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index cb23206..2ea27da 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -99,7 +99,7 @@ config X86
>  	select IRQ_FORCED_THREADING
>  	select HAVE_BPF_JIT if X86_64
>  	select HAVE_ARCH_TRANSPARENT_HUGEPAGE
> -	select HAVE_ARCH_HUGE_VMAP if X86_64 || (X86_32 && X86_PAE)
> +	select HAVE_ARCH_HUGE_VMAP if X86_64 || X86_PAE
>  	select ARCH_HAS_SG_CHAIN
>  	select CLKEVT_I8253
>  	select ARCH_HAVE_NMI_SAFE_CMPXCHG

This is an unrelated change, please carve it out in a separate patch.

> diff --git a/arch/x86/mm/pgtable.c b/arch/x86/mm/pgtable.c
> index 0b97d2c..4891fa1 100644
> --- a/arch/x86/mm/pgtable.c
> +++ b/arch/x86/mm/pgtable.c
> @@ -563,14 +563,19 @@ void native_set_fixmap(enum fixed_addresses idx, phys_addr_t phys,
>  }
>  
>  #ifdef CONFIG_HAVE_ARCH_HUGE_VMAP
> +/**
> + * pud_set_huge - setup kernel PUD mapping
> + *
> + * MTRR can override PAT memory types with 4KB granularity.  Therefore,
> + * it does not set up a huge page when the range is covered by a non-WB

"it" is what exactly?

> + * type of MTRR.  0xFF indicates that MTRR are disabled.

So this shows that this patch shouldn't be the first one in the series.

IMO you want to start with cleaning up mtrr_type_lookup(), add the
defines for its retval and *then* document its users. This way you won't
have to touch the same place twice, the net-size of your patchset will
go down and it will be easier for reviewiers.

> + *
> + * Return 1 on success, and 0 when no PUD was set.

"Returns 1 on success and 0 on failure."

> + */
>  int pud_set_huge(pud_t *pud, phys_addr_t addr, pgprot_t prot)
>  {
>  	u8 mtrr;
>  
> -	/*
> -	 * Do not use a huge page when the range is covered by non-WB type
> -	 * of MTRRs.
> -	 */
>  	mtrr = mtrr_type_lookup(addr, addr + PUD_SIZE);
>  	if ((mtrr != MTRR_TYPE_WRBACK) && (mtrr != 0xFF))
>  		return 0;

Ditto for the rest.

Thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v4 1/7] mm, x86: Document return values of mapping funcs

[Toshi Kani]

On Tue, 2015-05-05 at 13:19 +0200, Borislav Petkov wrote:
> On Tue, Mar 24, 2015 at 04:08:35PM -0600, Toshi Kani wrote:
> > Document the return values of KVA mapping functions,
> 
> KVA?
> Please write it out.

Will expand it as Kernel Virtual Address.

> > pud_set_huge(), pmd_set_huge, pud_clear_huge() and
> > pmd_clear_huge().
> > 
> > Simplify the conditions to select HAVE_ARCH_HUGE_VMAP
> > in the Kconfig, since X86_PAE depends on X86_32.
> > 
> > There is no functional change in this patch.
> > 
> > Signed-off-by: Toshi Kani <toshi.kani@hp.com>
> > ---
> >  arch/x86/Kconfig      |    2 +-
> >  arch/x86/mm/pgtable.c |   36 ++++++++++++++++++++++++++++--------
> >  2 files changed, 29 insertions(+), 9 deletions(-)
> > 
> > diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> > index cb23206..2ea27da 100644
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -99,7 +99,7 @@ config X86
> >  	select IRQ_FORCED_THREADING
> >  	select HAVE_BPF_JIT if X86_64
> >  	select HAVE_ARCH_TRANSPARENT_HUGEPAGE
> > -	select HAVE_ARCH_HUGE_VMAP if X86_64 || (X86_32 && X86_PAE)
> > +	select HAVE_ARCH_HUGE_VMAP if X86_64 || X86_PAE
> >  	select ARCH_HAS_SG_CHAIN
> >  	select CLKEVT_I8253
> >  	select ARCH_HAVE_NMI_SAFE_CMPXCHG
> 
> This is an unrelated change, please carve it out in a separate patch.

Will do.

> > diff --git a/arch/x86/mm/pgtable.c b/arch/x86/mm/pgtable.c
> > index 0b97d2c..4891fa1 100644
> > --- a/arch/x86/mm/pgtable.c
> > +++ b/arch/x86/mm/pgtable.c
> > @@ -563,14 +563,19 @@ void native_set_fixmap(enum fixed_addresses idx, phys_addr_t phys,
> >  }
> >  
> >  #ifdef CONFIG_HAVE_ARCH_HUGE_VMAP
> > +/**
> > + * pud_set_huge - setup kernel PUD mapping
> > + *
> > + * MTRR can override PAT memory types with 4KB granularity.  Therefore,
> > + * it does not set up a huge page when the range is covered by a non-WB
> 
> "it" is what exactly?

Will change to "this function".

> > + * type of MTRR.  0xFF indicates that MTRR are disabled.
> 
> So this shows that this patch shouldn't be the first one in the series.
> 
> IMO you want to start with cleaning up mtrr_type_lookup(), add the
> defines for its retval and *then* document its users. This way you won't
> have to touch the same place twice, the net-size of your patchset will
> go down and it will be easier for reviewiers.

Agreed.  This patch-set was originally a small set of patches, but was
extended later with additional patches, which ended up with touching the
same place again.  I will reorganize the patch-set. 

> > + *
> > + * Return 1 on success, and 0 when no PUD was set.
> 
> "Returns 1 on success and 0 on failure."

Will do.

> > + */
> >  int pud_set_huge(pud_t *pud, phys_addr_t addr, pgprot_t prot)
> >  {
> >  	u8 mtrr;
> >  
> > -	/*
> > -	 * Do not use a huge page when the range is covered by non-WB type
> > -	 * of MTRRs.
> > -	 */
> >  	mtrr = mtrr_type_lookup(addr, addr + PUD_SIZE);
> >  	if ((mtrr != MTRR_TYPE_WRBACK) && (mtrr != 0xFF))
> >  		return 0;
> 
> Ditto for the rest.

Will do.

Thanks,
-Toshi


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v4 1/7] mm, x86: Document return values of mapping funcs

[Toshi Kani]

On Tue, 2015-05-05 at 16:19 +0200, Borislav Petkov wrote:
> On Tue, May 05, 2015 at 07:46:36AM -0600, Toshi Kani wrote:
> > Agreed.  This patch-set was originally a small set of patches, but was
> > extended later with additional patches, which ended up with touching the
> > same place again.  I will reorganize the patch-set.
> 
> Ok, but please wait until I take a look at the rest.

Sure, I will wait for your review.  

> 
> Thanks.
> 
> Btw, is there anything else MTRR-related pending for tip?

Not exactly MTRR-related, but I am planing to re-submit my WT patchset
after checking to see if Luis's patchset (which you are reviewing) has
any conflict with this.

https://lkml.org/lkml/2015/2/24/773

Thanks,
-Toshi

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v4 1/7] mm, x86: Document return values of mapping funcs

[Borislav Petkov]

On Tue, May 05, 2015 at 07:46:36AM -0600, Toshi Kani wrote:
> Agreed.  This patch-set was originally a small set of patches, but was
> extended later with additional patches, which ended up with touching the
> same place again.  I will reorganize the patch-set.

Ok, but please wait until I take a look at the rest.

Thanks.

Btw, is there anything else MTRR-related pending for tip?

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v4 2/7] mtrr, x86: Fix MTRR lookup to handle inclusive entry

[Borislav Petkov]

On Tue, Mar 24, 2015 at 04:08:36PM -0600, Toshi Kani wrote:
> When an MTRR entry is inclusive to a requested range, i.e.
> the start and end of the request are not within the MTRR
> entry range but the range contains the MTRR entry entirely,
> __mtrr_type_lookup() ignores such a case because both
> start_state and end_state are set to zero.
> 
> This bug can cause the following issues:
> 1) reserve_memtype() tracks an effective memory type in case
>    a request type is WB (ex. /dev/mem blindly uses WB). Missing
>    to track with its effective type causes a subsequent request
>    to map the same range with the effective type to fail.
> 2) pud_set_huge() and pmd_set_huge() check if a requested range
>    has any overlap with MTRRs. Missing to detect an overlap may
>    cause a performance penalty or undefined behavior.
> 
> This patch fixes the bug by adding a new flag, 'inclusive',
> to detect the inclusive case.  This case is then handled in
> the same way as (!start_state && end_state).  With this fix,
> __mtrr_type_lookup() handles the inclusive case properly.
> 
> Signed-off-by: Toshi Kani <toshi.kani@hp.com>
> ---
>  arch/x86/kernel/cpu/mtrr/generic.c |   17 +++++++++--------
>  1 file changed, 9 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
> index 7d74f7b..a82e370 100644
> --- a/arch/x86/kernel/cpu/mtrr/generic.c
> +++ b/arch/x86/kernel/cpu/mtrr/generic.c
> @@ -154,7 +154,7 @@ static u8 __mtrr_type_lookup(u64 start, u64 end, u64 *partial_end, int *repeat)
>  
>  	prev_match = 0xFF;
>  	for (i = 0; i < num_var_ranges; ++i) {
> -		unsigned short start_state, end_state;
> +		unsigned short start_state, end_state, inclusive;
>  
>  		if (!(mtrr_state.var_ranges[i].mask_lo & (1 << 11)))
>  			continue;
> @@ -166,15 +166,16 @@ static u8 __mtrr_type_lookup(u64 start, u64 end, u64 *partial_end, int *repeat)
>  
>  		start_state = ((start & mask) == (base & mask));
>  		end_state = ((end & mask) == (base & mask));
> +		inclusive = ((start < base) && (end > base));
>  
> -		if (start_state != end_state) {
> +		if ((start_state != end_state) || inclusive) {
>  			/*
>  			 * We have start:end spanning across an MTRR.
> -			 * We split the region into
> -			 * either
> -			 * (start:mtrr_end) (mtrr_end:end)
> -			 * or
> -			 * (start:mtrr_start) (mtrr_start:end)
> +			 * We split the region into either
> +			 * - start_state:1
> +			 *     (start:mtrr_end) (mtrr_end:end)
> +			 * - end_state:1 or inclusive:1
> +			 *     (start:mtrr_start) (mtrr_start:end)

Ok, I'm confused. Shouldn't the inclusive:1 case be

			(start:mtrr_start) (mtrr_start:mtrr_end) (mtrr_end:end)

?

If so, this function would need more changes...

>  			 * depending on kind of overlap.
>  			 * Return the type for first region and a pointer to
>  			 * the start of second region so that caller will
> @@ -195,7 +196,7 @@ static u8 __mtrr_type_lookup(u64 start, u64 end, u64 *partial_end, int *repeat)
>  			*repeat = 1;
>  		}
>  
> -		if ((start & mask) != (base & mask))
> +		if (!start_state)
>  			continue;

That change actually makes the code more unreadable because you have to
go and look up what start_state was and the previous version actually
shows the check that start is within the range, exactly like it is
documented in the CPU manuals.

And I'd leave it this way because gcc is smart enough to reload the
result saved in start_state and not compute it again.

Thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v4 2/7] mtrr, x86: Fix MTRR lookup to handle inclusive entry

[Toshi Kani]

On Tue, 2015-05-05 at 19:11 +0200, Borislav Petkov wrote:
> On Tue, Mar 24, 2015 at 04:08:36PM -0600, Toshi Kani wrote:
> > When an MTRR entry is inclusive to a requested range, i.e.
> > the start and end of the request are not within the MTRR
> > entry range but the range contains the MTRR entry entirely,
> > __mtrr_type_lookup() ignores such a case because both
> > start_state and end_state are set to zero.
> > 
> > This bug can cause the following issues:
> > 1) reserve_memtype() tracks an effective memory type in case
> >    a request type is WB (ex. /dev/mem blindly uses WB). Missing
> >    to track with its effective type causes a subsequent request
> >    to map the same range with the effective type to fail.
> > 2) pud_set_huge() and pmd_set_huge() check if a requested range
> >    has any overlap with MTRRs. Missing to detect an overlap may
> >    cause a performance penalty or undefined behavior.
> > 
> > This patch fixes the bug by adding a new flag, 'inclusive',
> > to detect the inclusive case.  This case is then handled in
> > the same way as (!start_state && end_state).  With this fix,
> > __mtrr_type_lookup() handles the inclusive case properly.
> > 
> > Signed-off-by: Toshi Kani <toshi.kani@hp.com>
> > ---
> >  arch/x86/kernel/cpu/mtrr/generic.c |   17 +++++++++--------
> >  1 file changed, 9 insertions(+), 8 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
> > index 7d74f7b..a82e370 100644
> > --- a/arch/x86/kernel/cpu/mtrr/generic.c
> > +++ b/arch/x86/kernel/cpu/mtrr/generic.c
> > @@ -154,7 +154,7 @@ static u8 __mtrr_type_lookup(u64 start, u64 end, u64 *partial_end, int *repeat)
> >  
> >  	prev_match = 0xFF;
> >  	for (i = 0; i < num_var_ranges; ++i) {
> > -		unsigned short start_state, end_state;
> > +		unsigned short start_state, end_state, inclusive;
> >  
> >  		if (!(mtrr_state.var_ranges[i].mask_lo & (1 << 11)))
> >  			continue;
> > @@ -166,15 +166,16 @@ static u8 __mtrr_type_lookup(u64 start, u64 end, u64 *partial_end, int *repeat)
> >  
> >  		start_state = ((start & mask) == (base & mask));
> >  		end_state = ((end & mask) == (base & mask));
> > +		inclusive = ((start < base) && (end > base));
> >  
> > -		if (start_state != end_state) {
> > +		if ((start_state != end_state) || inclusive) {
> >  			/*
> >  			 * We have start:end spanning across an MTRR.
> > -			 * We split the region into
> > -			 * either
> > -			 * (start:mtrr_end) (mtrr_end:end)
> > -			 * or
> > -			 * (start:mtrr_start) (mtrr_start:end)
> > +			 * We split the region into either
> > +			 * - start_state:1
> > +			 *     (start:mtrr_end) (mtrr_end:end)
> > +			 * - end_state:1 or inclusive:1
> > +			 *     (start:mtrr_start) (mtrr_start:end)
> 
> Ok, I'm confused. Shouldn't the inclusive:1 case be
> 
> 			(start:mtrr_start) (mtrr_start:mtrr_end) (mtrr_end:end)
> 
> ?
> 
> If so, this function would need more changes...

Yes, that's how it gets separated eventually.  Since *repeat is set in
this case, the code only needs to separate the first part at a time.
The 2nd part gets separated in the next call with the *repeat.


> >  			 * depending on kind of overlap.
> >  			 * Return the type for first region and a pointer to
> >  			 * the start of second region so that caller will
> > @@ -195,7 +196,7 @@ static u8 __mtrr_type_lookup(u64 start, u64 end, u64 *partial_end, int *repeat)
> >  			*repeat = 1;
> >  		}
> >  
> > -		if ((start & mask) != (base & mask))
> > +		if (!start_state)
> >  			continue;
> 
> That change actually makes the code more unreadable because you have to
> go and look up what start_state was and the previous version actually
> shows the check that start is within the range, exactly like it is
> documented in the CPU manuals.
> 
> And I'd leave it this way because gcc is smart enough to reload the
> result saved in start_state and not compute it again.

When I see such re-calculation, it makes me look at the code again to
see if there is a case that updates the parameters after the first
calculation...  That said, I am OK as long as gcc is smart enough to
reload the value.  I will put it back to the original.

Thanks,
-Toshi

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v4 2/7] mtrr, x86: Fix MTRR lookup to handle inclusive entry

[Borislav Petkov]

On Tue, May 05, 2015 at 11:32:08AM -0600, Toshi Kani wrote:
> > Ok, I'm confused. Shouldn't the inclusive:1 case be
> > 
> > 			(start:mtrr_start) (mtrr_start:mtrr_end) (mtrr_end:end)
> > 
> > ?
> > 
> > If so, this function would need more changes...
> 
> Yes, that's how it gets separated eventually.  Since *repeat is set in
> this case, the code only needs to separate the first part at a time.
> The 2nd part gets separated in the next call with the *repeat.

Aah, right, the caller is supposed to adjust the interval limits on
subsequent calls. Please reflect this in the comment because:

		*     (start:mtrr_start) (mtrr_start:end)

is misleading for inclusive:1.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v4 2/7] mtrr, x86: Fix MTRR lookup to handle inclusive entry

[Toshi Kani]

On Tue, 2015-05-05 at 20:39 +0200, Borislav Petkov wrote:
> On Tue, May 05, 2015 at 11:32:08AM -0600, Toshi Kani wrote:
> > > Ok, I'm confused. Shouldn't the inclusive:1 case be
> > > 
> > > 			(start:mtrr_start) (mtrr_start:mtrr_end) (mtrr_end:end)
> > > 
> > > ?
> > > 
> > > If so, this function would need more changes...
> > 
> > Yes, that's how it gets separated eventually.  Since *repeat is set in
> > this case, the code only needs to separate the first part at a time.
> > The 2nd part gets separated in the next call with the *repeat.
> 
> Aah, right, the caller is supposed to adjust the interval limits on
> subsequent calls. Please reflect this in the comment because:
> 
> 		*     (start:mtrr_start) (mtrr_start:end)
> 
> is misleading for inclusive:1.

Well, the comment kinda says it already, but I will try to clarify it.

           /*
            * 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 or inclusive:1
            *     (start:mtrr_start) (mtrr_start:end)
            * depending on kind of overlap.
            * Return the type for first region and a pointer to
            * the start of second region so that caller will
            * lookup again on the second region.
            * Note: This way we handle multiple overlaps as well.
            */

Thanks,
-Toshi

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v4 2/7] mtrr, x86: Fix MTRR lookup to handle inclusive entry

[Toshi Kani]

On Tue, 2015-05-05 at 22:09 +0200, Borislav Petkov wrote:
> On Tue, May 05, 2015 at 01:31:32PM -0600, Toshi Kani wrote:
> > Well, the comment kinda says it already, but I will try to clarify it.
> > 
> >            /*
> >             * 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 or inclusive:1
> >             *     (start:mtrr_start) (mtrr_start:end)
> 
> What I mean is this:
> 
> 		* - 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 multiple overlaps as well.
> 		*/
> 
> We add comments so that people can read them and can quickly understand
> what the function does. Not to make them parse it and wonder why
> inclusive:1 is listed together with end_state:1 which returns two
> intervals.
> 
> Note that I changed the text to talk about the *next* region and not
> about the *second* region, to make it even more clear.

Thanks for the suggestion.  I see your point.  I will update it
accordingly.
-Toshi



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v4 2/7] mtrr, x86: Fix MTRR lookup to handle inclusive entry

[Borislav Petkov]

On Tue, May 05, 2015 at 01:31:32PM -0600, Toshi Kani wrote:
> Well, the comment kinda says it already, but I will try to clarify it.
> 
>            /*
>             * 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 or inclusive:1
>             *     (start:mtrr_start) (mtrr_start:end)

What I mean is this:

		* - 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 multiple overlaps as well.
		*/

We add comments so that people can read them and can quickly understand
what the function does. Not to make them parse it and wonder why
inclusive:1 is listed together with end_state:1 which returns two
intervals.

Note that I changed the text to talk about the *next* region and not
about the *second* region, to make it even more clear.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v4 3/7] mtrr, x86: Remove a wrong address check in __mtrr_type_lookup()

[Borislav Petkov]

On Tue, Mar 24, 2015 at 04:08:37PM -0600, Toshi Kani wrote:
> __mtrr_type_lookup() checks MTRR fixed ranges when
> mtrr_state.have_fixed is set and start is less than
> 0x100000.  However, the 'else if (start < 0x1000000)'
> in the code checks with a wrong address as it has
> an extra-zero in the address.  The code still runs
> correctly as this check is meaningless, though.
> 
> This patch replaces the wrong address check with 'else'
> with no condition.
> 
> Signed-off-by: Toshi Kani <toshi.kani@hp.com>
> ---
>  arch/x86/kernel/cpu/mtrr/generic.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Applied, thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v4 4/7] mtrr, x86: Fix MTRR state checks in mtrr_type_lookup()

[Borislav Petkov]

On Tue, Mar 24, 2015 at 04:08:38PM -0600, Toshi Kani wrote:
> 'mtrr_state.enabled' contains the FE (fixed MTRRs enabled)
> and E (MTRRs enabled) flags in MSR_MTRRdefType.  Intel SDM,
> section 11.11.2.1, defines these flags as follows:
>  - All MTRRs are disabled when the E flag is clear.
>    The FE flag has no affect when the E flag is clear.
>  - The default type is enabled when the E flag is set.
>  - MTRR variable ranges are enabled when the E flag is set.
>  - MTRR fixed ranges are enabled when both E and FE flags
>    are set.
> 
> MTRR state checks in __mtrr_type_lookup() do not match with
> SDM.  Hence, this patch makes the following changes:
>  - The current code detects MTRRs disabled when both E and
>    FE flags are clear in mtrr_state.enabled.  Fix to detect
>    MTRRs disabled when the E flag is clear.
>  - The current code does not check if the FE bit is set in
>    mtrr_state.enabled when looking into the fixed entries.
>    Fix to check the FE flag.
>  - The current code returns the default type when the E flag
>    is clear in mtrr_state.enabled.  However, the default type
>    is also disabled when the E flag is clear.  Fix to remove
>    the code as this case is handled as MTRR disabled with
>    the 1st change.
> 
> In addition, this patch defines the E and FE flags in
> mtrr_state.enabled as follows.
>  - FE flag: MTRR_STATE_MTRR_FIXED_ENABLED
>  - E  flag: MTRR_STATE_MTRR_ENABLED
> 
> print_mtrr_state() is also updated accordingly.
> 
> Signed-off-by: Toshi Kani <toshi.kani@hp.com>
> ---
>  arch/x86/include/uapi/asm/mtrr.h   |    4 ++++
>  arch/x86/kernel/cpu/mtrr/generic.c |   15 ++++++++-------
>  2 files changed, 12 insertions(+), 7 deletions(-)

You missed a spot in the conversion in
arch/x86/kernel/cpu/mtrr/cleanup.c::x86_get_mtrr_mem_range():

There we have

	if (base < (1<<(20-PAGE_SHIFT)) && mtrr_state.have_fixed &&
	    (mtrr_state.enabled & 1)) {

which should be mtrr_state.enabled & MTRR_STATE_MTRR_FIXED_ENABLED.

> diff --git a/arch/x86/include/uapi/asm/mtrr.h b/arch/x86/include/uapi/asm/mtrr.h
> index d0acb65..66ba88d 100644
> --- a/arch/x86/include/uapi/asm/mtrr.h
> +++ b/arch/x86/include/uapi/asm/mtrr.h
> @@ -88,6 +88,10 @@ struct mtrr_state_type {
>         mtrr_type def_type;
>  };
> 
> +/* Bit fields for enabled in struct mtrr_state_type */
> +#define MTRR_STATE_MTRR_FIXED_ENABLED  0x01
> +#define MTRR_STATE_MTRR_ENABLED                0x02
> +
>  #define MTRRphysBase_MSR(reg) (0x200 + 2 * (reg))
>  #define MTRRphysMask_MSR(reg) (0x200 + 2 * (reg) + 1)

Please add those to arch/x86/include/asm/mtrr.h instead. They have no
place in the uapi header.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v4 6/7] mtrr, x86: Clean up mtrr_type_lookup()

[Borislav Petkov]

On Tue, Mar 24, 2015 at 04:08:40PM -0600, Toshi Kani wrote:
> MTRRs contain fixed and variable entries.  mtrr_type_lookup()
> may repeatedly call __mtrr_type_lookup() to handle a request
> that overlaps with variable entries.  However,
> __mtrr_type_lookup() also handles the fixed entries, which
> do not have to be repeated.  Therefore, this patch creates
> separate functions, mtrr_type_lookup_fixed() and
> mtrr_type_lookup_variable(), to handle the fixed and variable
> ranges respectively.
> 
> The patch also updates the function headers to clarify the
> return values and output argument.  It updates comments to
> clarify that the repeating is necessary to handle overlaps
> with the default type, since overlaps with multiple entries
> alone can be handled without such repeating.
> 
> There is no functional change in this patch.
> 
> Signed-off-by: Toshi Kani <toshi.kani@hp.com>
> ---
>  arch/x86/kernel/cpu/mtrr/generic.c |  137 +++++++++++++++++++++++-------------
>  1 file changed, 86 insertions(+), 51 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
> index 8bd1298..3652e2b 100644
> --- a/arch/x86/kernel/cpu/mtrr/generic.c
> +++ b/arch/x86/kernel/cpu/mtrr/generic.c
> @@ -102,55 +102,69 @@ static int check_type_overlap(u8 *prev, u8 *curr)
>  	return 0;
>  }
>  
> -/*
> - * Error/Semi-error returns:
> - * MTRR_TYPE_INVALID - when MTRR is not enabled
> - * *repeat == 1 implies [start:end] spanned across MTRR range and type returned
> - *		corresponds only to [start:*partial_end].
> - *		Caller has to lookup again for [*partial_end:end].
> +/**
> + * mtrr_type_lookup_fixed - look up memory type in MTRR fixed entries
> + *
> + * 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

No need for those - simply a pointer to either the SDM or APM manuals'
section suffices as they both describe it good.

> + *
> + * Return Values:
> + * MTRR_TYPE_(type)  - Matched memory type
> + * MTRR_TYPE_INVALID - Unmatched or fixed entries are disabled
>   */
> -static u8 __mtrr_type_lookup(u64 start, u64 end, u64 *partial_end, int *repeat)
> +static u8 mtrr_type_lookup_fixed(u64 start, u64 end)
> +{
> +	int idx;
> +
> +	if (start >= 0x100000)
> +		return MTRR_TYPE_INVALID;
> +
> +	if (!(mtrr_state.have_fixed) ||
> +	    !(mtrr_state.enabled & MTRR_STATE_MTRR_FIXED_ENABLED))
> +		return MTRR_TYPE_INVALID;
> +
> +	if (start < 0x80000) {		/* 0x0 - 0x7FFFF */
> +		idx = 0;
> +		idx += (start >> 16);
> +		return mtrr_state.fixed_ranges[idx];
> +
> +	} else if (start < 0xC0000) {	/* 0x80000 - 0xBFFFF */
> +		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 Argument:
> + * 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].
> + */
> +static u8 mtrr_type_lookup_variable(u64 start, u64 end, u64 *partial_end,
> +				    int *repeat)
>  {
>  	int i;
>  	u64 base, mask;
>  	u8 prev_match, curr_match;
>  
>  	*repeat = 0;
> -	if (!mtrr_state_set)
> -		return MTRR_TYPE_INVALID;
> -
> -	if (!(mtrr_state.enabled & MTRR_STATE_MTRR_ENABLED))
> -		return MTRR_TYPE_INVALID;
>  
>  	/* Make end inclusive end, instead of exclusive */
>  	end--;
>  
> -	/* Look in fixed ranges. Just return the type as per start */
> -	if ((start < 0x100000) &&
> -	    (mtrr_state.have_fixed) &&
> -	    (mtrr_state.enabled & MTRR_STATE_MTRR_FIXED_ENABLED)) {
> -		int idx;
> -
> -		if (start < 0x80000) {
> -			idx = 0;
> -			idx += (start >> 16);
> -			return mtrr_state.fixed_ranges[idx];
> -		} else if (start < 0xC0000) {
> -			idx = 1 * 8;
> -			idx += ((start - 0x80000) >> 14);
> -			return mtrr_state.fixed_ranges[idx];
> -		} else {
> -			idx = 3 * 8;
> -			idx += ((start - 0xC0000) >> 12);
> -			return mtrr_state.fixed_ranges[idx];
> -		}
> -	}
> -
> -	/*
> -	 * Look in variable ranges
> -	 * Look of multiple ranges matching this address and pick type
> -	 * as per MTRR precedence
> -	 */
>  	prev_match = MTRR_TYPE_INVALID;
>  	for (i = 0; i < num_var_ranges; ++i) {
>  		unsigned short start_state, end_state, inclusive;
> @@ -179,7 +193,8 @@ static u8 __mtrr_type_lookup(u64 start, u64 end, u64 *partial_end, int *repeat)
>  			 * Return the type for first region and a pointer to
>  			 * the start of second region so that caller will
>  			 * lookup again on the second region.
> -			 * Note: This way we handle multiple overlaps as well.
> +			 * Note: This way we handle overlaps with multiple
> +			 * entries and the default type properly.
>  			 */
>  			if (start_state)
>  				*partial_end = base + get_mtrr_size(mask);
> @@ -208,21 +223,18 @@ static u8 __mtrr_type_lookup(u64 start, u64 end, u64 *partial_end, int *repeat)
>  			return curr_match;
>  	}
>  
> -	if (mtrr_tom2) {
> -		if (start >= (1ULL<<32) && (end < mtrr_tom2))
> -			return MTRR_TYPE_WRBACK;
> -	}
> -
>  	if (prev_match != MTRR_TYPE_INVALID)
>  		return prev_match;
>  
>  	return mtrr_state.def_type;
>  }
>  
> -/*
> - * Returns the effective MTRR type for the region
> - * Error return:
> - * MTRR_TYPE_INVALID - when MTRR is not enabled
> +/**
> + * mtrr_type_lookup - look up memory type in MTRR
> + *
> + * Return Values:
> + * MTRR_TYPE_(type)  - The effective MTRR type for the region
> + * MTRR_TYPE_INVALID - MTRR is disabled
>   */
>  u8 mtrr_type_lookup(u64 start, u64 end)
>  {
> @@ -230,22 +242,45 @@ u8 mtrr_type_lookup(u64 start, u64 end)
>  	int repeat;
>  	u64 partial_end;
>  
> -	type = __mtrr_type_lookup(start, end, &partial_end, &repeat);
> +	if (!mtrr_state_set)
> +		return MTRR_TYPE_INVALID;
> +
> +	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.
> +	 */
> +	type = mtrr_type_lookup_fixed(start, end);
> +	if (type != MTRR_TYPE_INVALID)
> +		return type;

Huh, why are we not looking at start?

I mean, fixed MTRRs cover the first 1MB so we can simply do:

        if ((start < 0x100000) &&
            (mtrr_state.have_fixed) &&
            (mtrr_state.enabled & MTRR_STATE_MTRR_FIXED_ENABLED))
		return mtrr_type_lookup_fixed(start, end);

and for all the other ranges we would do the variable lookup:

	type = mtrr_type_lookup_variable(start, end, &partial_end, &repeat);
	...

?

Although I don't know what the code is supposed to do when a region
starts in the fixed range and overlaps its end, i,e, something like
that:

	[ start ... 0x100000 ... end ]

The current code would return a fixed range index and that would be not
really correct.

OTOH, this has been like this forever so maybe we don't care...

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v4 4/7] mtrr, x86: Fix MTRR state checks in mtrr_type_lookup()

[Toshi Kani]

On Wed, 2015-05-06 at 13:47 +0200, Borislav Petkov wrote:
> On Tue, Mar 24, 2015 at 04:08:38PM -0600, Toshi Kani wrote:
> > 'mtrr_state.enabled' contains the FE (fixed MTRRs enabled)
> > and E (MTRRs enabled) flags in MSR_MTRRdefType.  Intel SDM,
> > section 11.11.2.1, defines these flags as follows:
> >  - All MTRRs are disabled when the E flag is clear.
> >    The FE flag has no affect when the E flag is clear.
> >  - The default type is enabled when the E flag is set.
> >  - MTRR variable ranges are enabled when the E flag is set.
> >  - MTRR fixed ranges are enabled when both E and FE flags
> >    are set.
> > 
> > MTRR state checks in __mtrr_type_lookup() do not match with
> > SDM.  Hence, this patch makes the following changes:
> >  - The current code detects MTRRs disabled when both E and
> >    FE flags are clear in mtrr_state.enabled.  Fix to detect
> >    MTRRs disabled when the E flag is clear.
> >  - The current code does not check if the FE bit is set in
> >    mtrr_state.enabled when looking into the fixed entries.
> >    Fix to check the FE flag.
> >  - The current code returns the default type when the E flag
> >    is clear in mtrr_state.enabled.  However, the default type
> >    is also disabled when the E flag is clear.  Fix to remove
> >    the code as this case is handled as MTRR disabled with
> >    the 1st change.
> > 
> > In addition, this patch defines the E and FE flags in
> > mtrr_state.enabled as follows.
> >  - FE flag: MTRR_STATE_MTRR_FIXED_ENABLED
> >  - E  flag: MTRR_STATE_MTRR_ENABLED
> > 
> > print_mtrr_state() is also updated accordingly.
> > 
> > Signed-off-by: Toshi Kani <toshi.kani@hp.com>
> > ---
> >  arch/x86/include/uapi/asm/mtrr.h   |    4 ++++
> >  arch/x86/kernel/cpu/mtrr/generic.c |   15 ++++++++-------
> >  2 files changed, 12 insertions(+), 7 deletions(-)
> 
> You missed a spot in the conversion in
> arch/x86/kernel/cpu/mtrr/cleanup.c::x86_get_mtrr_mem_range():
> 
> There we have
> 
> 	if (base < (1<<(20-PAGE_SHIFT)) && mtrr_state.have_fixed &&
> 	    (mtrr_state.enabled & 1)) {
> 
> which should be mtrr_state.enabled & MTRR_STATE_MTRR_FIXED_ENABLED.

Right.  I will also check both MTRR_STATE_MTRR_FIXED_ENABLED &
MTRR_STATE_MTRR_FIXED_ENABLED bits here.

> > diff --git a/arch/x86/include/uapi/asm/mtrr.h b/arch/x86/include/uapi/asm/mtrr.h
> > index d0acb65..66ba88d 100644
> > --- a/arch/x86/include/uapi/asm/mtrr.h
> > +++ b/arch/x86/include/uapi/asm/mtrr.h
> > @@ -88,6 +88,10 @@ struct mtrr_state_type {
> >         mtrr_type def_type;
> >  };
> > 
> > +/* Bit fields for enabled in struct mtrr_state_type */
> > +#define MTRR_STATE_MTRR_FIXED_ENABLED  0x01
> > +#define MTRR_STATE_MTRR_ENABLED                0x02
> > +
> >  #define MTRRphysBase_MSR(reg) (0x200 + 2 * (reg))
> >  #define MTRRphysMask_MSR(reg) (0x200 + 2 * (reg) + 1)
> 
> Please add those to arch/x86/include/asm/mtrr.h instead. They have no
> place in the uapi header.

I have a question.  Those bits define the bit field of enabled in struct
mtrr_state_type, which is defined in this header.  Is it OK to only move
those definitions to other header?

Thanks,
-Toshi


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v4 6/7] mtrr, x86: Clean up mtrr_type_lookup()

[Toshi Kani]

On Wed, 2015-05-06 at 15:41 +0200, Borislav Petkov wrote:
> On Tue, Mar 24, 2015 at 04:08:40PM -0600, Toshi Kani wrote:
> > MTRRs contain fixed and variable entries.  mtrr_type_lookup()
> > may repeatedly call __mtrr_type_lookup() to handle a request
> > that overlaps with variable entries.  However,
> > __mtrr_type_lookup() also handles the fixed entries, which
> > do not have to be repeated.  Therefore, this patch creates
> > separate functions, mtrr_type_lookup_fixed() and
> > mtrr_type_lookup_variable(), to handle the fixed and variable
> > ranges respectively.
> > 
> > The patch also updates the function headers to clarify the
> > return values and output argument.  It updates comments to
> > clarify that the repeating is necessary to handle overlaps
> > with the default type, since overlaps with multiple entries
> > alone can be handled without such repeating.
> > 
> > There is no functional change in this patch.
> > 
> > Signed-off-by: Toshi Kani <toshi.kani@hp.com>
> > ---
> >  arch/x86/kernel/cpu/mtrr/generic.c |  137 +++++++++++++++++++++++-------------
> >  1 file changed, 86 insertions(+), 51 deletions(-)
> > 
> > diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
> > index 8bd1298..3652e2b 100644
> > --- a/arch/x86/kernel/cpu/mtrr/generic.c
> > +++ b/arch/x86/kernel/cpu/mtrr/generic.c
> > @@ -102,55 +102,69 @@ static int check_type_overlap(u8 *prev, u8 *curr)
> >  	return 0;
> >  }
> >  
> > -/*
> > - * Error/Semi-error returns:
> > - * MTRR_TYPE_INVALID - when MTRR is not enabled
> > - * *repeat == 1 implies [start:end] spanned across MTRR range and type returned
> > - *		corresponds only to [start:*partial_end].
> > - *		Caller has to lookup again for [*partial_end:end].
> > +/**
> > + * mtrr_type_lookup_fixed - look up memory type in MTRR fixed entries
> > + *
> > + * 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
> 
> No need for those - simply a pointer to either the SDM or APM manuals'
> section suffices as they both describe it good.

Ingo asked me to describe this info here in his review...

> > + *
> > + * Return Values:
> > + * MTRR_TYPE_(type)  - Matched memory type
> > + * MTRR_TYPE_INVALID - Unmatched or fixed entries are disabled
> >   */
> > -static u8 __mtrr_type_lookup(u64 start, u64 end, u64 *partial_end, int *repeat)
> > +static u8 mtrr_type_lookup_fixed(u64 start, u64 end)
> > +{
> > +	int idx;
> > +
> > +	if (start >= 0x100000)
> > +		return MTRR_TYPE_INVALID;
> > +
> > +	if (!(mtrr_state.have_fixed) ||
> > +	    !(mtrr_state.enabled & MTRR_STATE_MTRR_FIXED_ENABLED))
> > +		return MTRR_TYPE_INVALID;
> > +
> > +	if (start < 0x80000) {		/* 0x0 - 0x7FFFF */
> > +		idx = 0;
> > +		idx += (start >> 16);
> > +		return mtrr_state.fixed_ranges[idx];
> > +
> > +	} else if (start < 0xC0000) {	/* 0x80000 - 0xBFFFF */
> > +		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 Argument:
> > + * 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].
> > + */
> > +static u8 mtrr_type_lookup_variable(u64 start, u64 end, u64 *partial_end,
> > +				    int *repeat)
> >  {
> >  	int i;
> >  	u64 base, mask;
> >  	u8 prev_match, curr_match;
> >  
> >  	*repeat = 0;
> > -	if (!mtrr_state_set)
> > -		return MTRR_TYPE_INVALID;
> > -
> > -	if (!(mtrr_state.enabled & MTRR_STATE_MTRR_ENABLED))
> > -		return MTRR_TYPE_INVALID;
> >  
> >  	/* Make end inclusive end, instead of exclusive */
> >  	end--;
> >  
> > -	/* Look in fixed ranges. Just return the type as per start */
> > -	if ((start < 0x100000) &&
> > -	    (mtrr_state.have_fixed) &&
> > -	    (mtrr_state.enabled & MTRR_STATE_MTRR_FIXED_ENABLED)) {
> > -		int idx;
> > -
> > -		if (start < 0x80000) {
> > -			idx = 0;
> > -			idx += (start >> 16);
> > -			return mtrr_state.fixed_ranges[idx];
> > -		} else if (start < 0xC0000) {
> > -			idx = 1 * 8;
> > -			idx += ((start - 0x80000) >> 14);
> > -			return mtrr_state.fixed_ranges[idx];
> > -		} else {
> > -			idx = 3 * 8;
> > -			idx += ((start - 0xC0000) >> 12);
> > -			return mtrr_state.fixed_ranges[idx];
> > -		}
> > -	}
> > -
> > -	/*
> > -	 * Look in variable ranges
> > -	 * Look of multiple ranges matching this address and pick type
> > -	 * as per MTRR precedence
> > -	 */
> >  	prev_match = MTRR_TYPE_INVALID;
> >  	for (i = 0; i < num_var_ranges; ++i) {
> >  		unsigned short start_state, end_state, inclusive;
> > @@ -179,7 +193,8 @@ static u8 __mtrr_type_lookup(u64 start, u64 end, u64 *partial_end, int *repeat)
> >  			 * Return the type for first region and a pointer to
> >  			 * the start of second region so that caller will
> >  			 * lookup again on the second region.
> > -			 * Note: This way we handle multiple overlaps as well.
> > +			 * Note: This way we handle overlaps with multiple
> > +			 * entries and the default type properly.
> >  			 */
> >  			if (start_state)
> >  				*partial_end = base + get_mtrr_size(mask);
> > @@ -208,21 +223,18 @@ static u8 __mtrr_type_lookup(u64 start, u64 end, u64 *partial_end, int *repeat)
> >  			return curr_match;
> >  	}
> >  
> > -	if (mtrr_tom2) {
> > -		if (start >= (1ULL<<32) && (end < mtrr_tom2))
> > -			return MTRR_TYPE_WRBACK;
> > -	}
> > -
> >  	if (prev_match != MTRR_TYPE_INVALID)
> >  		return prev_match;
> >  
> >  	return mtrr_state.def_type;
> >  }
> >  
> > -/*
> > - * Returns the effective MTRR type for the region
> > - * Error return:
> > - * MTRR_TYPE_INVALID - when MTRR is not enabled
> > +/**
> > + * mtrr_type_lookup - look up memory type in MTRR
> > + *
> > + * Return Values:
> > + * MTRR_TYPE_(type)  - The effective MTRR type for the region
> > + * MTRR_TYPE_INVALID - MTRR is disabled
> >   */
> >  u8 mtrr_type_lookup(u64 start, u64 end)
> >  {
> > @@ -230,22 +242,45 @@ u8 mtrr_type_lookup(u64 start, u64 end)
> >  	int repeat;
> >  	u64 partial_end;
> >  
> > -	type = __mtrr_type_lookup(start, end, &partial_end, &repeat);
> > +	if (!mtrr_state_set)
> > +		return MTRR_TYPE_INVALID;
> > +
> > +	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.
> > +	 */
> > +	type = mtrr_type_lookup_fixed(start, end);
> > +	if (type != MTRR_TYPE_INVALID)
> > +		return type;
> 
> Huh, why are we not looking at start?
> 
> I mean, fixed MTRRs cover the first 1MB so we can simply do:
> 
>         if ((start < 0x100000) &&
>             (mtrr_state.have_fixed) &&
>             (mtrr_state.enabled & MTRR_STATE_MTRR_FIXED_ENABLED))
> 		return mtrr_type_lookup_fixed(start, end);

mtrr_type_lookup_fixed() checks the above conditions at entry, and
returns immediately with TYPE_INVALID.  I think it is safer to have such
checks in mtrr_type_lookup_fixed() in case there will be multiple
callers.

> and for all the other ranges we would do the variable lookup:
> 
> 	type = mtrr_type_lookup_variable(start, end, &partial_end, &repeat);
> 	...
> 
> ?
> 
> Although I don't know what the code is supposed to do when a region
> starts in the fixed range and overlaps its end, i,e, something like
> that:
> 
> 	[ start ... 0x100000 ... end ]
> 
> The current code would return a fixed range index and that would be not
> really correct.
> 
> OTOH, this has been like this forever so maybe we don't care...

Right, and there is more.  As the original code had comment "Just return
the type as per start", which I noticed that I had accidentally removed,
the code only returns the type of the start address.  The fixed ranges
have multiple entries with different types.  Hence, a given range may
overlap with multiple fixed entries.  I will restore the comment in the
function header to clarify this limitation.

Thanks,
-Toshi


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v4 4/7] mtrr, x86: Fix MTRR state checks in mtrr_type_lookup()

[Borislav Petkov]

On Wed, May 06, 2015 at 09:23:31AM -0600, Toshi Kani wrote:
> I have a question.  Those bits define the bit field of enabled in struct
> mtrr_state_type, which is defined in this header.  Is it OK to only move
> those definitions to other header?

I think we shouldn't expose stuff to userspace if we don't have to
because then we're stuck with it. Userspace has managed so far without
those defines so I don't see why we should export them now.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v4 6/7] mtrr, x86: Clean up mtrr_type_lookup()

[Borislav Petkov]

On Wed, May 06, 2015 at 10:00:30AM -0600, Toshi Kani wrote:
> Ingo asked me to describe this info here in his review...

Ok.

> mtrr_type_lookup_fixed() checks the above conditions at entry, and
> returns immediately with TYPE_INVALID.  I think it is safer to have such
> checks in mtrr_type_lookup_fixed() in case there will be multiple
> callers.

This is not what I mean - I mean to call mtrr_type_lookup_fixed() based
on @start and not unconditionally, like you do.

And there most likely won't be multiple callers because we're phasing
out MTRR use.

And even if there are, they better look at how this function is being
called before calling it. Which I seriously doubt - it is a static
function which you *just* came up with.

> Right, and there is more.  As the original code had comment "Just return
> the type as per start", which I noticed that I had accidentally removed,
> the code only returns the type of the start address.  The fixed ranges
> have multiple entries with different types.  Hence, a given range may
> overlap with multiple fixed entries.  I will restore the comment in the
> function header to clarify this limitation.

Ok, let's cleanup this function first and then consider fixing other
possible bugs which haven't been fixed since forever. Again, we might
not even need to address them because we won't be using MTRRs once we
switch to PAT completely, which is what Luis is working on.

Thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v4 4/7] mtrr, x86: Fix MTRR state checks in mtrr_type_lookup()

[Toshi Kani]

On Thu, 2015-05-07 at 00:39 +0200, Borislav Petkov wrote:
> On Wed, May 06, 2015 at 09:23:31AM -0600, Toshi Kani wrote:
> > I have a question.  Those bits define the bit field of enabled in struct
> > mtrr_state_type, which is defined in this header.  Is it OK to only move
> > those definitions to other header?
> 
> I think we shouldn't expose stuff to userspace if we don't have to
> because then we're stuck with it. Userspace has managed so far without
> those defines so I don't see why we should export them now.

OK, I will move those bits definition to arch/x86/include/asm/mtrr.h.

Thanks for the clarification,
-Toshi


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v4 6/7] mtrr, x86: Clean up mtrr_type_lookup()

[Toshi Kani]

On Thu, 2015-05-07 at 00:49 +0200, Borislav Petkov wrote:
> On Wed, May 06, 2015 at 10:00:30AM -0600, Toshi Kani wrote:
> > Ingo asked me to describe this info here in his review...
> 
> Ok.
> 
> > mtrr_type_lookup_fixed() checks the above conditions at entry, and
> > returns immediately with TYPE_INVALID.  I think it is safer to have such
> > checks in mtrr_type_lookup_fixed() in case there will be multiple
> > callers.
> 
> This is not what I mean - I mean to call mtrr_type_lookup_fixed() based
> on @start and not unconditionally, like you do.
> 
> And there most likely won't be multiple callers because we're phasing
> out MTRR use.
> 
> And even if there are, they better look at how this function is being
> called before calling it. Which I seriously doubt - it is a static
> function which you *just* came up with.

Well, creating mtrr_type_lookup_fixed() is one of the comments I had in
the previous code review.  Anyway, let me make sure if I understand your
comment correctly.  Do the following changes look right to you?

1) Change the caller responsible for the condition checks.

        if ((start < 0x100000) &&
            (mtrr_state.have_fixed) &&
            (mtrr_state.enabled & MTRR_STATE_MTRR_FIXED_ENABLED))
                return mtrr_type_lookup_fixed(start, end);

2) Delete the checks with mtrr_state in mtrr_type_lookup_fixed() as they
are done by the caller.  Keep the check with '(start >= 0x100000)' to
assure that the code handles the range [0xC0000 - 0xFFFFF] correctly.

static u8 mtrr_type_lookup_fixed(u64 start, u64 end)
{
        int idx;

        if (start >= 0x100000)
                 return MTRR_TYPE_INVALID;
 
-       if (!(mtrr_state.have_fixed) ||
-           !(mtrr_state.enabled & MTRR_STATE_MTRR_FIXED_ENABLED))
-               return MTRR_TYPE_INVALID;


> > Right, and there is more.  As the original code had comment "Just return
> > the type as per start", which I noticed that I had accidentally removed,
> > the code only returns the type of the start address.  The fixed ranges
> > have multiple entries with different types.  Hence, a given range may
> > overlap with multiple fixed entries.  I will restore the comment in the
> > function header to clarify this limitation.
> 
> Ok, let's cleanup this function first and then consider fixing other
> possible bugs which haven't been fixed since forever. Again, we might
> not even need to address them because we won't be using MTRRs once we
> switch to PAT completely, which is what Luis is working on.

Right.

Thanks,
-Toshi

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v4 6/7] mtrr, x86: Clean up mtrr_type_lookup()

[Borislav Petkov]

On Wed, May 06, 2015 at 05:42:10PM -0600, Toshi Kani wrote:
> Well, creating mtrr_type_lookup_fixed() is one of the comments I had in
> the previous code review.  Anyway, let me make sure if I understand your
> comment correctly.  Do the following changes look right to you?
> 
> 1) Change the caller responsible for the condition checks.
> 
>         if ((start < 0x100000) &&
>             (mtrr_state.have_fixed) &&
>             (mtrr_state.enabled & MTRR_STATE_MTRR_FIXED_ENABLED))
>                 return mtrr_type_lookup_fixed(start, end);
> 
> 2) Delete the checks with mtrr_state in mtrr_type_lookup_fixed() as they
> are done by the caller.  Keep the check with '(start >= 0x100000)' to
> assure that the code handles the range [0xC0000 - 0xFFFFF] correctly.

That is a good defensive measure.

> static u8 mtrr_type_lookup_fixed(u64 start, u64 end)
> {
>         int idx;
> 
>         if (start >= 0x100000)
>                  return MTRR_TYPE_INVALID;
>  
> -       if (!(mtrr_state.have_fixed) ||
> -           !(mtrr_state.enabled & MTRR_STATE_MTRR_FIXED_ENABLED))
> -               return MTRR_TYPE_INVALID;

Yeah, that's what I mean.

Thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v4 6/7] mtrr, x86: Clean up mtrr_type_lookup()

[Toshi Kani]

On Thu, 2015-05-07 at 09:52 +0200, Borislav Petkov wrote:
> On Wed, May 06, 2015 at 05:42:10PM -0600, Toshi Kani wrote:
> > Well, creating mtrr_type_lookup_fixed() is one of the comments I had in
> > the previous code review.  Anyway, let me make sure if I understand your
> > comment correctly.  Do the following changes look right to you?
> > 
> > 1) Change the caller responsible for the condition checks.
> > 
> >         if ((start < 0x100000) &&
> >             (mtrr_state.have_fixed) &&
> >             (mtrr_state.enabled & MTRR_STATE_MTRR_FIXED_ENABLED))
> >                 return mtrr_type_lookup_fixed(start, end);
> > 
> > 2) Delete the checks with mtrr_state in mtrr_type_lookup_fixed() as they
> > are done by the caller.  Keep the check with '(start >= 0x100000)' to
> > assure that the code handles the range [0xC0000 - 0xFFFFF] correctly.
> 
> That is a good defensive measure.
> 
> > static u8 mtrr_type_lookup_fixed(u64 start, u64 end)
> > {
> >         int idx;
> > 
> >         if (start >= 0x100000)
> >                  return MTRR_TYPE_INVALID;
> >  
> > -       if (!(mtrr_state.have_fixed) ||
> > -           !(mtrr_state.enabled & MTRR_STATE_MTRR_FIXED_ENABLED))
> > -               return MTRR_TYPE_INVALID;
> 
> Yeah, that's what I mean.

Thanks for the clarification! Will change accordingly.
-Toshi

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v4 7/7] mtrr, mm, x86: Enhance MTRR checks for KVA huge page mapping

[Borislav Petkov]

On Tue, Mar 24, 2015 at 04:08:41PM -0600, Toshi Kani wrote:
> This patch adds an additional argument, 'uniform', to
> mtrr_type_lookup(), which returns 1 when a given range is
> covered uniformly by MTRRs, i.e. the range is fully covered
> by a single MTRR entry or the default type.
> 
> pud_set_huge() and pmd_set_huge() are changed to check the
> new 'uniform' flag to see if it is safe to create a huge page
> mapping to the range.  This allows them to create a huge page
> mapping to a range covered by a single MTRR entry of any
> memory type.  It also detects a non-optimal request properly.
> They continue to check with the WB type since the WB type has
> no effect even if a request spans multiple MTRR entries.
> 
> pmd_set_huge() logs a warning message to a non-optimal request
> so that driver writers will be aware of such a case.  Drivers
> should make a mapping request aligned to a single MTRR entry
> when the range is covered by MTRRs.
> 
> Signed-off-by: Toshi Kani <toshi.kani@hp.com>
> ---
>  arch/x86/include/asm/mtrr.h        |    5 +++--
>  arch/x86/kernel/cpu/mtrr/generic.c |   35 +++++++++++++++++++++++++++--------
>  arch/x86/mm/pat.c                  |    4 ++--
>  arch/x86/mm/pgtable.c              |   25 +++++++++++++++----------
>  4 files changed, 47 insertions(+), 22 deletions(-)

...

> @@ -235,13 +240,19 @@ static u8 mtrr_type_lookup_variable(u64 start, u64 end, u64 *partial_end,
>   * Return Values:
>   * MTRR_TYPE_(type)  - The effective MTRR type for the region
>   * MTRR_TYPE_INVALID - MTRR is disabled
> + *
> + * Output Argument:
> + * uniform - Set to 1 when MTRR covers the region uniformly, i.e. the region
> + *	     is fully covered by a single MTRR entry or the default type.

I'd call this "single_mtrr". "uniform" could also mean that the resulting
type is uniform, i.e. of the same type but spanning multiple MTRRs.

>   */
> -u8 mtrr_type_lookup(u64 start, u64 end)
> +u8 mtrr_type_lookup(u64 start, u64 end, u8 *uniform)
>  {
> -	u8 type, prev_type;
> +	u8 type, prev_type, is_uniform, dummy;
>  	int repeat;
>  	u64 partial_end;
>  
> +	*uniform = 1;
> +

You're setting it here...

>  	if (!mtrr_state_set)
>  		return MTRR_TYPE_INVALID;

... but if you return here, you would've changed the thing uniform
points to needlessly as you're returning an error.

> @@ -253,14 +264,17 @@ u8 mtrr_type_lookup(u64 start, u64 end)
>  	 * the variable ranges.
>  	 */
>  	type = mtrr_type_lookup_fixed(start, end);
> -	if (type != MTRR_TYPE_INVALID)
> +	if (type != MTRR_TYPE_INVALID) {
> +		*uniform = 0;
>  		return type;
> +	}
>  
>  	/*
>  	 * 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);
> +	type = mtrr_type_lookup_variable(start, end, &partial_end,
> +					 &repeat, &is_uniform);
>  
>  	/*
>  	 * Common path is with repeat = 0.
> @@ -271,16 +285,21 @@ u8 mtrr_type_lookup(u64 start, u64 end)
>  	while (repeat) {
>  		prev_type = type;
>  		start = partial_end;
> +		is_uniform = 0;

So I think it would be better if you added an out: label where you do
exit from the function and set return values there.

So something like that, I'm pasting the whole function here so that you
can follow better:

u8 mtrr_type_lookup(u64 start, u64 end, u8 *uniform)
{
        u8 type, prev_type, is_uniform = 1, dummy;
        int repeat;
        u64 partial_end;

        if (!mtrr_state_set)
                return MTRR_TYPE_INVALID;

        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.
         */
        type = mtrr_type_lookup_fixed(start, end);
        if (type != MTRR_TYPE_INVALID) {
                is_uniform = 0;
                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);

        /*
         * 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;

        }

        if (mtrr_tom2 && (start >= (1ULL<<32)) && (end < mtrr_tom2))
                type = MTRR_TYPE_WRBACK;

out:
        *uniform = is_uniform;
        return type;
}
---

This way you're setting the uniform pointer in a single location and you're
working with the local variable inside the function.

Much easier to follow.

> +
>  		type = mtrr_type_lookup_variable(start, end, &partial_end,
> -						 &repeat);
> +						 &repeat, &dummy);
>  
> -		if (check_type_overlap(&prev_type, &type))
> +		if (check_type_overlap(&prev_type, &type)) {
> +			*uniform = 0;
>  			return type;
> +		}
>  	}
>  
>  	if (mtrr_tom2 && (start >= (1ULL<<32)) && (end < mtrr_tom2))
>  		return MTRR_TYPE_WRBACK;
>  
> +	*uniform = is_uniform;
>  	return type;
>  }
>  
> diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c
> index 35af677..372ad42 100644
> --- a/arch/x86/mm/pat.c
> +++ b/arch/x86/mm/pat.c
> @@ -267,9 +267,9 @@ static unsigned long pat_x_mtrr_type(u64 start, u64 end,
>  	 * request is for WB.
>  	 */
>  	if (req_type == _PAGE_CACHE_MODE_WB) {
> -		u8 mtrr_type;
> +		u8 mtrr_type, uniform;
>  
> -		mtrr_type = mtrr_type_lookup(start, end);
> +		mtrr_type = mtrr_type_lookup(start, end, &uniform);
>  		if (mtrr_type != MTRR_TYPE_WRBACK)
>  			return _PAGE_CACHE_MODE_UC_MINUS;
>  
> diff --git a/arch/x86/mm/pgtable.c b/arch/x86/mm/pgtable.c
> index cfca4cf..3d6edea 100644
> --- a/arch/x86/mm/pgtable.c
> +++ b/arch/x86/mm/pgtable.c
> @@ -567,17 +567,18 @@ void native_set_fixmap(enum fixed_addresses idx, phys_addr_t phys,
>   * pud_set_huge - setup kernel PUD mapping
>   *
>   * MTRR can override PAT memory types with 4KB granularity.  Therefore,
> - * it does not set up a huge page when the range is covered by a non-WB
> - * type of MTRR.  MTRR_TYPE_INVALID indicates that MTRR are disabled.
> + * it only sets up a huge page when the range is mapped uniformly by MTRR
> + * (i.e. the range is fully covered by a single MTRR entry or the default
> + * type) or the MTRR memory type is WB.
>   *
>   * Return 1 on success, and 0 when no PUD was set.
>   */
>  int pud_set_huge(pud_t *pud, phys_addr_t addr, pgprot_t prot)
>  {
> -	u8 mtrr;
> +	u8 mtrr, uniform;
>  
> -	mtrr = mtrr_type_lookup(addr, addr + PUD_SIZE);
> -	if ((mtrr != MTRR_TYPE_WRBACK) && (mtrr != MTRR_TYPE_INVALID))
> +	mtrr = mtrr_type_lookup(addr, addr + PUD_SIZE, &uniform);
> +	if ((!uniform) && (mtrr != MTRR_TYPE_WRBACK))
>  		return 0;
>  
>  	prot = pgprot_4k_2_large(prot);
> @@ -593,18 +594,22 @@ int pud_set_huge(pud_t *pud, phys_addr_t addr, pgprot_t prot)
>   * pmd_set_huge - setup kernel PMD mapping
>   *
>   * MTRR can override PAT memory types with 4KB granularity.  Therefore,
> - * it does not set up a huge page when the range is covered by a non-WB
> - * type of MTRR.  MTRR_TYPE_INVALID indicates that MTRR are disabled.
> + * it only sets up a huge page when the range is mapped uniformly by MTRR
> + * (i.e. the range is fully covered by a single MTRR entry or the default
> + * type) or the MTRR memory type is WB.
>   *
>   * Return 1 on success, and 0 when no PMD was set.
>   */
>  int pmd_set_huge(pmd_t *pmd, phys_addr_t addr, pgprot_t prot)
>  {
> -	u8 mtrr;
> +	u8 mtrr, uniform;
>  
> -	mtrr = mtrr_type_lookup(addr, addr + PMD_SIZE);
> -	if ((mtrr != MTRR_TYPE_WRBACK) && (mtrr != MTRR_TYPE_INVALID))
> +	mtrr = mtrr_type_lookup(addr, addr + PMD_SIZE, &uniform);
> +	if ((!uniform) && (mtrr != MTRR_TYPE_WRBACK)) {
> +		pr_warn("pmd_set_huge: requesting [mem %#010llx-%#010llx], which spans more than a single MTRR entry\n",
> +				addr, addr + PMD_SIZE);
>  		return 0;

So this returns 0, i.e. failure already. Why do we even have to warn?
Caller already knows it failed.

And this warning would flood dmesg needlessly.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[tip:x86/mm] x86/mm/mtrr: Remove incorrect address check in __mtrr_type_lookup()

[tip-bot for Toshi Kani]

Commit-ID:  cd2f6a5a4704a359635eb34919317052e6a96ba7
Gitweb:     http://git.kernel.org/tip/cd2f6a5a4704a359635eb34919317052e6a96ba7
Author:     Toshi Kani <toshi.kani@hp.com>
AuthorDate: Mon, 11 May 2015 10:15:52 +0200
Committer:  Ingo Molnar <mingo@kernel.org>
CommitDate: Mon, 11 May 2015 10:38:44 +0200

x86/mm/mtrr: Remove incorrect address check in __mtrr_type_lookup()

__mtrr_type_lookup() checks MTRR fixed ranges when mtrr_state.have_fixed
is set and start is less than 0x100000.

However, the 'else if (start < 0x1000000)' in the code checks with an
incorrect address as it has an extra-zero in the address.

The code still runs correctly as this check is meaningless, though.

This patch replaces the incorrect address check with 'else' with no
condition.

Signed-off-by: Toshi Kani <toshi.kani@hp.com>
Signed-off-by: Borislav Petkov <bp@suse.de>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Andy Lutomirski <luto@amacapital.net>
Cc: Borislav Petkov <bp@alien8.de>
Cc: Brian Gerst <brgerst@gmail.com>
Cc: Denys Vlasenko <dvlasenk@redhat.com>
Cc: Elliott@hp.com
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Luis R. Rodriguez <mcgrof@suse.com>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: dave.hansen@intel.com
Cc: linux-mm <linux-mm@kvack.org>
Cc: pebolle@tiscali.nl
Link: http://lkml.kernel.org/r/1427234921-19737-4-git-send-email-toshi.kani@hp.com
Link: http://lkml.kernel.org/r/1431332153-18566-8-git-send-email-bp@alien8.de
Signed-off-by: Ingo Molnar <mingo@kernel.org>
---
 arch/x86/kernel/cpu/mtrr/generic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c
index 7d74f7b..5b23967 100644
--- a/arch/x86/kernel/cpu/mtrr/generic.c
+++ b/arch/x86/kernel/cpu/mtrr/generic.c
@@ -137,7 +137,7 @@ static u8 __mtrr_type_lookup(u64 start, u64 end, u64 *partial_end, int *repeat)
 			idx = 1 * 8;
 			idx += ((start - 0x80000) >> 14);
 			return mtrr_state.fixed_ranges[idx];
-		} else if (start < 0x1000000) {
+		} else {
 			idx = 3 * 8;
 			idx += ((start - 0xC0000) >> 12);
 			return mtrr_state.fixed_ranges[idx];

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v4 7/7] mtrr, mm, x86: Enhance MTRR checks for KVA huge page mapping

[Toshi Kani]

On Sat, 2015-05-09 at 11:08 +0200, Borislav Petkov wrote:
> On Tue, Mar 24, 2015 at 04:08:41PM -0600, Toshi Kani wrote:
 :
> > @@ -235,13 +240,19 @@ static u8 mtrr_type_lookup_variable(u64 start, u64 end, u64 *partial_end,
> >   * Return Values:
> >   * MTRR_TYPE_(type)  - The effective MTRR type for the region
> >   * MTRR_TYPE_INVALID - MTRR is disabled
> > + *
> > + * Output Argument:
> > + * uniform - Set to 1 when MTRR covers the region uniformly, i.e. the region
> > + *	     is fully covered by a single MTRR entry or the default type.
> 
> I'd call this "single_mtrr". "uniform" could also mean that the resulting
> type is uniform, i.e. of the same type but spanning multiple MTRRs.

Actually, that is the intend of "uniform" and the same type but spanning
multiple MTRRs should set "uniform" to 1.  The patch does not check such
case for simplicity since we do not need to maximize the performance
with MTRRs for every corner case since they are legacy and their use is
expected to be phased out.  It makes sure that a type conflict with
MTRRs is detected so that huge page mappings are made safely.

Also, in most of the cases, "uniform" is set to 1 because there is no
MTRR entry that covers the range, i.e. the default type.


> >   */
> > -u8 mtrr_type_lookup(u64 start, u64 end)
> > +u8 mtrr_type_lookup(u64 start, u64 end, u8 *uniform)
> >  {
> > -	u8 type, prev_type;
> > +	u8 type, prev_type, is_uniform, dummy;
> >  	int repeat;
> >  	u64 partial_end;
> >  
> > +	*uniform = 1;
> > +
> 
> You're setting it here...
> 
> >  	if (!mtrr_state_set)
> >  		return MTRR_TYPE_INVALID;
> 
> ... but if you return here, you would've changed the thing uniform
> points to needlessly as you're returning an error.

We need to set "uniform" to 1 when MTRRs are disabled since there is no
type conflict with MTRRs. 


> > @@ -253,14 +264,17 @@ u8 mtrr_type_lookup(u64 start, u64 end)
> >  	 * the variable ranges.
> >  	 */
> >  	type = mtrr_type_lookup_fixed(start, end);
> > -	if (type != MTRR_TYPE_INVALID)
> > +	if (type != MTRR_TYPE_INVALID) {
> > +		*uniform = 0;
> >  		return type;
> > +	}
> >  
> >  	/*
> >  	 * 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);
> > +	type = mtrr_type_lookup_variable(start, end, &partial_end,
> > +					 &repeat, &is_uniform);
> >  
> >  	/*
> >  	 * Common path is with repeat = 0.
> > @@ -271,16 +285,21 @@ u8 mtrr_type_lookup(u64 start, u64 end)
> >  	while (repeat) {
> >  		prev_type = type;
> >  		start = partial_end;
> > +		is_uniform = 0;
> 
> So I think it would be better if you added an out: label where you do
> exit from the function and set return values there.
> 
> So something like that, I'm pasting the whole function here so that you
> can follow better:
 :
> 
> This way you're setting the uniform pointer in a single location and you're
> working with the local variable inside the function.
> 
> Much easier to follow.

With the label, the above check will be:

        if (!mtrr_state_set) {
		is_uniform = 1;
                type = MTRR_TYPE_INVALID;
		goto out;
	}

I can follow your suggestion of using the label if you still prefer
using it.


> >   */
> >  int pmd_set_huge(pmd_t *pmd, phys_addr_t addr, pgprot_t prot)
> >  {
> > -	u8 mtrr;
> > +	u8 mtrr, uniform;
> >  
> > -	mtrr = mtrr_type_lookup(addr, addr + PMD_SIZE);
> > -	if ((mtrr != MTRR_TYPE_WRBACK) && (mtrr != MTRR_TYPE_INVALID))
> > +	mtrr = mtrr_type_lookup(addr, addr + PMD_SIZE, &uniform);
> > +	if ((!uniform) && (mtrr != MTRR_TYPE_WRBACK)) {
> > +		pr_warn("pmd_set_huge: requesting [mem %#010llx-%#010llx], which spans more than a single MTRR entry\n",
> > +				addr, addr + PMD_SIZE);
> >  		return 0;
> 
> So this returns 0, i.e. failure already. Why do we even have to warn?
> Caller already knows it failed.
> 
> And this warning would flood dmesg needlessly.

The warning was suggested by reviewers in the previous review so that
driver writers will notice the issue.  Returning 0 here will lead
ioremap() to use 4KB mappings, but does not cause ioremap() to fail.

Thanks,
-Toshi


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v4 7/7] mtrr, mm, x86: Enhance MTRR checks for KVA huge page mapping

[Borislav Petkov]

On Mon, May 11, 2015 at 01:25:16PM -0600, Toshi Kani wrote:
> > > @@ -235,13 +240,19 @@ static u8 mtrr_type_lookup_variable(u64 start, u64 end, u64 *partial_end,
> > >   * Return Values:
> > >   * MTRR_TYPE_(type)  - The effective MTRR type for the region
> > >   * MTRR_TYPE_INVALID - MTRR is disabled
> > > + *
> > > + * Output Argument:
> > > + * uniform - Set to 1 when MTRR covers the region uniformly, i.e. the region
> > > + *	     is fully covered by a single MTRR entry or the default type.
> > 
> > I'd call this "single_mtrr". "uniform" could also mean that the resulting
> > type is uniform, i.e. of the same type but spanning multiple MTRRs.
> 
> Actually, that is the intend of "uniform" and the same type but spanning
> multiple MTRRs should set "uniform" to 1.  The patch does not check such

So why does it say "is fully covered by a single MTRR entry or the
default type." - the stress being on *single*

You need to make up your mind.

> We need to set "uniform" to 1 when MTRRs are disabled since there is no
> type conflict with MTRRs.

No, this is wrong.

When we return an *error*, "uniform" should be *undefined* because MTRRs
are disabled and callers should be checking whether it returned an error
first and only *then* look at uniform.

> The warning was suggested by reviewers in the previous review so that
> driver writers will notice the issue.

No, we don't flood dmesg so that driver writers notice stuff. We better
fix the callers.

> Returning 0 here will lead
> ioremap() to use 4KB mappings, but does not cause ioremap() to fail.

I guess a pr_warn_once() should be better then. Flooding dmesg with
error messages for which the user can't really do anything about doesn't
bring us anything.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v4 7/7] mtrr, mm, x86: Enhance MTRR checks for KVA huge page mapping

[Toshi Kani]

On Mon, 2015-05-11 at 22:18 +0200, Borislav Petkov wrote:
> On Mon, May 11, 2015 at 01:25:16PM -0600, Toshi Kani wrote:
> > > > @@ -235,13 +240,19 @@ static u8 mtrr_type_lookup_variable(u64 start, u64 end, u64 *partial_end,
> > > >   * Return Values:
> > > >   * MTRR_TYPE_(type)  - The effective MTRR type for the region
> > > >   * MTRR_TYPE_INVALID - MTRR is disabled
> > > > + *
> > > > + * Output Argument:
> > > > + * uniform - Set to 1 when MTRR covers the region uniformly, i.e. the region
> > > > + *	     is fully covered by a single MTRR entry or the default type.
> > > 
> > > I'd call this "single_mtrr". "uniform" could also mean that the resulting
> > > type is uniform, i.e. of the same type but spanning multiple MTRRs.
> > 
> > Actually, that is the intend of "uniform" and the same type but spanning
> > multiple MTRRs should set "uniform" to 1.  The patch does not check such
> 
> So why does it say "is fully covered by a single MTRR entry or the
> default type." - the stress being on *single*
> 
> You need to make up your mind.

I will clarify the comment as follows.
===
uniform - Set to 1 when the region is not covered with multiple memory
types by MTRRs.  It is set for any return value.

NOTE: The current code sets 'uniform' to 1 when the region is fully
covered by a single MTRR entry or fully uncovered.  However, it does not
detect a uniform case that the region is covered by the same type but
spanning multiple MTRR entries for simplicity.
===

> > We need to set "uniform" to 1 when MTRRs are disabled since there is no
> > type conflict with MTRRs.
> 
> No, this is wrong.
> 
> When we return an *error*, "uniform" should be *undefined* because MTRRs
> are disabled and callers should be checking whether it returned an error
> first and only *then* look at uniform.

MTRRs disabled is not an error case as it could be a normal
configuration on some platforms / BIOS setups.  I clarified it in the
above comment that uniform is set for any return value.


> > The warning was suggested by reviewers in the previous review so that
> > driver writers will notice the issue.
> 
> No, we don't flood dmesg so that driver writers notice stuff. We better
> fix the callers.
> 
> > Returning 0 here will lead
> > ioremap() to use 4KB mappings, but does not cause ioremap() to fail.
> 
> I guess a pr_warn_once() should be better then. Flooding dmesg with
> error messages for which the user can't really do anything about doesn't
> bring us anything.

OK, I will change it to pr_warn_once().

Thanks,
-Toshi


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v4 7/7] mtrr, mm, x86: Enhance MTRR checks for KVA huge page mapping

[Borislav Petkov]

On Mon, May 11, 2015 at 02:38:46PM -0600, Toshi Kani wrote:
> MTRRs disabled is not an error case as it could be a normal
> configuration on some platforms / BIOS setups.

Normal how? PAT-only systems? Examples please...

> I clarified it in the above comment that uniform is set for any return
> value.

Hell no!

u8 mtrr_type_lookup(u64 start, u64 end, u8 *uniform)
{

	...

        *uniform = 1;

        if (!mtrr_state_set)
                return MTRR_TYPE_INVALID;

        if (!(mtrr_state.enabled & MTRR_STATE_MTRR_ENABLED))
                return MTRR_TYPE_INVALID;


This is wrong and the fact that I still need to persuade you about it
says a lot.

If you want to be able to state that a type is uniform even if MTRRs are
disabled, you need to define another retval which means exactly that.

Or add an inline function called mtrr_enabled() and call it in the
mtrr_type_lookup() callers.

Or whatever.

I don't want any confusing states with two return types and people
having to figure out what it exactly means and digging into the code
and scratching heads WTF is that supposed to mean.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v4 7/7] mtrr, mm, x86: Enhance MTRR checks for KVA huge page mapping

[Toshi Kani]

On Mon, 2015-05-11 at 23:42 +0200, Borislav Petkov wrote:
> On Mon, May 11, 2015 at 02:38:46PM -0600, Toshi Kani wrote:
> > MTRRs disabled is not an error case as it could be a normal
> > configuration on some platforms / BIOS setups.
> 
> Normal how? PAT-only systems? Examples please...

BIOS initializes and enables MTRRs at POST.  While the most (if not all)
BIOSes do it today, I do not think the x86 arch requires BIOS to enable
them.

Here is a quote from Intel SDM:
===
11.11.5 MTRR Initialization

On a hardware reset, the P6 and more recent processors clear the valid
flags in variable-range MTRRs and clear the E flag in the
IA32_MTRR_DEF_TYPE MSR to disable all MTRRs. All other bits in the MTRRs
are undefined.

Prior to initializing the MTRRs, software (normally the system BIOS)
must initialize all fixed-range and variablerange MTRR register fields
to 0. Software can then initialize the MTRRs according to known types of
memory, including memory on devices that it auto-configures.
Initialization is expected to occur prior to booting the operating
system.
===

> > I clarified it in the above comment that uniform is set for any return
> > value.
> 
> Hell no!
> 
> u8 mtrr_type_lookup(u64 start, u64 end, u8 *uniform)
> {
> 
> 	...
> 
>         *uniform = 1;
> 
>         if (!mtrr_state_set)
>                 return MTRR_TYPE_INVALID;
> 
>         if (!(mtrr_state.enabled & MTRR_STATE_MTRR_ENABLED))
>                 return MTRR_TYPE_INVALID;
> 
> 
> This is wrong and the fact that I still need to persuade you about it
> says a lot.
> 
> If you want to be able to state that a type is uniform even if MTRRs are
> disabled, you need to define another retval which means exactly that.

There may not be any type conflict with MTRR_TYPE_INVALID. 

> Or add an inline function called mtrr_enabled() and call it in the
> mtrr_type_lookup() callers.
> 
> Or whatever.
> 
> I don't want any confusing states with two return types and people
> having to figure out what it exactly means and digging into the code
> and scratching heads WTF is that supposed to mean.

I will change the caller to check MTRR_TYPE_INVALID, and treat it as a
uniform case.

Thanks,
-Toshi



--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v4 7/7] mtrr, mm, x86: Enhance MTRR checks for KVA huge page mapping

[Borislav Petkov]

On Mon, May 11, 2015 at 04:09:39PM -0600, Toshi Kani wrote:
> There may not be any type conflict with MTRR_TYPE_INVALID.

Because...?

Let me guess: you cannot change this function to return a signed value
which is the type when positive and an error when negative?

> I will change the caller to check MTRR_TYPE_INVALID, and treat it as a
> uniform case.

That would be, of course, also wrong.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v4 7/7] mtrr, mm, x86: Enhance MTRR checks for KVA huge page mapping

[Toshi Kani]

On Tue, 2015-05-12 at 09:28 +0200, Borislav Petkov wrote:
> On Mon, May 11, 2015 at 04:09:39PM -0600, Toshi Kani wrote:
> > There may not be any type conflict with MTRR_TYPE_INVALID.
> 
> Because...?

Because you cannot have a memory type conflict with MTRRs when MTRRs are
disabled.  mtrr_type_lookup() returns MTRR_TYPE_INVALID when MTRRs are
disabled.  This is stated in the comments of mtrr_type_lookup() and the
MTRR_TYPE_INVALID definition itself.

BIOS can disable MTRRs, or VM may choose not to implement MTRRs.  The OS
needs to handle this case as a valid config, and this is not an error
case.

> Let me guess: you cannot change this function to return a signed value
> which is the type when positive and an error when negative?

No, that is not the reason. 

> > I will change the caller to check MTRR_TYPE_INVALID, and treat it as a
> > uniform case.
> 
> That would be, of course, also wrong.

I am confused... In your previous comments, you mentioned that:

| If you want to be able to state that a type is uniform even if MTRRs
| are disabled, you need to define another retval which means exactly
| that.

There may not be type conflict when MTRRs are disabled.  There is no
point of defining a new return value.

| Or add an inline function called mtrr_enabled() and call it in the
| mtrr_type_lookup() callers.

MTRR_TYPE_INVALID means MTRRs disabled.  So, the caller checking with
this value is the same as checking with mtrr_enabled() you suggested.

Thanks,
-Toshi


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v4 7/7] mtrr, mm, x86: Enhance MTRR checks for KVA huge page mapping

[Borislav Petkov]

On Tue, May 12, 2015 at 08:30:30AM -0600, Toshi Kani wrote:
> MTRR_TYPE_INVALID means MTRRs disabled.  So, the caller checking with
> this value is the same as checking with mtrr_enabled() you suggested.

So then you don't have to set *uniform = 1 on entry to
mtrr_type_lookup(). And change the retval test

	if ((!uniform) && (mtrr != MTRR_TYPE_WRBACK))

to
	if ((mtrr != MTRR_TYPE_INVALID) && (!uniform) && (mtrr != MTRR_TYPE_WRBACK))

You can put the MTRR_TYPE_INVALID first so that it shortcuts.

You need the distinction between MTRRs *disabled* and an MTRR region
being {non-,}uniform.

If MTRRs are disabled, uniform doesn't *mean* *anything* because it is
undefined. When MTRRs are disabled, the range is *not* covered by MTRRs
because, well, them MTRRs are disabled.

And it might be fine for *your* use case to set *uniform even when MTRRs
are disabled but it might matter in the future. So we better design it
correct from the beginning.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v4 7/7] mtrr, mm, x86: Enhance MTRR checks for KVA huge page mapping

[Toshi Kani]

On Tue, 2015-05-12 at 18:31 +0200, Borislav Petkov wrote:
> On Tue, May 12, 2015 at 08:30:30AM -0600, Toshi Kani wrote:
> > MTRR_TYPE_INVALID means MTRRs disabled.  So, the caller checking with
> > this value is the same as checking with mtrr_enabled() you suggested.
> 
> So then you don't have to set *uniform = 1 on entry to
> mtrr_type_lookup(). And change the retval test
> 
> 	if ((!uniform) && (mtrr != MTRR_TYPE_WRBACK))
> 
> to
> 	if ((mtrr != MTRR_TYPE_INVALID) && (!uniform) && (mtrr != MTRR_TYPE_WRBACK))

Yes, that's what I was thinking as well.  Will do.

> You can put the MTRR_TYPE_INVALID first so that it shortcuts.
> 
> You need the distinction between MTRRs *disabled* and an MTRR region
> being {non-,}uniform.
> 
> If MTRRs are disabled, uniform doesn't *mean* *anything* because it is
> undefined. When MTRRs are disabled, the range is *not* covered by MTRRs
> because, well, them MTRRs are disabled.
> 
> And it might be fine for *your* use case to set *uniform even when MTRRs
> are disabled but it might matter in the future. So we better design it
> correct from the beginning.

I think it is a matter of how "uniform" is defined, but your point is
taken and I will change it accordingly.

Thanks,
-Toshi


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>