x86: PAT: make pat_x_mtrr_type() more readable

PATCH Applied Andreas Herrmann 3 participants 7 messages Jun 10, 2008 – Jun 19, 2008

Subsystems (2)

x86 x86 MM

lore

Share & tools

lore

Andreas Herrmann

author

2008-06-10 14:08 UTC #0 lore raw

PATCH 5/5 x86: PAT: make pat_x_mtrr_type() more readable

I've found it inconvenient to review pat_x_mtrr_type().
Thus I slightly changed it and added some comment to make it more readable.
I've also added BUG statements for (some) unused/unhandled PAT/MTRR types.

Signed-off-by Andreas Herrmann <[email protected]>

---
 arch/x86/mm/pat.c |   30 +++++++++++++++++++++---------
 1 files changed, 21 insertions(+), 9 deletions(-)

arch/x86/mm/pat.c +21 -9

index 2f6c33d..fc0a397 100644
@@ -185,18 +185,30 @@ static int pat_x_mtrr_type(u64 start, u64 end, unsigned long prot,
 	prot &= (~_PAGE_CACHE_MASK);
 
 	/* Currently doing intersection by hand. Optimize it later. */
-	if (pat_type == _PAGE_CACHE_WC) {
+	/*    effective type		        ret_prot
+	  pat \ mtrr  WB WC UC		pat \ mtrr  WB  WC  UC
+	  WC          WC WC WC		WC          WC  WC  WC
+	  UC-         UC WC UC		UC-         UC- UC- UC-
+	  UC          UC UC UC		UC          UC  UC  UC
+	  WB          WB WC UC		WB          WB  WC  UC
+	 */
+	if (pat_type == _PAGE_CACHE_WC)
 		*ret_prot = prot | _PAGE_CACHE_WC;
-	} else if (pat_type == _PAGE_CACHE_UC_MINUS) {
+	else if (pat_type == _PAGE_CACHE_UC_MINUS)
 		*ret_prot = prot | _PAGE_CACHE_UC_MINUS;
-	} else if (pat_type == _PAGE_CACHE_UC ||
-		   mtrr_type == MTRR_TYPE_UNCACHABLE) {
+	else if (pat_type == _PAGE_CACHE_UC)
 		*ret_prot = prot | _PAGE_CACHE_UC;
-	} else if (mtrr_type == MTRR_TYPE_WRCOMB) {
-		*ret_prot = prot | _PAGE_CACHE_WC;
-	} else {
-		*ret_prot = prot | _PAGE_CACHE_WB;
-	}
+	else if (pat_type == _PAGE_CACHE_WB) {
+		if (mtrr_type == MTRR_TYPE_WRBACK)
+			*ret_prot = prot | _PAGE_CACHE_WB;
+		else if (mtrr_type == MTRR_TYPE_WRCOMB)
+			*ret_prot = prot | _PAGE_CACHE_WC;
+		else if (mtrr_type == MTRR_TYPE_UNCACHABLE)
+			*ret_prot = prot | _PAGE_CACHE_UC;
+		else
+			BUG();
+	} else
+		  BUG();
 
 	return 0;
 }

Signature · 1 line

1.5.5.3

Andreas Herrmann

author

2008-06-16 12:58 UTC #1 lore raw

PATCH v2 5/5 x86: PAT: make pat_x_mtrr_type() more readable

I've found it inconvenient to review pat_x_mtrr_type(). Thus I slightly changed it and added some comment to make it more readable.

I've also added BUG statements for (some) unused/unhandled PAT/MTRR types.

Signed-off-by Andreas Herrmann <[email protected]>

---
New patch version. (against current tip/x86/pat - v2.6.26-rc6-105-gfaeca31) Previous version had some conflict with another commit in tip/master.

Regards,
Andreas

 arch/x86/mm/pat.c |   50 +++++++++++++++++++++++++++-----------------------
 1 files changed, 27 insertions(+), 23 deletions(-)

