This patch fixes a bug of ioremap_nocache. ioremap_nocache() will call
__ioremap() with flags != 0 to do the real work, which will call
change_page_attr_addr() if phys_addr + size - 1 < (end_pfn_map << PAGE_SHIFT).
But some pages between 0 ~ end_pfn_map << PAGE_SHIFT are not mapped by
identity map, this will make change_page_attr_addr failed.
This patch is based on latest x86 git and has been tested on x86_64 platform.
Signed-off-by: Huang Ying <[email protected]>
---
arch/x86/mm/ioremap_64.c | 7 +++++++
1 file changed, 7 insertions(+)
--- a/arch/x86/mm/ioremap_64.c
+++ b/arch/x86/mm/ioremap_64.c
@@ -41,8 +41,15 @@ ioremap_change_attr(unsigned long phys_a
if (phys_addr + size - 1 < (end_pfn_map << PAGE_SHIFT)) {
unsigned long npages = (size + PAGE_SIZE - 1) >> PAGE_SHIFT;
unsigned long vaddr = (unsigned long) __va(phys_addr);
+ int level;
/*
+ * If there is no identity map for this address,
+ * change_page_attr_addr is unnecessary
+ */
+ if (!lookup_address(vaddr, &level))
+ return err;
+ /*
* Must use a address here and not struct page because the phys addr
* can be a in hole between nodes and not have an memmap entry.
*/
* Huang, Ying <[email protected]> wrote:
> This patch fixes a bug of ioremap_nocache. ioremap_nocache() will call
> __ioremap() with flags != 0 to do the real work, which will call
> change_page_attr_addr() if phys_addr + size - 1 < (end_pfn_map <<
> PAGE_SHIFT). But some pages between 0 ~ end_pfn_map << PAGE_SHIFT are
> not mapped by identity map, this will make change_page_attr_addr
> failed.
very interesting! Is this in response to a bug you've triggered?
i believe the scenario you outlne could trigger on 64-bit boxen, if they
try to ioremap an area not covered by the direct ptes:
> @@ -41,8 +41,15 @@ ioremap_change_attr(unsigned long phys_a
> if (phys_addr + size - 1 < (end_pfn_map << PAGE_SHIFT)) {
> unsigned long npages = (size + PAGE_SIZE - 1) >> PAGE_SHIFT;
> unsigned long vaddr = (unsigned long) __va(phys_addr);
> + int level;
>
> /*
> + * If there is no identity map for this address,
> + * change_page_attr_addr is unnecessary
> + */
> + if (!lookup_address(vaddr, &level))
> + return err;
> + /*
and we'd incorrectly fail the ioremap() with -EINVAL, and return a hard
error to the driver - and causing broken boxes, right?
Thomas has unified most of ioremap*.c as well, and we'll make sure this
fix survives the unification. The 64-bit code limped along before by
accident, because the (pre-cleanup) 64-bit __change_page_attr() code
incorrectly returned 0 for invalid ranges:
kpte = lookup_address(address);
if (!kpte) return 0;
which masked the bug you fixed now. 32-bit always returned -EINVAL on
invalid ranges.
Ingo
On Jan 25, 2008 6:27 PM, Ingo Molnar <[email protected]> wrote:
>
> * Huang, Ying <[email protected]> wrote:
>
> > This patch fixes a bug of ioremap_nocache. ioremap_nocache() will call
> > __ioremap() with flags != 0 to do the real work, which will call
> > change_page_attr_addr() if phys_addr + size - 1 < (end_pfn_map <<
> > PAGE_SHIFT). But some pages between 0 ~ end_pfn_map << PAGE_SHIFT are
> > not mapped by identity map, this will make change_page_attr_addr
> > failed.
>
> very interesting! Is this in response to a bug you've triggered?
>
> i believe the scenario you outlne could trigger on 64-bit boxen, if they
> try to ioremap an area not covered by the direct ptes:
Yes. The efifb driver try to ioremap 0x40000000 on my 64-bit box and
it failed because 0x40000000 < (end_pfn_map << PAGE_SHIFT) and it is
not covered by direct ptes.
> > @@ -41,8 +41,15 @@ ioremap_change_attr(unsigned long phys_a
> > if (phys_addr + size - 1 < (end_pfn_map << PAGE_SHIFT)) {
> > unsigned long npages = (size + PAGE_SIZE - 1) >> PAGE_SHIFT;
> > unsigned long vaddr = (unsigned long) __va(phys_addr);
> > + int level;
> >
> > /*
> > + * If there is no identity map for this address,
> > + * change_page_attr_addr is unnecessary
> > + */
> > + if (!lookup_address(vaddr, &level))
> > + return err;
> > + /*
>
> and we'd incorrectly fail the ioremap() with -EINVAL, and return a hard
> error to the driver - and causing broken boxes, right?
Yes.
> Thomas has unified most of ioremap*.c as well, and we'll make sure this
> fix survives the unification. The 64-bit code limped along before by
> accident, because the (pre-cleanup) 64-bit __change_page_attr() code
> incorrectly returned 0 for invalid ranges:
>
> kpte = lookup_address(address);
> if (!kpte) return 0;
>
> which masked the bug you fixed now. 32-bit always returned -EINVAL on
> invalid ranges.
Best Regards,
Huang Ying