Messages in this thread Patch in this message | | | Date | Fri, 25 Jan 2008 21:50:14 +0800 | From | "huang ying" <> | Subject | Re: [PATCH] x86: ioremap_nocache fix |
| |
On Jan 25, 2008 6:27 PM, Ingo Molnar <mingo@elte.hu> wrote: > > * Huang, Ying <ying.huang@intel.com> 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
| |