arch/x86/mm/pat.c +27 ~~-23~~

index 4beccea..cdf2c15 100644
@@ -169,35 +169,39 @@ static int pat_x_mtrr_type(u64 start, u64 end, unsigned long prot,
 	prot &= (~_PAGE_CACHE_MASK);
 
 	/*
-	 * We return the PAT request directly for types where PAT takes
-	 * precedence with respect to MTRR and for UC_MINUS.
+	 * We return the PAT request directly for types where PAT
+	 * takes precedence with respect to MTRR and for UC_MINUS. In
+	 * case of pat_type==WB we have to know mtrr_type to get the
+	 * effective type.
+	 *
+	 *    effective type                ret_prot
+	 *  pat \ mtrr  WB WC UC    pat \ mtrr  WB  WC  UC
+	 *  WC          WC WC WC    WC          WC  WC  WC
+	 *  UC-         UC WC UC    UC-         UC- UC- UC-
+	 *  UC          UC UC UC    UC          UC  UC  UC
+	 *  WB          WB WC UC    WB          WB  WC  UC
+	 *
 	 * Consistency checks with other PAT requests is done later
 	 * while going through memtype list.
 	 */
-	if (pat_type == _PAGE_CACHE_WC) {
+	if (pat_type == _PAGE_CACHE_WC)
 		*ret_prot = prot | _PAGE_CACHE_WC;
-		return 0;
-	} else if (pat_type == _PAGE_CACHE_UC_MINUS) {
+	else if (pat_type == _PAGE_CACHE_UC_MINUS)
 		*ret_prot = prot | _PAGE_CACHE_UC_MINUS;
-		return 0;
-	} else if (pat_type == _PAGE_CACHE_UC) {
-		*ret_prot = prot | _PAGE_CACHE_UC;
-		return 0;
-	}
-
-	/*
-	 * Look for MTRR hint to get the effective type in case where PAT
-	 * request is for WB.
-	 */
-	mtrr_type = mtrr_type_lookup(start, end);
-
-	if (mtrr_type == MTRR_TYPE_UNCACHABLE) {
+	else if (pat_type == _PAGE_CACHE_UC)
 		*ret_prot = prot | _PAGE_CACHE_UC;
-	} else if (mtrr_type == MTRR_TYPE_WRCOMB) {
-		*ret_prot = prot | _PAGE_CACHE_WC;
-	} else {
-		*ret_prot = prot | _PAGE_CACHE_WB;
-	}
+	else if (pat_type == _PAGE_CACHE_WB) {
+		mtrr_type = mtrr_type_lookup(start, end);
+		if (mtrr_type == MTRR_TYPE_WRBACK)
+			*ret_prot = prot | _PAGE_CACHE_WB;
+		else if (mtrr_type == MTRR_TYPE_WRCOMB)
+			*ret_prot = prot | _PAGE_CACHE_WC;
+		else if (mtrr_type == MTRR_TYPE_UNCACHABLE)
+			*ret_prot = prot | _PAGE_CACHE_UC;
+		else
+			BUG();
+	} else
+		  BUG();
 
 	return 0;
 }

Signature · 1 line

1.5.5.4

Hugh Dickins

2008-06-16 17:44 UTC #2 lore raw

Quoted 3 lines · Andreas Herrmann

I've found it inconvenient to review pat_x_mtrr_type(). Thus I slightly changed it and added some comment to make it more readable.

I found it hard to read too; but it's amusing how utterly different are our ideas to make it more readable. Most of what it is doing seems to me confusingly left over from earlier implementations.

I've appended my version at the bottom: my involvement in pat.c is not very strong, so would you like to take over my patch and combine best of both into one? I don't particularly want to stroll along after, undoing what you did.

Quoted 3 lines

I've also added BUG statements for (some) unused/unhandled PAT/MTRR types.

