x86: ioremap: fix wrong physical address handling
in [Kernel]
|
Prev: pcmcia: fix 'driver ... did not release config properly' warning
Next: [PATCH] x86: OLPC: use pr_debug() for EC commands
From: Matthew Wilcox on 16 Jun 2010 23:00 On Thu, Jun 17, 2010 at 10:30:06AM +0900, Kenji Kaneshige wrote: > Index: linux-2.6.34/arch/x86/mm/ioremap.c > =================================================================== > --- linux-2.6.34.orig/arch/x86/mm/ioremap.c 2010-06-15 04:43:00.978332015 +0900 > +++ linux-2.6.34/arch/x86/mm/ioremap.c 2010-06-15 05:32:59.291693007 +0900 > @@ -62,8 +62,8 @@ > static void __iomem *__ioremap_caller(resource_size_t phys_addr, > unsigned long size, unsigned long prot_val, void *caller) > { > - unsigned long pfn, offset, vaddr; > - resource_size_t last_addr; > + unsigned long offset, vaddr; > + resource_size_t pfn, last_pfn, last_addr; I have a hard time understanding this change. pfn is always a physical address shifted by PAGE_SHIFT. So a 32-bit pfn supports up to 44-bit physical addresses. Are your addresses above 44-bits? > @@ -115,7 +113,7 @@ > * Mappings have to be page-aligned > */ > offset = phys_addr & ~PAGE_MASK; > - phys_addr &= PAGE_MASK; > + phys_addr = (phys_addr >> PAGE_SHIFT) << PAGE_SHIFT; I'd rather see PAGE_MASK fixed. Would this work? #define PAGE_SIZE (_AC(1,UL) << PAGE_SHIFT) -#define PAGE_MASK (~(PAGE_SIZE-1)) +#define PAGE_MASK (~(PAGE_SIZE-1ULL)) -- Matthew Wilcox Intel Open Source Technology Centre "Bill, look, we understand that you're interested in selling us this operating system, but compare it to ours. We can't possibly take such a retrograde step." -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: H. Peter Anvin on 17 Jun 2010 00:30 On 06/16/2010 07:50 PM, Matthew Wilcox wrote: > On Thu, Jun 17, 2010 at 10:30:06AM +0900, Kenji Kaneshige wrote: >> Index: linux-2.6.34/arch/x86/mm/ioremap.c >> =================================================================== >> --- linux-2.6.34.orig/arch/x86/mm/ioremap.c 2010-06-15 04:43:00.978332015 +0900 >> +++ linux-2.6.34/arch/x86/mm/ioremap.c 2010-06-15 05:32:59.291693007 +0900 >> @@ -62,8 +62,8 @@ >> static void __iomem *__ioremap_caller(resource_size_t phys_addr, >> unsigned long size, unsigned long prot_val, void *caller) >> { >> - unsigned long pfn, offset, vaddr; >> - resource_size_t last_addr; >> + unsigned long offset, vaddr; >> + resource_size_t pfn, last_pfn, last_addr; > > I have a hard time understanding this change. pfn is always a physical > address shifted by PAGE_SHIFT. So a 32-bit pfn supports up to 44-bit > physical addresses. Are your addresses above 44-bits? > I think they might be. Kenji? -hpa -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Kenji Kaneshige on 17 Jun 2010 01:00 (2010/06/17 13:22), H. Peter Anvin wrote: > On 06/16/2010 07:50 PM, Matthew Wilcox wrote: >> On Thu, Jun 17, 2010 at 10:30:06AM +0900, Kenji Kaneshige wrote: >>> Index: linux-2.6.34/arch/x86/mm/ioremap.c >>> =================================================================== >>> --- linux-2.6.34.orig/arch/x86/mm/ioremap.c 2010-06-15 >>> 04:43:00.978332015 +0900 >>> +++ linux-2.6.34/arch/x86/mm/ioremap.c 2010-06-15 05:32:59.291693007 >>> +0900 >>> @@ -62,8 +62,8 @@ >>> static void __iomem *__ioremap_caller(resource_size_t phys_addr, >>> unsigned long size, unsigned long prot_val, void *caller) >>> { >>> - unsigned long pfn, offset, vaddr; >>> - resource_size_t last_addr; >>> + unsigned long offset, vaddr; >>> + resource_size_t pfn, last_pfn, last_addr; >> >> I have a hard time understanding this change. pfn is always a physical >> address shifted by PAGE_SHIFT. So a 32-bit pfn supports up to 44-bit >> physical addresses. Are your addresses above 44-bits? >> > > I think they might be. Kenji? No. My addresses are in the 44-bits range (around fc000000000). So it is not required for my problem. This change assumes that phys_addr can be above 44-bits (up to 52-bits (and higher in the future?)). By the way, is there linux kernel limit regarding above 44-bits physical address in x86_32 PAE? For example, pfn above 32-bits is not supported? #ifdef CONFIG_X86_PAE /* 44=32+12, the limit we can fit into an unsigned long pfn */ #define __PHYSICAL_MASK_SHIFT 44 #define __VIRTUAL_MASK_SHIFT 32 If there is 44-bits physical address limit, I think it's better to use PHYSICAL_PAGE_MASK for masking physical address, instead of "(phys_addr >> PAGE_SHIFT) << PAGE_SHIFT)". The PHYSICAL_PAGE_MASK would become greater value when 44-bits physical address limit is eliminated. And maybe we need to change phys_addr_valid() returns error if physical address is above (1 << __PHYSICAL_MASK_SHIFT)? Thanks, Kenji Kaneshige -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: H. Peter Anvin on 17 Jun 2010 02:10 On 06/16/2010 09:55 PM, Kenji Kaneshige wrote: >> >> I think they might be. Kenji? > > No. My addresses are in the 44-bits range (around fc000000000). So it is > not required for my problem. This change assumes that phys_addr can be > above 44-bits (up to 52-bits (and higher in the future?)). > > By the way, is there linux kernel limit regarding above 44-bits physical > address in x86_32 PAE? For example, pfn above 32-bits is not supported? > There are probably places at which PFNs are held in 32-bit numbers, although it would be good to track them down if it isn't too expensive to fix them (i.e. doesn't affect generic code.) This also affects paravirt systems, i.e. right now Xen has to locate all 32-bit guests below 64 GB, which limits its usefulness. > #ifdef CONFIG_X86_PAE > /* 44=32+12, the limit we can fit into an unsigned long pfn */ > #define __PHYSICAL_MASK_SHIFT 44 > #define __VIRTUAL_MASK_SHIFT 32 > > If there is 44-bits physical address limit, I think it's better to use > PHYSICAL_PAGE_MASK for masking physical address, instead of "(phys_addr >>> PAGE_SHIFT) << PAGE_SHIFT)". The PHYSICAL_PAGE_MASK would become > greater value when 44-bits physical address limit is eliminated. And > maybe we need to change phys_addr_valid() returns error if physical > address is above (1 << __PHYSICAL_MASK_SHIFT)? The real question is how much we can fix without an unreasonable cost. -hpa -- H. Peter Anvin, Intel Open Source Technology Center I work for Intel. I don't speak on their behalf. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Kenji Kaneshige on 17 Jun 2010 02:30
(2010/06/17 11:50), Matthew Wilcox wrote: > On Thu, Jun 17, 2010 at 10:30:06AM +0900, Kenji Kaneshige wrote: >> Index: linux-2.6.34/arch/x86/mm/ioremap.c >> =================================================================== >> --- linux-2.6.34.orig/arch/x86/mm/ioremap.c 2010-06-15 04:43:00.978332015 +0900 >> +++ linux-2.6.34/arch/x86/mm/ioremap.c 2010-06-15 05:32:59.291693007 +0900 >> @@ -62,8 +62,8 @@ >> static void __iomem *__ioremap_caller(resource_size_t phys_addr, >> unsigned long size, unsigned long prot_val, void *caller) >> { >> - unsigned long pfn, offset, vaddr; >> - resource_size_t last_addr; >> + unsigned long offset, vaddr; >> + resource_size_t pfn, last_pfn, last_addr; > > I have a hard time understanding this change. pfn is always a physical > address shifted by PAGE_SHIFT. So a 32-bit pfn supports up to 44-bit > physical addresses. Are your addresses above 44-bits? > >> @@ -115,7 +113,7 @@ >> * Mappings have to be page-aligned >> */ >> offset = phys_addr& ~PAGE_MASK; >> - phys_addr&= PAGE_MASK; >> + phys_addr = (phys_addr>> PAGE_SHIFT)<< PAGE_SHIFT; > > I'd rather see PAGE_MASK fixed. Would this work? > > #define PAGE_SIZE (_AC(1,UL)<< PAGE_SHIFT) > -#define PAGE_MASK (~(PAGE_SIZE-1)) > +#define PAGE_MASK (~(PAGE_SIZE-1ULL)) > I think it should work. But I'm worrying about regressions. Now I think using PHYSICAL_PAGE_MASK (as my v.1 patch did) is good idea again. What do you think about this? Thanks, Kenji Kaneshige -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |