Re: Folding _PAGE_PWT into _PAGE_PCD (was Re: unify pagetable accessors patch causes double fault II)
Venki Pallipadi wrote: On Tue, Jan 15, 2008 at 09:16:50AM -0800, Jeremy Fitzhardinge wrote: Ingo Molnar wrote: -#define _PAGE_PRESENT (_AC(1, UL)<<_PAGE_BIT_PRESENT) -#define _PAGE_RW (_AC(1, UL)<<_PAGE_BIT_RW) -#define _PAGE_USER (_AC(1, UL)<<_PAGE_BIT_USER) -#define _PAGE_PWT (_AC(1, UL)<<_PAGE_BIT_PWT) -#define _PAGE_PCD ((_AC(1, UL)<<_PAGE_BIT_PCD) | _PAGE_PWT) BTW, I just noticed that _PAGE_PWT has been folded into _PAGE_PCD. This seems like a really bad idea to me, since it breaks the rule that _PAGE_X == 1 << _PAGE_BIT_X. I can't think of a specific place where this would cause problems, but this kind of non-uniformity always ends up biting someone in the arse. I think having a specific _PAGE_NOCACHE which combines these bits is a better approach. J How about the patch below. It defines new _PAGE_UC. One concern is drivers continuing to use _PAGE_PCD and getting wrong attributes. May be we need to rename _PAGE_PCD to catch those errors as well? Sure, looks fine. I would have said that _NOCACHE matches current usage better, but if it makes more sense to have _UC and _WC then that's fine with me. I guess renaming _PAGE_BIT_PCD to _PAGE_BIT__PCD and the corresponding _PAGE__PCD might be reasonable if you think there's a chance of new misusers appearing (I guess something like out of tree DRI/proprietary patches are a source of that). J -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Folding _PAGE_PWT into _PAGE_PCD (was Re: unify pagetable accessors patch causes double fault II)
On Tue, Jan 15, 2008 at 09:16:50AM -0800, Jeremy Fitzhardinge wrote:
> Ingo Molnar wrote:
> >-#define _PAGE_PRESENT (_AC(1, UL)<<_PAGE_BIT_PRESENT)
> >-#define _PAGE_RW(_AC(1, UL)<<_PAGE_BIT_RW)
> >-#define _PAGE_USER (_AC(1, UL)<<_PAGE_BIT_USER)
> >-#define _PAGE_PWT (_AC(1, UL)<<_PAGE_BIT_PWT)
> >-#define _PAGE_PCD ((_AC(1, UL)<<_PAGE_BIT_PCD) | _PAGE_PWT)
> >
>
> BTW, I just noticed that _PAGE_PWT has been folded into _PAGE_PCD. This
> seems like a really bad idea to me, since it breaks the rule that
> _PAGE_X == 1 << _PAGE_BIT_X. I can't think of a specific place where
> this would cause problems, but this kind of non-uniformity always ends
> up biting someone in the arse.
>
> I think having a specific _PAGE_NOCACHE which combines these bits is a
> better approach.
>
>J
How about the patch below. It defines new _PAGE_UC. One concern is drivers
continuing to use _PAGE_PCD and getting wrong attributes. May be we need to
rename _PAGE_PCD to catch those errors as well?
Thanks,
Venki
Do not fold PCD and PWT bits in _PAGE_PCD. Instead, introduce a new
_PAGE_UC which defines uncached mappings and use it in place of _PAGE_PCD.
Signed-off-by: Venkatesh Pallipadi <[EMAIL PROTECTED]>
Index: linux-2.6.git/arch/x86/mm/ioremap_32.c
===
--- linux-2.6.git.orig/arch/x86/mm/ioremap_32.c 2008-01-15 03:29:38.0
-0800
+++ linux-2.6.git/arch/x86/mm/ioremap_32.c 2008-01-15 04:42:59.0
-0800
@@ -173,7 +173,7 @@
void __iomem *ioremap_nocache (unsigned long phys_addr, unsigned long size)
{
- return __ioremap(phys_addr, size, _PAGE_PCD);
+ return __ioremap(phys_addr, size, _PAGE_UC);
}
EXPORT_SYMBOL(ioremap_nocache);
Index: linux-2.6.git/arch/x86/mm/ioremap_64.c
===
--- linux-2.6.git.orig/arch/x86/mm/ioremap_64.c 2008-01-15 03:29:38.0
-0800
+++ linux-2.6.git/arch/x86/mm/ioremap_64.c 2008-01-15 04:43:07.0
-0800
@@ -150,7 +150,7 @@
void __iomem *ioremap_nocache (unsigned long phys_addr, unsigned long size)
{
- return __ioremap(phys_addr, size, _PAGE_PCD);
+ return __ioremap(phys_addr, size, _PAGE_UC);
}
EXPORT_SYMBOL(ioremap_nocache);
Index: linux-2.6.git/arch/x86/mm/pat.c
===
--- linux-2.6.git.orig/arch/x86/mm/pat.c2008-01-15 03:29:38.0
-0800
+++ linux-2.6.git/arch/x86/mm/pat.c 2008-01-15 05:01:43.0 -0800
@@ -64,7 +64,7 @@
if (smp_processor_id() && !pat_wc_enabled)
return;
- /* Set PWT+PCD to Write-Combining. All other bits stay the same */
+ /* Set PCD to Write-Combining. All other bits stay the same */
/* PTE encoding used in Linux:
PAT
|PCD
@@ -72,7 +72,7 @@
|||
000 WB default
010 WC _PAGE_WC
- 011 UC _PAGE_PCD
+ 011 UC _PAGE_UC
PAT bit unused */
pat = PAT(0,WB) | PAT(1,WT) | PAT(2,WC) | PAT(3,UC) |
PAT(4,WB) | PAT(5,WT) | PAT(6,WC) | PAT(7,UC);
@@ -97,7 +97,7 @@
{
switch (flags & _PAGE_CACHE_MASK) {
case _PAGE_WC: return "write combining";
- case _PAGE_PCD: return "uncached";
+ case _PAGE_UC: return "uncached";
case 0: return "default";
default:return "broken";
}
@@ -144,7 +144,7 @@
if (!fattr)
return -EINVAL;
else
- *fattr = _PAGE_PCD;
+ *fattr = _PAGE_UC;
}
return 0;
@@ -227,13 +227,13 @@
unsigned long flags;
unsigned long want_flags = 0;
if (file->f_flags & O_SYNC)
- want_flags = _PAGE_PCD;
+ want_flags = _PAGE_UC;
#ifdef CONFIG_X86_32
/*
* On the PPro and successors, the MTRRs are used to set
* memory types for physical addresses outside main memory,
-* so blindly setting PCD or PWT on those pages is wrong.
+* so blindly setting UC or PWT on those pages is wrong.
* For Pentiums and earlier, the surround logic should disable
* caching for the high addresses through the KEN pin, but
* we maintain the tradition of paranoia in this code.
@@ -244,7 +244,7 @@
test_bit(X86_FEATURE_CYRIX_ARR, boot_cpu_data.x86_capability) ||
test_bit(X86_FEATURE_CENTAUR_MCR,
boot_cpu_data.x86_capability)) &&
offset >= __pa(high_memory))
- want_flags = _PAGE_PCD;
+ want_flags = _PAGE_UC;
#endif
/* ignore error because we can't handle it here */
Index: linux-2.6.git/arch/x86/pci/i386.c
===
---
Re: Folding _PAGE_PWT into _PAGE_PCD (was Re: unify pagetable accessors patch causes double fault II)
> Good, but isn't the name _PAGE_CACHE misleading? Or does it mean It's all bits related to caching. > something else in the context of PAT? It could mean arbitary things with PAT -- PAT essentially allows to reprogram these bits freely to various different modi using a mapping table. In the default configuration any bit set should resort in a uncacheable mode though. -Andi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Folding _PAGE_PWT into _PAGE_PCD (was Re: unify pagetable accessors patch causes double fault II)
Andi Kleen wrote: BTW, I just noticed that _PAGE_PWT has been folded into _PAGE_PCD. This seems like a really bad idea to me, since it breaks the rule that _PAGE_X == 1 << _PAGE_BIT_X. I can't think of a specific place where this would cause problems, but this kind of non-uniformity always ends up biting someone in the arse. Agreed that it's a bad idea. I think having a specific _PAGE_NOCACHE which combines these bits is a better approach. CPA series adds that already +/* Needs special handling for large pages */ +#define _PAGE_CACHE (_PAGE_PCD|_PAGE_PWT|_PAGE_PAT) Good, but isn't the name _PAGE_CACHE misleading? Or does it mean something else in the context of PAT? J -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Folding _PAGE_PWT into _PAGE_PCD (was Re: unify pagetable accessors patch causes double fault II)
> BTW, I just noticed that _PAGE_PWT has been folded into _PAGE_PCD. This > seems like a really bad idea to me, since it breaks the rule that > _PAGE_X == 1 << _PAGE_BIT_X. I can't think of a specific place where > this would cause problems, but this kind of non-uniformity always ends > up biting someone in the arse. Agreed that it's a bad idea. > > I think having a specific _PAGE_NOCACHE which combines these bits is a > better approach. CPA series adds that already +/* Needs special handling for large pages */ +#define _PAGE_CACHE (_PAGE_PCD|_PAGE_PWT|_PAGE_PAT) + -Andi -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Folding _PAGE_PWT into _PAGE_PCD (was Re: unify pagetable accessors patch causes double fault II)
Ingo Molnar wrote: -#define _PAGE_PRESENT (_AC(1, UL)<<_PAGE_BIT_PRESENT) -#define _PAGE_RW (_AC(1, UL)<<_PAGE_BIT_RW) -#define _PAGE_USER (_AC(1, UL)<<_PAGE_BIT_USER) -#define _PAGE_PWT (_AC(1, UL)<<_PAGE_BIT_PWT) -#define _PAGE_PCD ((_AC(1, UL)<<_PAGE_BIT_PCD) | _PAGE_PWT) BTW, I just noticed that _PAGE_PWT has been folded into _PAGE_PCD. This seems like a really bad idea to me, since it breaks the rule that _PAGE_X == 1 << _PAGE_BIT_X. I can't think of a specific place where this would cause problems, but this kind of non-uniformity always ends up biting someone in the arse. I think having a specific _PAGE_NOCACHE which combines these bits is a better approach. J -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Folding _PAGE_PWT into _PAGE_PCD (was Re: unify pagetable accessors patch causes double fault II)
Ingo Molnar wrote: -#define _PAGE_PRESENT (_AC(1, UL)_PAGE_BIT_PRESENT) -#define _PAGE_RW (_AC(1, UL)_PAGE_BIT_RW) -#define _PAGE_USER (_AC(1, UL)_PAGE_BIT_USER) -#define _PAGE_PWT (_AC(1, UL)_PAGE_BIT_PWT) -#define _PAGE_PCD ((_AC(1, UL)_PAGE_BIT_PCD) | _PAGE_PWT) BTW, I just noticed that _PAGE_PWT has been folded into _PAGE_PCD. This seems like a really bad idea to me, since it breaks the rule that _PAGE_X == 1 _PAGE_BIT_X. I can't think of a specific place where this would cause problems, but this kind of non-uniformity always ends up biting someone in the arse. I think having a specific _PAGE_NOCACHE which combines these bits is a better approach. J -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Folding _PAGE_PWT into _PAGE_PCD (was Re: unify pagetable accessors patch causes double fault II)
BTW, I just noticed that _PAGE_PWT has been folded into _PAGE_PCD. This seems like a really bad idea to me, since it breaks the rule that _PAGE_X == 1 _PAGE_BIT_X. I can't think of a specific place where this would cause problems, but this kind of non-uniformity always ends up biting someone in the arse. Agreed that it's a bad idea. I think having a specific _PAGE_NOCACHE which combines these bits is a better approach. CPA series adds that already +/* Needs special handling for large pages */ +#define _PAGE_CACHE (_PAGE_PCD|_PAGE_PWT|_PAGE_PAT) + -Andi -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Folding _PAGE_PWT into _PAGE_PCD (was Re: unify pagetable accessors patch causes double fault II)
Andi Kleen wrote: BTW, I just noticed that _PAGE_PWT has been folded into _PAGE_PCD. This seems like a really bad idea to me, since it breaks the rule that _PAGE_X == 1 _PAGE_BIT_X. I can't think of a specific place where this would cause problems, but this kind of non-uniformity always ends up biting someone in the arse. Agreed that it's a bad idea. I think having a specific _PAGE_NOCACHE which combines these bits is a better approach. CPA series adds that already +/* Needs special handling for large pages */ +#define _PAGE_CACHE (_PAGE_PCD|_PAGE_PWT|_PAGE_PAT) Good, but isn't the name _PAGE_CACHE misleading? Or does it mean something else in the context of PAT? J -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Folding _PAGE_PWT into _PAGE_PCD (was Re: unify pagetable accessors patch causes double fault II)
Good, but isn't the name _PAGE_CACHE misleading? Or does it mean It's all bits related to caching. something else in the context of PAT? It could mean arbitary things with PAT -- PAT essentially allows to reprogram these bits freely to various different modi using a mapping table. In the default configuration any bit set should resort in a uncacheable mode though. -Andi -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Folding _PAGE_PWT into _PAGE_PCD (was Re: unify pagetable accessors patch causes double fault II)
On Tue, Jan 15, 2008 at 09:16:50AM -0800, Jeremy Fitzhardinge wrote:
Ingo Molnar wrote:
-#define _PAGE_PRESENT (_AC(1, UL)_PAGE_BIT_PRESENT)
-#define _PAGE_RW(_AC(1, UL)_PAGE_BIT_RW)
-#define _PAGE_USER (_AC(1, UL)_PAGE_BIT_USER)
-#define _PAGE_PWT (_AC(1, UL)_PAGE_BIT_PWT)
-#define _PAGE_PCD ((_AC(1, UL)_PAGE_BIT_PCD) | _PAGE_PWT)
BTW, I just noticed that _PAGE_PWT has been folded into _PAGE_PCD. This
seems like a really bad idea to me, since it breaks the rule that
_PAGE_X == 1 _PAGE_BIT_X. I can't think of a specific place where
this would cause problems, but this kind of non-uniformity always ends
up biting someone in the arse.
I think having a specific _PAGE_NOCACHE which combines these bits is a
better approach.
J
How about the patch below. It defines new _PAGE_UC. One concern is drivers
continuing to use _PAGE_PCD and getting wrong attributes. May be we need to
rename _PAGE_PCD to catch those errors as well?
Thanks,
Venki
Do not fold PCD and PWT bits in _PAGE_PCD. Instead, introduce a new
_PAGE_UC which defines uncached mappings and use it in place of _PAGE_PCD.
Signed-off-by: Venkatesh Pallipadi [EMAIL PROTECTED]
Index: linux-2.6.git/arch/x86/mm/ioremap_32.c
===
--- linux-2.6.git.orig/arch/x86/mm/ioremap_32.c 2008-01-15 03:29:38.0
-0800
+++ linux-2.6.git/arch/x86/mm/ioremap_32.c 2008-01-15 04:42:59.0
-0800
@@ -173,7 +173,7 @@
void __iomem *ioremap_nocache (unsigned long phys_addr, unsigned long size)
{
- return __ioremap(phys_addr, size, _PAGE_PCD);
+ return __ioremap(phys_addr, size, _PAGE_UC);
}
EXPORT_SYMBOL(ioremap_nocache);
Index: linux-2.6.git/arch/x86/mm/ioremap_64.c
===
--- linux-2.6.git.orig/arch/x86/mm/ioremap_64.c 2008-01-15 03:29:38.0
-0800
+++ linux-2.6.git/arch/x86/mm/ioremap_64.c 2008-01-15 04:43:07.0
-0800
@@ -150,7 +150,7 @@
void __iomem *ioremap_nocache (unsigned long phys_addr, unsigned long size)
{
- return __ioremap(phys_addr, size, _PAGE_PCD);
+ return __ioremap(phys_addr, size, _PAGE_UC);
}
EXPORT_SYMBOL(ioremap_nocache);
Index: linux-2.6.git/arch/x86/mm/pat.c
===
--- linux-2.6.git.orig/arch/x86/mm/pat.c2008-01-15 03:29:38.0
-0800
+++ linux-2.6.git/arch/x86/mm/pat.c 2008-01-15 05:01:43.0 -0800
@@ -64,7 +64,7 @@
if (smp_processor_id() !pat_wc_enabled)
return;
- /* Set PWT+PCD to Write-Combining. All other bits stay the same */
+ /* Set PCD to Write-Combining. All other bits stay the same */
/* PTE encoding used in Linux:
PAT
|PCD
@@ -72,7 +72,7 @@
|||
000 WB default
010 WC _PAGE_WC
- 011 UC _PAGE_PCD
+ 011 UC _PAGE_UC
PAT bit unused */
pat = PAT(0,WB) | PAT(1,WT) | PAT(2,WC) | PAT(3,UC) |
PAT(4,WB) | PAT(5,WT) | PAT(6,WC) | PAT(7,UC);
@@ -97,7 +97,7 @@
{
switch (flags _PAGE_CACHE_MASK) {
case _PAGE_WC: return write combining;
- case _PAGE_PCD: return uncached;
+ case _PAGE_UC: return uncached;
case 0: return default;
default:return broken;
}
@@ -144,7 +144,7 @@
if (!fattr)
return -EINVAL;
else
- *fattr = _PAGE_PCD;
+ *fattr = _PAGE_UC;
}
return 0;
@@ -227,13 +227,13 @@
unsigned long flags;
unsigned long want_flags = 0;
if (file-f_flags O_SYNC)
- want_flags = _PAGE_PCD;
+ want_flags = _PAGE_UC;
#ifdef CONFIG_X86_32
/*
* On the PPro and successors, the MTRRs are used to set
* memory types for physical addresses outside main memory,
-* so blindly setting PCD or PWT on those pages is wrong.
+* so blindly setting UC or PWT on those pages is wrong.
* For Pentiums and earlier, the surround logic should disable
* caching for the high addresses through the KEN pin, but
* we maintain the tradition of paranoia in this code.
@@ -244,7 +244,7 @@
test_bit(X86_FEATURE_CYRIX_ARR, boot_cpu_data.x86_capability) ||
test_bit(X86_FEATURE_CENTAUR_MCR,
boot_cpu_data.x86_capability))
offset = __pa(high_memory))
- want_flags = _PAGE_PCD;
+ want_flags = _PAGE_UC;
#endif
/* ignore error because we can't handle it here */
Index: linux-2.6.git/arch/x86/pci/i386.c
===
--- linux-2.6.git.orig/arch/x86/pci/i386.c 2008-01-15
Re: Folding _PAGE_PWT into _PAGE_PCD (was Re: unify pagetable accessors patch causes double fault II)
Venki Pallipadi wrote: On Tue, Jan 15, 2008 at 09:16:50AM -0800, Jeremy Fitzhardinge wrote: Ingo Molnar wrote: -#define _PAGE_PRESENT (_AC(1, UL)_PAGE_BIT_PRESENT) -#define _PAGE_RW (_AC(1, UL)_PAGE_BIT_RW) -#define _PAGE_USER (_AC(1, UL)_PAGE_BIT_USER) -#define _PAGE_PWT (_AC(1, UL)_PAGE_BIT_PWT) -#define _PAGE_PCD ((_AC(1, UL)_PAGE_BIT_PCD) | _PAGE_PWT) BTW, I just noticed that _PAGE_PWT has been folded into _PAGE_PCD. This seems like a really bad idea to me, since it breaks the rule that _PAGE_X == 1 _PAGE_BIT_X. I can't think of a specific place where this would cause problems, but this kind of non-uniformity always ends up biting someone in the arse. I think having a specific _PAGE_NOCACHE which combines these bits is a better approach. J How about the patch below. It defines new _PAGE_UC. One concern is drivers continuing to use _PAGE_PCD and getting wrong attributes. May be we need to rename _PAGE_PCD to catch those errors as well? Sure, looks fine. I would have said that _NOCACHE matches current usage better, but if it makes more sense to have _UC and _WC then that's fine with me. I guess renaming _PAGE_BIT_PCD to _PAGE_BIT__PCD and the corresponding _PAGE__PCD might be reasonable if you think there's a chance of new misusers appearing (I guess something like out of tree DRI/proprietary patches are a source of that). J -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