I suspect your pat_type BUG is uninteresting (given _PAGE_CACHE_MASK only has two bits to play with), and your mtrr_type BUG dangerous - mtrr_type_lookup may be returning other values, which the current code tolerates, I believe intentionally; but perhaps you've read further than I did, and convinced yourself they cannot get there.

Quoted 23 lines

Signed-off-by Andreas Herrmann <[email protected]>
---
New patch version. (against current tip/x86/pat - v2.6.26-rc6-105-gfaeca31) Previous version had some conflict with another commit in tip/master.
Regards,
Andreas
 arch/x86/mm/pat.c |   50 +++++++++++++++++++++++++++-----------------------
 1 files changed, 27 insertions(+), 23 deletions(-)
arch/x86/mm/pat.c +1 -2
index 4beccea..cdf2c15 100644
@@ -169,35 +169,39 @@ static int pat_x_mtrr_type(u64 start, u64 end, unsigned long prot,
 	prot &= (~_PAGE_CACHE_MASK);
 
 	/*
-	 * We return the PAT request directly for types where PAT takes
-	 * precedence with respect to MTRR and for UC_MINUS.
+	 * We return the PAT request directly for types where PAT

arch/x86/mm/pat.c static int pat_x_mtrr_type(u64 start, u64 end, unsigned long prot, · line 182

Quoted 10 lines

arch/x86/mm/pat.c +10 -0

+	 * takes precedence with respect to MTRR and for UC_MINUS. In
+	 * case of pat_type==WB we have to know mtrr_type to get the
+	 * effective type.
+	 *
+	 *    effective type                ret_prot
+	 *  pat \ mtrr  WB WC UC    pat \ mtrr  WB  WC  UC
+	 *  WC          WC WC WC    WC          WC  WC  WC
+	 *  UC-         UC WC UC    UC-         UC- UC- UC-
+	 *  UC          UC UC UC    UC          UC  UC  UC
+	 *  WB          WB WC UC    WB          WB  WC  UC

I'm inclined to think that once the obfuscations in the code are removed, the code becomes easier to understand than your comment table (which, to be honest, I haven't even checked through).

Quoted 36 lines

arch/x86/mm/pat.c +9 ~~-21~~

+	 *
 	 * Consistency checks with other PAT requests is done later
 	 * while going through memtype list.
 	 */
-	if (pat_type == _PAGE_CACHE_WC) {
+	if (pat_type == _PAGE_CACHE_WC)
 		*ret_prot = prot | _PAGE_CACHE_WC;
-		return 0;
-	} else if (pat_type == _PAGE_CACHE_UC_MINUS) {
+	else if (pat_type == _PAGE_CACHE_UC_MINUS)
 		*ret_prot = prot | _PAGE_CACHE_UC_MINUS;
-		return 0;
-	} else if (pat_type == _PAGE_CACHE_UC) {
-		*ret_prot = prot | _PAGE_CACHE_UC;
-		return 0;
-	}
-
-	/*
-	 * Look for MTRR hint to get the effective type in case where PAT
-	 * request is for WB.
-	 */
-	mtrr_type = mtrr_type_lookup(start, end);
-
-	if (mtrr_type == MTRR_TYPE_UNCACHABLE) {
+	else if (pat_type == _PAGE_CACHE_UC)
 		*ret_prot = prot | _PAGE_CACHE_UC;
-	} else if (mtrr_type == MTRR_TYPE_WRCOMB) {
-		*ret_prot = prot | _PAGE_CACHE_WC;
-	} else {
-		*ret_prot = prot | _PAGE_CACHE_WB;
-	}
+	else if (pat_type == _PAGE_CACHE_WB) {
+		mtrr_type = mtrr_type_lookup(start, end);
+		if (mtrr_type == MTRR_TYPE_WRBACK)
+			*ret_prot = prot | _PAGE_CACHE_WB;
+		else if (mtrr_type == MTRR_TYPE_WRCOMB)

arch/x86/mm/pat.c static int pat_x_mtrr_type(u64 start, u64 end, unsigned long prot, · line 207

Quoted 10 lines

arch/x86/mm/pat.c +7 -0

+			*ret_prot = prot | _PAGE_CACHE_WC;
+		else if (mtrr_type == MTRR_TYPE_UNCACHABLE)
+			*ret_prot = prot | _PAGE_CACHE_UC;
+		else
+			BUG();
+	} else
+		  BUG();
 
 	return 0;
 }

Clean up over-complications in pat_x_mtrr_type(). And if reserve_memtype() ignores stray req_type bits when pat_enabled, it's better to mask them off when not also.

Signed-off-by Hugh Dickins <[email protected]>

---

 arch/x86/mm/pat.c |   47 +++++++++++---------------------------------
 1 file changed, 12 insertions(+), 35 deletions(-)

arch/x86/mm/pat.c +12 ~~-35~~

@@ -145,47 +145,31 @@ static DEFINE_SPINLOCK(memtype_lock); 	/
  * The intersection is based on "Effective Memory Type" tables in IA-32
  * SDM vol 3a
  */
-static int pat_x_mtrr_type(u64 start, u64 end, unsigned long prot,
-				unsigned long *ret_prot)
+static unsigned long pat_x_mtrr_type(u64 start, u64 end, unsigned long req_type)
 {
-	unsigned long pat_type;
 	u8 mtrr_type;
 
-	pat_type = prot & _PAGE_CACHE_MASK;
-	prot &= (~_PAGE_CACHE_MASK);
-
 	/*
 	 * We return the PAT request directly for types where PAT takes
 	 * precedence with respect to MTRR and for UC_MINUS.
 	 * Consistency checks with other PAT requests is done later
 	 * while going through memtype list.
 	 */
-	if (pat_type == _PAGE_CACHE_WC) {
-		*ret_prot = prot | _PAGE_CACHE_WC;
-		return 0;
-	} else if (pat_type == _PAGE_CACHE_UC_MINUS) {
-		*ret_prot = prot | _PAGE_CACHE_UC_MINUS;
-		return 0;
-	} else if (pat_type == _PAGE_CACHE_UC) {
-		*ret_prot = prot | _PAGE_CACHE_UC;
-		return 0;
-	}
+	if (req_type == _PAGE_CACHE_WC ||
+	    req_type == _PAGE_CACHE_UC_MINUS ||
+	    req_type == _PAGE_CACHE_UC)
+		return req_type;
 
 	/*
 	 * Look for MTRR hint to get the effective type in case where PAT
 	 * request is for WB.
 	 */
 	mtrr_type = mtrr_type_lookup(start, end);
-
-	if (mtrr_type == MTRR_TYPE_UNCACHABLE) {
-		*ret_prot = prot | _PAGE_CACHE_UC;
-	} else if (mtrr_type == MTRR_TYPE_WRCOMB) {
-		*ret_prot = prot | _PAGE_CACHE_WC;
-	} else {
-		*ret_prot = prot | _PAGE_CACHE_WB;
-	}
-
-	return 0;
+	if (mtrr_type == MTRR_TYPE_UNCACHABLE)
+		return _PAGE_CACHE_UC;
+	if (mtrr_type == MTRR_TYPE_WRCOMB)
+		return _PAGE_CACHE_WC;
+	return _PAGE_CACHE_WB;
 }
 
 /*

@@ -218,7 +202,7 @@ int reserve_memtype(u64 start, u64 end, 
 			if (req_type == -1) {
 				*ret_type = _PAGE_CACHE_WB;
 			} else {
-				*ret_type = req_type;
+				*ret_type = req_type & _PAGE_CACHE_MASK;
 			}
 		}
 		return 0;

@@ -250,14 +234,7 @@ int reserve_memtype(u64 start, u64 end, 
 		}
 	} else {
 		req_type &= _PAGE_CACHE_MASK;
-		err = pat_x_mtrr_type(start, end, req_type, &actual_type);
-	}
-
-	if (err) {
-		if (ret_type)
-			*ret_type = actual_type;
-
-		return -EINVAL;
+		actual_type = pat_x_mtrr_type(start, end, req_type);
 	}
 
 	new_entry  = kmalloc(sizeof(struct memtype), GFP_KERNEL);

Ingo Molnar

Applied

2008-06-18 09:52 UTC #3 lore raw

Quoted 3 lines · Hugh Dickins

Clean up over-complications in pat_x_mtrr_type(). And if reserve_memtype() ignores stray req_type bits when pat_enabled, it's better to mask them off when not also.

applied, to tip/x86/pat, thanks Hugh. Andreas, if there's any other clean up you can think of here please send a delta patch.

Ingo

Andreas Herrmann

author

2008-06-18 13:39 UTC #4 lore raw

In reply to Hugh Dickins

Quoted 8 lines · Hugh Dickins

Quoted 3 lines · Andreas Herrmann
I've found it inconvenient to review pat_x_mtrr_type(). Thus I slightly changed it and added some comment to make it more readable.
I found it hard to read too; but it's amusing how utterly different are our ideas to make it more readable. Most of what it is doing seems to me confusingly left over from earlier implementations.

;-)

Quoted 4 lines

I've appended my version at the bottom: my involvement in pat.c is not very strong, so would you like to take over my patch and combine best of both into one? I don't particularly want to stroll along after, undoing what you did.

Why combining both patches?
I've checked your patch and found it more straightforward than mine. And the attached patch makes it even shorter.
(patch against tip/x86/pat -- where your patch is already residing)

Quoted 5 lines

Quoted 3 lines
I've also added BUG statements for (some) unused/unhandled PAT/MTRR types.
I suspect your pat_type BUG is uninteresting (given _PAGE_CACHE_MASK

It's just interesting if someone would change that mask and forget to handle potential new pat_types. I admit that is rather unlikely.

Quoted 1 line

only has two bits to play with), and your mtrr_type BUG dangerous -

Well, the former code had this snippet in pat_x_mtrr_type():

- mtrr_type = mtrr_type_lookup(start, end);
- if (mtrr_type == 0xFF) { /* MTRR not enabled */
- *ret_prot = prot;
- return 0;
- }
- if (mtrr_type == 0xFE) { /* MTRR match error */
- *ret_prot = _PAGE_CACHE_UC;
- return -1;
- }
- if (mtrr_type != MTRR_TYPE_UNCACHABLE &&
- mtrr_type != MTRR_TYPE_WRBACK &&
- mtrr_type != MTRR_TYPE_WRCOMB) { /* MTRR type unhandled */
- *ret_prot = _PAGE_CACHE_UC;
- return -1;
- }
-

