[RFC PATCH -tip] x86/pat: fix pat_x_mtrr_type to its essentials

[RFC PATCH -tip] x86/pat: fix pat_x_mtrr_type to its essentials

[Xiaotian Feng]

Due to the IA-32 SDM vol 3a "Effective Memory Type" tables, the PAT
and mtrr relations should be:
	|	UC	UC-	WC	WB	[PAT]
--------+--------------------------------------
UC	|	UC	UC(UC-)	WC	UC(UC-)
WC	|	UC	WC(UC-)	WC	WC(UC-)
WB	|	UC	UC(UC-)	WC	WB
[MTRR]
* In () is the current return value.

So, commit b6ff32, Fix pat_x_mtrr_type() to use UC_MINUS when the
mtrr type return UC. But it also made mtrr type WC use UC_MINUS.
This is not reasonable.

Signed-off-by: Xiaotian Feng <dfeng@redhat.com>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@redhat.com>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Suresh Siddha <suresh.b.siddha@intel.com>
Cc: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
---
 arch/x86/mm/pat.c |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c
index c37fd51..92e9627 100644
--- a/arch/x86/mm/pat.c
+++ b/arch/x86/mm/pat.c
@@ -228,10 +228,11 @@ static unsigned long pat_x_mtrr_type(u64 start, u64 end, unsigned long req_type)
 		u8 mtrr_type;
 
 		mtrr_type = mtrr_type_lookup(start, end);
-		if (mtrr_type != MTRR_TYPE_WRBACK)
-			return _PAGE_CACHE_UC_MINUS;
-
-		return _PAGE_CACHE_WB;
+		if (mtrr_type == MTRR_TYPE_WRBACK)
+			return _PAGE_CACHE_WB;
+		else if (mtrr_type == MTRR_TYPE_WRCOMB)
+			return _PAGE_CACHE_WC;
+		return _PAGE_CACHE_UC_MINUS;
 	}
 
 	return req_type;
-- 
1.6.5.2

Re: [RFC PATCH -tip] x86/pat: fix pat_x_mtrr_type to its essentials

[Suresh Siddha]

Xiaotian,

We look through the mtrr's only when the incoming request type is WB
(which is mostly the case for legacy /dev/mem mmap mapping). And only in
this scenario we use more conservative UC- instead of WC, if mtrr says
WC.

This will ensure the scenarios like ioremap() (which defaults to UC-) by
legacy drivers, before and after the user level mmap are consistent and
don't fail because of inconsistent mappings etc.

There is no reason why we need to use WC when especially the incoming
request was not strict enough and specified WB.

Also BTW,  your current effective return values mentioned in the below
table () is wrong.

i.e., for example, when we use UC- for PAT and mtrr has WC, effective
memory type used by cpu will be WC.

thanks,
suresh

On Thu, 2009-11-26 at 04:15 -0800, Xiaotian Feng wrote:
> Due to the IA-32 SDM vol 3a "Effective Memory Type" tables, the PAT
> and mtrr relations should be:
> 	|	UC	UC-	WC	WB	[PAT]
> --------+--------------------------------------
> UC	|	UC	UC(UC-)	WC	UC(UC-)
> WC	|	UC	WC(UC-)	WC	WC(UC-)
> WB	|	UC	UC(UC-)	WC	WB
> [MTRR]
> * In () is the current return value.
> 
> So, commit b6ff32, Fix pat_x_mtrr_type() to use UC_MINUS when the
> mtrr type return UC. But it also made mtrr type WC use UC_MINUS.
> This is not reasonable.
> 
> Signed-off-by: Xiaotian Feng <dfeng@redhat.com>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@redhat.com>
> Cc: H. Peter Anvin <hpa@zytor.com>
> Cc: Suresh Siddha <suresh.b.siddha@intel.com>
> Cc: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
> ---
>  arch/x86/mm/pat.c |    9 +++++----
>  1 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c
> index c37fd51..92e9627 100644
> --- a/arch/x86/mm/pat.c
> +++ b/arch/x86/mm/pat.c
> @@ -228,10 +228,11 @@ static unsigned long pat_x_mtrr_type(u64 start, u64 end, unsigned long req_type)
>  		u8 mtrr_type;
>  
>  		mtrr_type = mtrr_type_lookup(start, end);
> -		if (mtrr_type != MTRR_TYPE_WRBACK)
> -			return _PAGE_CACHE_UC_MINUS;
> -
> -		return _PAGE_CACHE_WB;
> +		if (mtrr_type == MTRR_TYPE_WRBACK)
> +			return _PAGE_CACHE_WB;
> +		else if (mtrr_type == MTRR_TYPE_WRCOMB)
> +			return _PAGE_CACHE_WC;
> +		return _PAGE_CACHE_UC_MINUS;
>  	}
>  
>  	return req_type;

