diff mbox

[v9,7/10] x86, mm, asm: Add WT support to set_page_memtype()

Message ID 1431551151-19124-8-git-send-email-toshi.kani@hp.com
State Superseded
Headers show

Commit Message

Toshi Kani May 13, 2015, 9:05 p.m. UTC
As set_memory_wb() calls free_ram_pages_type(), which then calls
set_page_memtype() with -1, _PGMT_DEFAULT is used for tracking
the WB type.  _PGMT_WB is defined but unused.  Hence, this patch
renames _PGMT_DEFAULT to _PGMT_WB to clarify the usage, and
releases the slot used by _PGMT_WB before.  free_ram_pages_type()
is changed to call set_page_memtype() with _PGMT_WB, and
get_page_memtype() returns _PAGE_CACHE_MODE_WB for _PGMT_WB.

This patch then defines _PGMT_WT to the released slot.  This enables
set_page_memtype() to track the WT type.

Signed-off-by: Toshi Kani <toshi.kani@hp.com>
---
 arch/x86/mm/pat.c |   48 +++++++++++++++++++++---------------------------
 1 file changed, 21 insertions(+), 27 deletions(-)

Comments

Thomas Gleixner May 22, 2015, 7:35 a.m. UTC | #1
On Wed, 13 May 2015, Toshi Kani wrote:
> + * X86 PAT uses page flags arch_1 and uncached together to keep track of
> + * memory type of pages that have backing page struct. X86 PAT supports 4
> + * different memory types, _PAGE_CACHE_MODE_WT, _PAGE_CACHE_MODE_WC,
> + * _PAGE_CACHE_MODE_UC_MINUS and _PAGE_CACHE_MODE_WB where page's memory
> + * type has not been changed from its default.

This is a horrible sentence.

 * X86 PAT supports 4 different memory types:
 *  - _PAGE_CACHE_MODE_WB
 *  - _PAGE_CACHE_MODE_WC
 *  - _PAGE_CACHE_MODE_UC_MINUS
 *  - _PAGE_CACHE_MODE_WT
 *
 * _PAGE_CACHE_MODE_WB is the default type.
 */
Hmm?

>   * Note we do not support _PAGE_CACHE_MODE_UC here.

This can be removed as it is completely redundant.

>   */
>  
> -#define _PGMT_DEFAULT		0
> +#define _PGMT_WB		0	/* default */

We just established two lines above that this is the default

