Hello,
I'm looking into a customer issue where they hit a kernel BUG when
starting X with our driver (attached logfile). This only occurs with
2.6.12 kernels.
this appears to be the 'vmalloc guard page causing change_page_attr
problems' bug. at one point, iounmap subtracted the guard page before
calling change_page_attr, but I see this was reverted as part of a
recent cleanup.
in this case, we're remapping a single page of the extended pci config
space. note that in the log RAX is the the physical address
00000000e00001e3, but the stack indicates that __change_page_attr was
called with address ffff8100e0001000 and pfn 00000000000e0001.
from looking at the implementation in 2.6.12-pre4, I'm not clear how
the guard page is accounted for in iounmap. in vmalloc.c, the guard
page is subtracted from the vm_struct in remove_vm_area (which calls
unmap_vm_area). but iounmap in ioremap.c calls unmap_vm_area directly
rather than calling remove_vm_area, so the guard page is never
subtracted and the wrong size is passed to change_page_attr.
is the intent that iounmap should call remove_vm_area rather than
unmap_vm_area (with additional changes to not unlink the vma itself)?
or that the guard page should be removed by unmap_ rather than
remove_?
when debugging this issue, I also ran into problems with iounmap using
virt_to_page on a pci IO region. this problem went away when I tried
calling change_page_attr_addr with the virtual address instead. but
perhaps iounmap should be calling ioremap_change_attr rather than
change_page_attr directly. I'll run some additional tests and send out
a patch.
Thanks,
Terence
Terence Ripperda <[email protected]> writes:
> this appears to be the 'vmalloc guard page causing change_page_attr
> problems' bug. at one point, iounmap subtracted the guard page before
> calling change_page_attr, but I see this was reverted as part of a
> recent cleanup.
Hmm, yes looks like it was reintroduced.
> from looking at the implementation in 2.6.12-pre4, I'm not clear how
I suppose you mean rc4, not pre4?
> the guard page is accounted for in iounmap. in vmalloc.c, the guard
> page is subtracted from the vm_struct in remove_vm_area (which calls
> unmap_vm_area). but iounmap in ioremap.c calls unmap_vm_area directly
> rather than calling remove_vm_area, so the guard page is never
> subtracted and the wrong size is passed to change_page_attr.
Ok obviously needs to be fixed.
>
> is the intent that iounmap should call remove_vm_area rather than
> unmap_vm_area (with additional changes to not unlink the vma itself)?
> or that the guard page should be removed by unmap_ rather than
> remove_?
There doesn't seem to be a clear rule, that is where the confusion
comes from I guess. I would consider it cleaner to handle it in
the higher level vmalloc code.
>
> when debugging this issue, I also ran into problems with iounmap using
> virt_to_page on a pci IO region. this problem went away when I tried
> calling change_page_attr_addr with the virtual address instead. but
A patch for that already went into mainline.
> perhaps iounmap should be calling ioremap_change_attr rather than
What is ioremap_change_attr?
> change_page_attr directly. I'll run some additional tests and send out
> a patch.
-Andi
On Thu, May 19, 2005 at 01:29:01AM +0200, [email protected] wrote:
> > from looking at the implementation in 2.6.12-pre4, I'm not clear how
>
> I suppose you mean rc4, not pre4?
yes, just confused.
> > is the intent that iounmap should call remove_vm_area rather than
> > unmap_vm_area (with additional changes to not unlink the vma itself)?
> > or that the guard page should be removed by unmap_ rather than
> > remove_?
>
> There doesn't seem to be a clear rule, that is where the confusion
> comes from I guess. I would consider it cleaner to handle it in
> the higher level vmalloc code.
that certainly sounds reasonable. I'll try putting together a patch
that does this and send it to our end user to see if it fixes his
problem.
> > when debugging this issue, I also ran into problems with iounmap using
> > virt_to_page on a pci IO region. this problem went away when I tried
> > calling change_page_attr_addr with the virtual address instead. but
>
> A patch for that already went into mainline.
ok, great.
> > perhaps iounmap should be calling ioremap_change_attr rather than
>
> What is ioremap_change_attr?
a static function in ioremap.c that is called by __ioremap. it's a
wrapper function around change_page_attr. I only see it in the x86_64
architecture, not i386.
Thanks,
Terence
On Wed, May 18, 2005 at 06:34:25PM -0500, Terence Ripperda wrote:
> > > perhaps iounmap should be calling ioremap_change_attr rather than
> >
> > What is ioremap_change_attr?
>
> a static function in ioremap.c that is called by __ioremap. it's a
> wrapper function around change_page_attr. I only see it in the x86_64
> architecture, not i386.
Oh yes, indeed. I forgot that I wrote it :) Calling global_flush_tlb
in iounmap is indeed a very good idea.
-Andi