But now it seems that the function intentional handles MTRR_TYPE_WRTHROUGH and the 0xFE/0xFF cases and thus the BUG statement is wrong.

Thanks,

Andreas

---
[PATCH] x86: shrink pat_x_mtrr_type to its essentials

Signed-off-by Andreas Herrmann <[email protected]>

---
 arch/x86/mm/pat.c |   30 +++++++++++-------------------
 1 files changed, 11 insertions(+), 19 deletions(-)

arch/x86/mm/pat.c +11 ~~-19~~

index ac3a2b1..227df3c 100644
@@ -161,29 +161,21 @@ static DEFINE_SPINLOCK(memtype_lock); 	/* protects memtype list */
  */
 static unsigned long pat_x_mtrr_type(u64 start, u64 end, unsigned long req_type)
 {
-	u8 mtrr_type;
-
-	/*
-	 * We return the PAT request directly for types where PAT takes
-	 * precedence with respect to MTRR and for UC_MINUS.
-	 * Consistency checks with other PAT requests is done later
-	 * while going through memtype list.
-	 */
-	if (req_type == _PAGE_CACHE_WC ||
-	    req_type == _PAGE_CACHE_UC_MINUS ||
-	    req_type == _PAGE_CACHE_UC)
-		return req_type;
-
 	/*
 	 * Look for MTRR hint to get the effective type in case where PAT
 	 * request is for WB.
 	 */
-	mtrr_type = mtrr_type_lookup(start, end);
-	if (mtrr_type == MTRR_TYPE_UNCACHABLE)
-		return _PAGE_CACHE_UC;
-	if (mtrr_type == MTRR_TYPE_WRCOMB)
-		return _PAGE_CACHE_WC;
-	return _PAGE_CACHE_WB;
+	if (req_type == _PAGE_CACHE_WB) {
+		u8 mtrr_type;
+
+		mtrr_type = mtrr_type_lookup(start, end);
+		if (mtrr_type == MTRR_TYPE_UNCACHABLE)
+			return _PAGE_CACHE_UC;
+		if (mtrr_type == MTRR_TYPE_WRCOMB)
+			return _PAGE_CACHE_WC;
+	}
+
+	return req_type;
 }
 
 /*

Signature · 1 line

1.5.5.4

Hugh Dickins

Acked-by

2008-06-18 16:29 UTC #5 lore raw

Quoted 2 lines · Andreas Herrmann

And the attached patch makes it even shorter.
(patch against tip/x86/pat -- where your patch is already residing)

Yes, that's delightful, even better, thanks. After accusing you of an uninteresting BUG on a fifth value from 2 bits, I had an inkling that maybe my patch was therefore still stupidly verbose.

Quoted 4 lines

---
[PATCH] x86: shrink pat_x_mtrr_type to its essentials
Signed-off-by Andreas Herrmann <[email protected]>

Acked-by Hugh Dickins <[email protected]>

Quoted 49 lines

---
 arch/x86/mm/pat.c |   30 +++++++++++-------------------
 1 files changed, 11 insertions(+), 19 deletions(-)

arch/x86/mm/pat.c +11 ~~-19~~

index ac3a2b1..227df3c 100644
@@ -161,29 +161,21 @@ static DEFINE_SPINLOCK(memtype_lock); 	/* protects memtype list */
  */
 static unsigned long pat_x_mtrr_type(u64 start, u64 end, unsigned long req_type)
 {
-	u8 mtrr_type;
-
-	/*
-	 * We return the PAT request directly for types where PAT takes
-	 * precedence with respect to MTRR and for UC_MINUS.
-	 * Consistency checks with other PAT requests is done later
-	 * while going through memtype list.
-	 */
-	if (req_type == _PAGE_CACHE_WC ||
-	    req_type == _PAGE_CACHE_UC_MINUS ||
-	    req_type == _PAGE_CACHE_UC)
-		return req_type;
-
 	/*
 	 * Look for MTRR hint to get the effective type in case where PAT
 	 * request is for WB.
 	 */
-	mtrr_type = mtrr_type_lookup(start, end);
-	if (mtrr_type == MTRR_TYPE_UNCACHABLE)
-		return _PAGE_CACHE_UC;
-	if (mtrr_type == MTRR_TYPE_WRCOMB)
-		return _PAGE_CACHE_WC;
-	return _PAGE_CACHE_WB;
+	if (req_type == _PAGE_CACHE_WB) {
+		u8 mtrr_type;
+
+		mtrr_type = mtrr_type_lookup(start, end);
+		if (mtrr_type == MTRR_TYPE_UNCACHABLE)
+			return _PAGE_CACHE_UC;
+		if (mtrr_type == MTRR_TYPE_WRCOMB)
+			return _PAGE_CACHE_WC;
+	}
+
+	return req_type;
 }
 
 /*

Ingo Molnar

Applied

2008-06-19 10:39 UTC #6 lore raw

In reply to Andreas Herrmann

Quoted 3 lines · Andreas Herrmann

[PATCH] x86: shrink pat_x_mtrr_type to its essentials
Signed-off-by Andreas Herrmann <[email protected]>

applied to tip/x86/pat - thanks Andreas.

now pat_x_mtrr_type() is not nearly as scary anymore :-)

Ingo