>  #define _PGMT_WC		(1UL << PG_arch_1)
>  #define _PGMT_UC_MINUS		(1UL << PG_uncached)
> -#define _PGMT_WB		(1UL << PG_uncached | 1UL << PG_arch_1)
> +#define _PGMT_WT		(1UL << PG_uncached | 1UL << PG_arch_1)
>  #define _PGMT_MASK		(1UL << PG_uncached | 1UL << PG_arch_1)
>  #define _PGMT_CLEAR_MASK	(~_PGMT_MASK)
>  
> @@ -88,14 +88,14 @@ static inline enum page_cache_mode get_page_memtype(struct page *pg)
>  {
>  	unsigned long pg_flags = pg->flags & _PGMT_MASK;
>  
> -	if (pg_flags == _PGMT_DEFAULT)
> -		return -1;
> +	if (pg_flags == _PGMT_WB)
> +		return _PAGE_CACHE_MODE_WB;
>  	else if (pg_flags == _PGMT_WC)
>  		return _PAGE_CACHE_MODE_WC;
>  	else if (pg_flags == _PGMT_UC_MINUS)
>  		return _PAGE_CACHE_MODE_UC_MINUS;
>  	else
> -		return _PAGE_CACHE_MODE_WB;
> +		return _PAGE_CACHE_MODE_WT;
>  }
>  
>  static inline void set_page_memtype(struct page *pg,
> @@ -112,11 +112,12 @@ static inline void set_page_memtype(struct page *pg,
>  	case _PAGE_CACHE_MODE_UC_MINUS:
>  		memtype_flags = _PGMT_UC_MINUS;
>  		break;
> -	case _PAGE_CACHE_MODE_WB:
> -		memtype_flags = _PGMT_WB;
> +	case _PAGE_CACHE_MODE_WT:
> +		memtype_flags = _PGMT_WT;
>  		break;
> +	case _PAGE_CACHE_MODE_WB:
>  	default:
> -		memtype_flags = _PGMT_DEFAULT;
> +		memtype_flags = _PGMT_WB;	/* default */

What's the value of that  comment?

       default:
		 /* default */

Aside of the, please do not use tail comments. They make code harder
to parse.

>  /*
>   * For RAM pages, we use page flags to mark the pages with appropriate type.
> - * The page flags are limited to three types, WB, WC and UC-.
> - * WT and WP requests fail with -EINVAL, and UC gets redirected to UC-.
> + * The page flags are limited to four types, WB (default), WC, WT and UC-.
> + * WP request fails with -EINVAL, and UC gets redirected to UC-.

> + * A new memtype can only be set to the default memtype WB.

I have no idea what that line means.

> @@ -582,13 +583,6 @@ static enum page_cache_mode lookup_memtype(u64 paddr)
>  		struct page *page;
>  		page = pfn_to_page(paddr >> PAGE_SHIFT);
>  		rettype = get_page_memtype(page);

		return get_page_memtype(page);

And while you are at it please add the missing newline between the
variable declaration and code.

Thanks,

	tglx
Toshi Kani May 22, 2015, 3:04 p.m. UTC | #2
On Fri, 2015-05-22 at 09:35 +0200, Thomas Gleixner wrote:
> On Wed, 13 May 2015, Toshi Kani wrote:
> > + * X86 PAT uses page flags arch_1 and uncached together to keep track of
> > + * memory type of pages that have backing page struct. X86 PAT supports 4
> > + * different memory types, _PAGE_CACHE_MODE_WT, _PAGE_CACHE_MODE_WC,
> > + * _PAGE_CACHE_MODE_UC_MINUS and _PAGE_CACHE_MODE_WB where page's memory
> > + * type has not been changed from its default.
> 
> This is a horrible sentence.
> 
>  * X86 PAT supports 4 different memory types:
>  *  - _PAGE_CACHE_MODE_WB
>  *  - _PAGE_CACHE_MODE_WC
>  *  - _PAGE_CACHE_MODE_UC_MINUS
>  *  - _PAGE_CACHE_MODE_WT
>  *
>  * _PAGE_CACHE_MODE_WB is the default type.
>  */
> Hmm?

Sounds good. I will update as suggested.

> >   * Note we do not support _PAGE_CACHE_MODE_UC here.
> 
> This can be removed as it is completely redundant.

Will remove this sentence.

> >   */
> >  
> > -#define _PGMT_DEFAULT		0
> > +#define _PGMT_WB		0	/* default */
> 
> We just established two lines above that this is the default

Will remove this comment as well.

> >  #define _PGMT_WC		(1UL << PG_arch_1)
> >  #define _PGMT_UC_MINUS		(1UL << PG_uncached)
> > -#define _PGMT_WB		(1UL << PG_uncached | 1UL << PG_arch_1)
> > +#define _PGMT_WT		(1UL << PG_uncached | 1UL << PG_arch_1)
> >  #define _PGMT_MASK		(1UL << PG_uncached | 1UL << PG_arch_1)
> >  #define _PGMT_CLEAR_MASK	(~_PGMT_MASK)
> >  
> > @@ -88,14 +88,14 @@ static inline enum page_cache_mode get_page_memtype(struct page *pg)
> >  {
> >  	unsigned long pg_flags = pg->flags & _PGMT_MASK;
> >  
> > -	if (pg_flags == _PGMT_DEFAULT)
> > -		return -1;
> > +	if (pg_flags == _PGMT_WB)
> > +		return _PAGE_CACHE_MODE_WB;
> >  	else if (pg_flags == _PGMT_WC)
> >  		return _PAGE_CACHE_MODE_WC;
> >  	else if (pg_flags == _PGMT_UC_MINUS)
> >  		return _PAGE_CACHE_MODE_UC_MINUS;
> >  	else
> > -		return _PAGE_CACHE_MODE_WB;
> > +		return _PAGE_CACHE_MODE_WT;
> >  }
> >  
> >  static inline void set_page_memtype(struct page *pg,
> > @@ -112,11 +112,12 @@ static inline void set_page_memtype(struct page *pg,
> >  	case _PAGE_CACHE_MODE_UC_MINUS:
> >  		memtype_flags = _PGMT_UC_MINUS;
> >  		break;
> > -	case _PAGE_CACHE_MODE_WB:
> > -		memtype_flags = _PGMT_WB;
> > +	case _PAGE_CACHE_MODE_WT:
> > +		memtype_flags = _PGMT_WT;
> >  		break;
> > +	case _PAGE_CACHE_MODE_WB:
> >  	default:
> > -		memtype_flags = _PGMT_DEFAULT;
> > +		memtype_flags = _PGMT_WB;	/* default */
> 
> What's the value of that  comment?
> 
>        default:
> 		 /* default */

I was trying to preserve the original name of "_PGMT_DEFAULT" with the
comment because _PGMT_WB takes the place of _PGMT_DEFAULT with this
change.  But I agree that this may be redundant.  I will remove this
comment as well.

> Aside of the, please do not use tail comments. They make code harder
> to parse.

Got it.

> >  /*
> >   * For RAM pages, we use page flags to mark the pages with appropriate type.
> > - * The page flags are limited to three types, WB, WC and UC-.
> > - * WT and WP requests fail with -EINVAL, and UC gets redirected to UC-.
> > + * The page flags are limited to four types, WB (default), WC, WT and UC-.
> > + * WP request fails with -EINVAL, and UC gets redirected to UC-.
> 
> > + * A new memtype can only be set to the default memtype WB.
> 
> I have no idea what that line means.

I will change it to "Setting a new memory type is only allowed to a page
mapped with the default WB type."

> > @@ -582,13 +583,6 @@ static enum page_cache_mode lookup_memtype(u64 paddr)
> >  		struct page *page;
> >  		page = pfn_to_page(paddr >> PAGE_SHIFT);
> >  		rettype = get_page_memtype(page);
> 
> 		return get_page_memtype(page);
> 
> And while you are at it please add the missing newline between the
> variable declaration and code.

Will do.

Thanks,
-Toshi
diff mbox

Patch

diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c
index aee5cdf..ed191e0 100644
--- a/arch/x86/mm/pat.c
+++ b/arch/x86/mm/pat.c
@@ -69,18 +69,18 @@  static u64 __read_mostly boot_pat_state;
 
 #ifdef CONFIG_X86_PAT
 /*
- * X86 PAT uses page flags WC and Uncached together to keep track of
- * memory type of pages that have backing page struct. X86 PAT supports 3
- * different memory types, _PAGE_CACHE_MODE_WB, _PAGE_CACHE_MODE_WC and
- * _PAGE_CACHE_MODE_UC_MINUS and fourth state where page's memory type has not
- * been changed from its default (value of -1 used to denote this).
+ * X86 PAT uses page flags arch_1 and uncached together to keep track of
+ * memory type of pages that have backing page struct. X86 PAT supports 4
+ * different memory types, _PAGE_CACHE_MODE_WT, _PAGE_CACHE_MODE_WC,
+ * _PAGE_CACHE_MODE_UC_MINUS and _PAGE_CACHE_MODE_WB where page's memory
+ * type has not been changed from its default.
  * Note we do not support _PAGE_CACHE_MODE_UC here.
  */
 
-#define _PGMT_DEFAULT		0
+#define _PGMT_WB		0	/* default */
 #define _PGMT_WC		(1UL << PG_arch_1)
 #define _PGMT_UC_MINUS		(1UL << PG_uncached)
-#define _PGMT_WB		(1UL << PG_uncached | 1UL << PG_arch_1)
+#define _PGMT_WT		(1UL << PG_uncached | 1UL << PG_arch_1)
 #define _PGMT_MASK		(1UL << PG_uncached | 1UL << PG_arch_1)
 #define _PGMT_CLEAR_MASK	(~_PGMT_MASK)
 
@@ -88,14 +88,14 @@  static inline enum page_cache_mode get_page_memtype(struct page *pg)
 {
 	unsigned long pg_flags = pg->flags & _PGMT_MASK;
 
-	if (pg_flags == _PGMT_DEFAULT)
-		return -1;
+	if (pg_flags == _PGMT_WB)
+		return _PAGE_CACHE_MODE_WB;
 	else if (pg_flags == _PGMT_WC)
 		return _PAGE_CACHE_MODE_WC;
 	else if (pg_flags == _PGMT_UC_MINUS)
 		return _PAGE_CACHE_MODE_UC_MINUS;
 	else
-		return _PAGE_CACHE_MODE_WB;
+		return _PAGE_CACHE_MODE_WT;
 }
 
 static inline void set_page_memtype(struct page *pg,
@@ -112,11 +112,12 @@  static inline void set_page_memtype(struct page *pg,
 	case _PAGE_CACHE_MODE_UC_MINUS:
 		memtype_flags = _PGMT_UC_MINUS;
 		break;
-	case _PAGE_CACHE_MODE_WB:
-		memtype_flags = _PGMT_WB;
+	case _PAGE_CACHE_MODE_WT:
+		memtype_flags = _PGMT_WT;
 		break;
+	case _PAGE_CACHE_MODE_WB:
 	default:
-		memtype_flags = _PGMT_DEFAULT;
+		memtype_flags = _PGMT_WB;	/* default */
 		break;
 	}
 
@@ -365,8 +366,9 @@  static int pat_pagerange_is_ram(resource_size_t start, resource_size_t end)
 
 /*
  * For RAM pages, we use page flags to mark the pages with appropriate type.
- * The page flags are limited to three types, WB, WC and UC-.
- * WT and WP requests fail with -EINVAL, and UC gets redirected to UC-.
+ * The page flags are limited to four types, WB (default), WC, WT and UC-.
+ * WP request fails with -EINVAL, and UC gets redirected to UC-.
+ * A new memtype can only be set to the default memtype WB.
  * Here we do two pass:
  * - Find the memtype of all the pages in the range, look for any conflicts
  * - In case of no conflicts, set the new memtype for pages in the range
@@ -378,8 +380,7 @@  static int reserve_ram_pages_type(u64 start, u64 end,
 	struct page *page;
 	u64 pfn;
 
-	if ((req_type == _PAGE_CACHE_MODE_WT) ||
-	    (req_type == _PAGE_CACHE_MODE_WP)) {
+	if (req_type == _PAGE_CACHE_MODE_WP) {
 		if (new_type)
 			*new_type = _PAGE_CACHE_MODE_UC_MINUS;
 		return -EINVAL;
@@ -396,7 +397,7 @@  static int reserve_ram_pages_type(u64 start, u64 end,
 
 		page = pfn_to_page(pfn);
 		type = get_page_memtype(page);
-		if (type != -1) {
+		if (type != _PAGE_CACHE_MODE_WB) {
 			pr_info("reserve_ram_pages_type failed [mem %#010Lx-%#010Lx], track 0x%x, req 0x%x\n",
 				start, end - 1, type, req_type);
 			if (new_type)
@@ -423,7 +424,7 @@  static int free_ram_pages_type(u64 start, u64 end)
 
 	for (pfn = (start >> PAGE_SHIFT); pfn < (end >> PAGE_SHIFT); ++pfn) {
 		page = pfn_to_page(pfn);
-		set_page_memtype(page, -1);
+		set_page_memtype(page, _PAGE_CACHE_MODE_WB);
 	}
 	return 0;
 }
@@ -568,7 +569,7 @@  int free_memtype(u64 start, u64 end)
  * Only to be called when PAT is enabled
  *
  * Returns _PAGE_CACHE_MODE_WB, _PAGE_CACHE_MODE_WC, _PAGE_CACHE_MODE_UC_MINUS
- * or _PAGE_CACHE_MODE_UC
+ * or _PAGE_CACHE_MODE_WT.
  */
 static enum page_cache_mode lookup_memtype(u64 paddr)
 {
@@ -582,13 +583,6 @@  static enum page_cache_mode lookup_memtype(u64 paddr)
 		struct page *page;
 		page = pfn_to_page(paddr >> PAGE_SHIFT);
 		rettype = get_page_memtype(page);
-		/*
-		 * -1 from get_page_memtype() implies RAM page is in its
-		 * default state and not reserved, and hence of type WB
-		 */
-		if (rettype == -1)
-			rettype = _PAGE_CACHE_MODE_WB;
-
 		return rettype;
 	}