Re: [RFC PATCH -tip] x86/pat: fix pat_x_mtrr_type to its essentials

[Xiaotian Feng]

On 12/01/2009 07:21 AM, Suresh Siddha wrote:
> Xiaotian,
>
> We look through the mtrr's only when the incoming request type is WB
> (which is mostly the case for legacy /dev/mem mmap mapping). And only in
> this scenario we use more conservative UC- instead of WC, if mtrr says
> WC.
>
> This will ensure the scenarios like ioremap() (which defaults to UC-) by
> legacy drivers, before and after the user level mmap are consistent and
> don't fail because of inconsistent mappings etc.
>
> There is no reason why we need to use WC when especially the incoming
> request was not strict enough and specified WB.
>
> Also BTW,  your current effective return values mentioned in the below
> table () is wrong.
>
> i.e., for example, when we use UC- for PAT and mtrr has WC, effective
> memory type used by cpu will be WC.
Thanks for the detailed explanation, I'm clear now, thank you very much.
>
> thanks,
> suresh
>
> On Thu, 2009-11-26 at 04:15 -0800, Xiaotian Feng wrote:
>> Due to the IA-32 SDM vol 3a "Effective Memory Type" tables, the PAT
>> and mtrr relations should be:
>> 	|	UC	UC-	WC	WB	[PAT]
>> --------+--------------------------------------
>> UC	|	UC	UC(UC-)	WC	UC(UC-)
>> WC	|	UC	WC(UC-)	WC	WC(UC-)
>> WB	|	UC	UC(UC-)	WC	WB
>> [MTRR]
>> * In () is the current return value.
>>
>> So, commit b6ff32, Fix pat_x_mtrr_type() to use UC_MINUS when the
>> mtrr type return UC. But it also made mtrr type WC use UC_MINUS.
>> This is not reasonable.
>>
>> Signed-off-by: Xiaotian Feng<dfeng@redhat.com>
>> Cc: Thomas Gleixner<tglx@linutronix.de>
>> Cc: Ingo Molnar<mingo@redhat.com>
>> Cc: H. Peter Anvin<hpa@zytor.com>
>> Cc: Suresh Siddha<suresh.b.siddha@intel.com>
>> Cc: Venkatesh Pallipadi<venkatesh.pallipadi@intel.com>
>> ---
>>   arch/x86/mm/pat.c |    9 +++++----
>>   1 files changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/x86/mm/pat.c b/arch/x86/mm/pat.c
>> index c37fd51..92e9627 100644
>> --- a/arch/x86/mm/pat.c
>> +++ b/arch/x86/mm/pat.c
>> @@ -228,10 +228,11 @@ static unsigned long pat_x_mtrr_type(u64 start, u64 end, unsigned long req_type)
>>   		u8 mtrr_type;
>>
>>   		mtrr_type = mtrr_type_lookup(start, end);
>> -		if (mtrr_type != MTRR_TYPE_WRBACK)
>> -			return _PAGE_CACHE_UC_MINUS;
>> -
>> -		return _PAGE_CACHE_WB;
>> +		if (mtrr_type == MTRR_TYPE_WRBACK)
>> +			return _PAGE_CACHE_WB;
>> +		else if (mtrr_type == MTRR_TYPE_WRCOMB)
>> +			return _PAGE_CACHE_WC;
>> +		return _PAGE_CACHE_UC_MINUS;
>>   	}
>>
>>   	return req_type;
>
>