Date: Fri, 25 Jan 2008 11:27:38 +0100
From: Ingo Molnar <mingo@elte.hu>
To: "Huang, Ying" <ying.huang@intel.com>
Cc: Ingo Molnar <mingo@redhat.com>, "H. Peter Anvin" <hpa@zytor.com>,
       Thomas Gleixner <tglx@linutronix.de>, Andi Kleen <ak@suse.de>,
       linux-kernel@vger.kernel.org
Subject: Re: [PATCH] x86: ioremap_nocache fix
Message-ID: <20080125102738.GQ23708@elte.hu>
References: <1201238561.15972.17.camel@caritas-dev.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1201238561.15972.17.camel@caritas-dev.intel.com>
User-Agent: Mutt/1.5.17 (2007-11-01)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1854
Lines: 49


* 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:

> @@ -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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/