On Wednesday 09 January 2008 11:47:11 Ingo Molnar wrote:
> * Andi Kleen <[EMAIL PROTECTED]> wrote:
> > There's one Jan pointed out: iounmap does not subtract the guard page
> > size so it ends up resetting one page too much. That is probably what
> > causes your problem. But again you should be pas
* Jeremy Fitzhardinge <[EMAIL PROTECTED]> wrote:
> OK, great. Ingo, that means we can use this and go back to folding
> _PAGE_GLOBAL into __PAGE_KERNEL_*. Well, at least give it a try.
ok but please send a delta patch for that - the current stuff is
reasonably stable for the time being.
* Andi Kleen <[EMAIL PROTECTED]> wrote:
> There's one Jan pointed out: iounmap does not subtract the guard page
> size so it ends up resetting one page too much. That is probably what
> causes your problem. But again you should be passing in G in the first
> place.
>
> -Andi
>
> Here was Jan
>The "problem" is a BUG() in pageattr_64.c:change_page_attr(), which to
>me looks spurious. It arises because __PAGE_KERNEL_* doesn't contain
>_PAGE_GLOBAL, but PAGE_KERNEL_* does. When ioremap()
>change_page_attr(), it does so in a way that guarentees that the test
>
> if (pgprot_val(pr
Andi Kleen wrote:
Yeah, that may be true, but this particular tree is weird, and I'm trying
to understand what's going on here. Specifically, 64-bit ioremap()s
*don't* set _PAGE_GLOBAL, which appears to be an accident resulting from
the strange definitions of __PAGE_KERNEL_* vs PAGE_KERNEL_*.
> Yeah, that may be true, but this particular tree is weird, and I'm trying
> to understand what's going on here. Specifically, 64-bit ioremap()s
> *don't* set _PAGE_GLOBAL, which appears to be an accident resulting from
> the strange definitions of __PAGE_KERNEL_* vs PAGE_KERNEL_*.
ioremap()
Andi Kleen wrote:
Or to put it another way, what's the underlying rationale for making
__PAGE_KERNEL_* not include the GLOBAL flag, but including it in the pgprot
versions? It means that code like this in ioremap_64.c:
There is none, but that is not what change_page_attr() cares about.
I
On Tue, Jan 08, 2008 at 05:35:26PM -0800, Jeremy Fitzhardinge wrote:
> Andi Kleen wrote:
>>> of the if()? If not, can't we just remove it and avoid this present
>>> problem?
>>>
>>
>> That would just hide your problem.
>>
>
> The "problem" is a BUG() in pageattr_64.c:change_page_attr(), w
Andi Kleen wrote:
of the if()? If not, can't we just remove it and avoid this present
problem?
That would just hide your problem.
The "problem" is a BUG() in pageattr_64.c:change_page_attr(), which to
me looks spurious. It arises because __PAGE_KERNEL_* doesn't contain
_PAGE_GLOBA
* Jeremy Fitzhardinge <[EMAIL PROTECTED]> wrote:
>>> +#define GLOBAL_PGPROT(prot)__pgprot(prot | _PAGE_GLOBAL)
>>> +
>>> +#define PAGE_KERNELGLOBAL_PGPROT(__PAGE_KERNEL)
>>> +#define PAGE_KERNEL_RO GLOBAL_PGPROT(__PAGE_KERNEL_RO)
>>> +#define PA
On Wed, Jan 09, 2008 at 02:22:57AM +0100, Ingo Molnar wrote:
>
> * Andi Kleen <[EMAIL PROTECTED]> wrote:
>
> > On Wed, Jan 09, 2008 at 02:16:09AM +0100, Ingo Molnar wrote:
> > >
> > > * Jeremy Fitzhardinge <[EMAIL PROTECTED]> wrote:
> > >
> > > > This isn't correct, because it will set _PAGE_GL
* Andi Kleen <[EMAIL PROTECTED]> wrote:
> On Wed, Jan 09, 2008 at 02:16:09AM +0100, Ingo Molnar wrote:
> >
> > * Jeremy Fitzhardinge <[EMAIL PROTECTED]> wrote:
> >
> > > This isn't correct, because it will set _PAGE_GLOBAL on 32-bit
> > > unconditionally. It needs to be something like:
> >
>
Ingo Molnar wrote:
* Jeremy Fitzhardinge <[EMAIL PROTECTED]> wrote:
This isn't correct, because it will set _PAGE_GLOBAL on 32-bit
unconditionally. It needs to be something like:
hm, why is that a problem? Ah, the VDSO page must not be global-mapped,
right? It's getting late here, i
* Andi Kleen <[EMAIL PROTECTED]> wrote:
> >> this code is plain buggy but fixing it triggered driver bugs in the
> >> past so we've been procrastinating it forever ...
> >>
> >
> > What kind of driver bugs? Do drivers manage to get into the other
> > branch
>
> That was just the return val
On Wed, Jan 09, 2008 at 02:16:09AM +0100, Ingo Molnar wrote:
>
> * Jeremy Fitzhardinge <[EMAIL PROTECTED]> wrote:
>
> > This isn't correct, because it will set _PAGE_GLOBAL on 32-bit
> > unconditionally. It needs to be something like:
>
> hm, why is that a problem?
Not so ancient 32bit CPUs
* Jeremy Fitzhardinge <[EMAIL PROTECTED]> wrote:
> This isn't correct, because it will set _PAGE_GLOBAL on 32-bit
> unconditionally. It needs to be something like:
hm, why is that a problem? Ah, the VDSO page must not be global-mapped,
right? It's getting late here, i should stop modifying th
On Tue, Jan 08, 2008 at 05:07:08PM -0800, Jeremy Fitzhardinge wrote:
> Ingo Molnar wrote:
>> * Jeremy Fitzhardinge <[EMAIL PROTECTED]> wrote:
>>
>>
>>> In other words, prot and ref_prot can never be equal, so this path is
>>> always taken, and the other branch which tests pte_huge() is never ru
Ingo Molnar wrote:
* Ingo Molnar <[EMAIL PROTECTED]> wrote:
* Ingo Molnar <[EMAIL PROTECTED]> wrote:
#define __PAGE_KERNEL_EXEC \
- (_PAGE_PRESENT | _PAGE_RW | _PAGE_DIRTY | _PAGE_ACCESSED | _PAGE_GLOBAL)
+ (_PAGE_PRESENT | _PAG
> Is _PAGE_GLOBAL causing the first if() to fall through to the second
Yes it obviously is.
> clause? Because otherwise it shouldn't have any effect on the pte_huge()
> test.
>
> Gah! This can't be right! I think the original change_page_attr() code is
> plain buggy.
It has a few rough e
Ingo Molnar wrote:
* Jeremy Fitzhardinge <[EMAIL PROTECTED]> wrote:
In other words, prot and ref_prot can never be equal, so this path is
always taken, and the other branch which tests pte_huge() is never
run.
Andi? Jan? Is this code just buggy, or is there something else going
on here
* Jeremy Fitzhardinge <[EMAIL PROTECTED]> wrote:
> In other words, prot and ref_prot can never be equal, so this path is
> always taken, and the other branch which tests pte_huge() is never
> run.
>
> Andi? Jan? Is this code just buggy, or is there something else going
> on here?
this code
Ingo Molnar wrote:
* Ingo Molnar <[EMAIL PROTECTED]> wrote:
* Ingo Molnar <[EMAIL PROTECTED]> wrote:
#define __PAGE_KERNEL_EXEC \
- (_PAGE_PRESENT | _PAGE_RW | _PAGE_DIRTY | _PAGE_ACCESSED | _PAGE_GLOBAL)
+ (_PAGE_PRESENT | _PAG
Ingo Molnar wrote:
* Ingo Molnar <[EMAIL PROTECTED]> wrote:
#define __PAGE_KERNEL_EXEC \
- (_PAGE_PRESENT | _PAGE_RW | _PAGE_DIRTY | _PAGE_ACCESSED | _PAGE_GLOBAL)
+ (_PAGE_PRESENT | _PAGE_RW | _PAGE_DIRTY | _PAGE_ACCESSED)
* Ingo Molnar <[EMAIL PROTECTED]> wrote:
>
> * Ingo Molnar <[EMAIL PROTECTED]> wrote:
>
> > > >>> #define __PAGE_KERNEL_EXEC
> > > >>> \
> > > >>> - (_PAGE_PRESENT | _PAGE_RW | _PAGE_DIRTY | _PAGE_ACCESSED |
> > > >>> _PAGE_GLOBAL)
> > > >>> +
* Ingo Molnar <[EMAIL PROTECTED]> wrote:
> > >>> #define __PAGE_KERNEL_EXEC
> > >>> \
> > >>> - (_PAGE_PRESENT | _PAGE_RW | _PAGE_DIRTY | _PAGE_ACCESSED |
> > >>> _PAGE_GLOBAL)
> > >>> + (_PAGE_PRESENT | _PAGE_RW | _PAGE_DIRTY | _PAGE_ACC
> >>> #define __PAGE_KERNEL_EXEC
> >>> \
> >>> - (_PAGE_PRESENT | _PAGE_RW | _PAGE_DIRTY | _PAGE_ACCESSED | _PAGE_GLOBAL)
> >>> + (_PAGE_PRESENT | _PAGE_RW | _PAGE_DIRTY | _PAGE_ACCESSED)
> >>>
> >
> > This shouldn't be necessary. The old 64-bi
* Jeremy Fitzhardinge <[EMAIL PROTECTED]> wrote:
> Ingo Molnar wrote:
>> * Ingo Molnar <[EMAIL PROTECTED]> wrote:
>>
>>
>>> #define __PAGE_KERNEL_EXEC \
>>> - (_PAGE_PRESENT | _PAGE_RW | _PAGE_DIRTY | _PAGE_ACCESSED | _PAGE_GLOBAL)
>>> + (_PAGE_PRES
Ingo Molnar wrote:
* Ingo Molnar <[EMAIL PROTECTED]> wrote:
#define __PAGE_KERNEL_EXEC \
- (_PAGE_PRESENT | _PAGE_RW | _PAGE_DIRTY | _PAGE_ACCESSED | _PAGE_GLOBAL)
+ (_PAGE_PRESENT | _PAGE_RW | _PAGE_DIRTY | _PAGE_ACCESSED)
This
Ingo Molnar wrote:
right now i'm trying to bisect this crash on 64-bit:
Calling initcall 0x80395877: pci_init+0x0/0x2b()
[ cut here ]
kernel BUG at arch/x86/mm/pageattr_64.c:176!
[...]
Call Trace:
[] change_page_attr_addr+0x9e/0x119
[] ioremap_change_attr
* Ingo Molnar <[EMAIL PROTECTED]> wrote:
> #define __PAGE_KERNEL_EXEC \
> - (_PAGE_PRESENT | _PAGE_RW | _PAGE_DIRTY | _PAGE_ACCESSED | _PAGE_GLOBAL)
> + (_PAGE_PRESENT | _PAGE_RW | _PAGE_DIRTY | _PAGE_ACCESSED)
btw., __PAGE_KERNEL_EXEC should pr
* Ingo Molnar <[EMAIL PROTECTED]> wrote:
> > it's one of these patches:
> >
> > Subject: x86: move all asm/pgtable constants into one place
> > Subject: x86: avoid name conflict for Voyager leave_mm
> > Subject: x86/pgtable: unify pagetable accessors
>
> it's the first one that causes the cr
* Ingo Molnar <[EMAIL PROTECTED]> wrote:
> it's one of these patches:
>
> Subject: x86: move all asm/pgtable constants into one place
> Subject: x86: avoid name conflict for Voyager leave_mm
> Subject: x86/pgtable: unify pagetable accessors
it's the first one that causes the crash.
reviewed
Ingo Molnar wrote:
threw this into the random test setup: it found the attached config
after a few iterations, which needed the fix below. (config builds and
boots fine with this fix)
the problem was that set_pud() is used by
include/asm-generic/pgtable-nopud.h, to build set_pgd():
#define
* Ingo Molnar <[EMAIL PROTECTED]> wrote:
> > Hi Ingo,
> >
> > Here's a series which goes some way to tidying up pgtable.h. It is
> > not complete yet, but this lot compiles and works for me.
> >
> > In many cases I've moved code around with no attempt at adjusting
> > the formatting to accep
* Jeremy Fitzhardinge <[EMAIL PROTECTED]> wrote:
> Hi Ingo,
>
> Here's a series which goes some way to tidying up pgtable.h. It is
> not complete yet, but this lot compiles and works for me.
>
> In many cases I've moved code around with no attempt at adjusting the
> formatting to acceptible
Hi Ingo,
Here's a series which goes some way to tidying up pgtable.h. It is
not complete yet, but this lot compiles and works for me.
In many cases I've moved code around with no attempt at adjusting the
formatting to acceptible style. That can be done in a later patch,
but it means that checkp
36 matches
Mail list logo
