[PATCH 0 of 3] define and use phys_addr_t

[PATCH 0 of 3] define and use phys_addr_t

[Jeremy Fitzhardinge]

This series:
 - adds a globally defined phys_addr_t, replacing the
   arch-specific x86 and powerpc ones
 - makes PFN_PHYS return a phys_addr_t
 - redefines resource_size_t as phys_addr_t

The first two are bugfixes and should be applied to git.

Thanks,
	J

[PATCH 1 of 3] add phys_addr_t for holding physical addresses

[Jeremy Fitzhardinge]

Add a kernel-wide "phys_addr_t" which is guaranteed to be able to hold
any physical address.  By default it equals the word size of the
architecture, but a 32-bit architecture can set ARCH_PHYS_ADDR_T_64BIT
if it needs a 64-bit phys_addr_t.

Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 arch/powerpc/Kconfig             |    3 +++
 arch/powerpc/include/asm/types.h |    7 -------
 arch/x86/Kconfig                 |    3 +++
 include/asm-x86/page_32.h        |    2 --
 include/asm-x86/page_64.h        |    1 -
 include/linux/types.h            |    6 ++++++
 mm/Kconfig                       |    3 +++
 7 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -21,6 +21,9 @@
 
 config PPC_MERGE
 	def_bool y
+
+config ARCH_PHYS_ADDR_T_64BIT
+       def_bool PPC64 || PHYS_64BIT
 
 config MMU
 	bool
diff --git a/arch/powerpc/include/asm/types.h b/arch/powerpc/include/asm/types.h
--- a/arch/powerpc/include/asm/types.h
+++ b/arch/powerpc/include/asm/types.h
@@ -48,13 +48,6 @@
 
 typedef __vector128 vector128;
 
-/* Physical address used by some IO functions */
-#if defined(CONFIG_PPC64) || defined(CONFIG_PHYS_64BIT)
-typedef u64 phys_addr_t;
-#else
-typedef u32 phys_addr_t;
-#endif
-
 #ifdef __powerpc64__
 typedef u64 dma_addr_t;
 #else
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -938,6 +938,9 @@
 	  has the cost of more pagetable lookup overhead, and also
 	  consumes more pagetable space per process.
 
+config ARCH_PHYS_ADDR_T_64BIT
+       def_bool X86_64 || X86_PAE
+
 # Common NUMA Features
 config NUMA
 	bool "Numa Memory Allocation and Scheduler Support (EXPERIMENTAL)"
diff --git a/include/asm-x86/page_32.h b/include/asm-x86/page_32.h
--- a/include/asm-x86/page_32.h
+++ b/include/asm-x86/page_32.h
@@ -33,7 +33,6 @@
 typedef u64	pudval_t;
 typedef u64	pgdval_t;
 typedef u64	pgprotval_t;
-typedef u64	phys_addr_t;
 
 typedef union {
 	struct {
@@ -54,7 +53,6 @@
 typedef unsigned long	pudval_t;
 typedef unsigned long	pgdval_t;
 typedef unsigned long	pgprotval_t;
-typedef unsigned long	phys_addr_t;
 
 typedef union {
 	pteval_t pte;
diff --git a/include/asm-x86/page_64.h b/include/asm-x86/page_64.h
--- a/include/asm-x86/page_64.h
+++ b/include/asm-x86/page_64.h
@@ -79,7 +79,6 @@
 typedef unsigned long	pudval_t;
 typedef unsigned long	pgdval_t;
 typedef unsigned long	pgprotval_t;
-typedef unsigned long	phys_addr_t;
 
 typedef struct page *pgtable_t;
 
diff --git a/include/linux/types.h b/include/linux/types.h
--- a/include/linux/types.h
+++ b/include/linux/types.h
@@ -197,6 +197,12 @@
 typedef u32 resource_size_t;
 #endif
 
+#ifdef CONFIG_PHYS_ADDR_T_64BIT
+typedef u64 phys_addr_t;
+#else
+typedef u32 phys_addr_t;
+#endif
+
 struct ustat {
 	__kernel_daddr_t	f_tfree;
 	__kernel_ino_t		f_tinode;
diff --git a/mm/Kconfig b/mm/Kconfig
--- a/mm/Kconfig
+++ b/mm/Kconfig
@@ -187,6 +187,9 @@
 	help
 	  This option allows memory and IO resources to be 64 bit.
 
+config PHYS_ADDR_T_64BIT
+	def_bool 64BIT || ARCH_PHYS_ADDR_T_64BIT
+
 config ZONE_DMA_FLAG
 	int
 	default "0" if !ZONE_DMA

Re: [PATCH 1 of 3] add phys_addr_t for holding physical addresses

[Andrew Morton]

On Tue, 19 Aug 2008 13:02:50 -0700
Jeremy Fitzhardinge <jeremy@goop.org> wrote:

> Add a kernel-wide "phys_addr_t" which is guaranteed to be able to hold
> any physical address.  By default it equals the word size of the
> architecture, but a 32-bit architecture can set ARCH_PHYS_ADDR_T_64BIT
> if it needs a 64-bit phys_addr_t.
> 

You say this is a bugfix but you don't describe the bug.  This makes it
rather hard to make the 2.6.2[5678] decisions.

Ditto on [patch 2/3].

Re: [PATCH 1 of 3] add phys_addr_t for holding physical addresses

[Jeremy Fitzhardinge]

Andrew Morton wrote:
> On Tue, 19 Aug 2008 13:02:50 -0700
> Jeremy Fitzhardinge <jeremy@goop.org> wrote:
>
>   
>> Add a kernel-wide "phys_addr_t" which is guaranteed to be able to hold
>> any physical address.  By default it equals the word size of the
>> architecture, but a 32-bit architecture can set ARCH_PHYS_ADDR_T_64BIT
>> if it needs a 64-bit phys_addr_t.
>>
>>     
>
> You say this is a bugfix but you don't describe the bug.  This makes it
> rather hard to make the 2.6.2[5678] decisions.
>
> Ditto on [patch 2/3].
>   

1/3 is not a bugfix in itself, but a pre-requisite for 2/3.

2/3 replaces an ad-hoc Xen fix with a general fix to prevent address
truncation when using PFN_PHYS() on any PFN above the 4G mark.  The Xen
crash is the only bug I know of that's directly attributable to this,
and it was already addressed in older kernels with the casts in the Xen
code that this patch removes.

So I don't think there's any strong need to push this to earlier kernels.

    J

Re: [PATCH 1 of 3] add phys_addr_t for holding physical addresses

[Andrew Morton]

On Fri, 22 Aug 2008 14:11:16 -0700
Jeremy Fitzhardinge <jeremy@goop.org> wrote:

> Andrew Morton wrote:
> > On Tue, 19 Aug 2008 13:02:50 -0700
> > Jeremy Fitzhardinge <jeremy@goop.org> wrote:
> >
> >   
> >> Add a kernel-wide "phys_addr_t" which is guaranteed to be able to hold
> >> any physical address.  By default it equals the word size of the
> >> architecture, but a 32-bit architecture can set ARCH_PHYS_ADDR_T_64BIT
> >> if it needs a 64-bit phys_addr_t.
> >>
> >>     
> >
> > You say this is a bugfix but you don't describe the bug.  This makes it
> > rather hard to make the 2.6.2[5678] decisions.
> >
> > Ditto on [patch 2/3].
> >   
> 
> 1/3 is not a bugfix in itself, but a pre-requisite for 2/3.
> 
> 2/3 replaces an ad-hoc Xen fix with a general fix to prevent address
> truncation when using PFN_PHYS() on any PFN above the 4G mark.  The Xen
> crash is the only bug I know of that's directly attributable to this,
> and it was already addressed in older kernels with the casts in the Xen
> code that this patch removes.
> 
> So I don't think there's any strong need to push this to earlier kernels.
> 

Still confused.  The above implies that 2.6.27 doesn't need fixing
either, because the typecasts already avoid the crash.

Re: [PATCH 1 of 3] add phys_addr_t for holding physical addresses

[Jeremy Fitzhardinge]

Andrew Morton wrote:
> On Fri, 22 Aug 2008 14:11:16 -0700
> Jeremy Fitzhardinge <jeremy@goop.org> wrote:
>
>   
>> Andrew Morton wrote:
>>     
>>> On Tue, 19 Aug 2008 13:02:50 -0700
>>> Jeremy Fitzhardinge <jeremy@goop.org> wrote:
>>>
>>>   
>>>       
>>>> Add a kernel-wide "phys_addr_t" which is guaranteed to be able to hold
>>>> any physical address.  By default it equals the word size of the
>>>> architecture, but a 32-bit architecture can set ARCH_PHYS_ADDR_T_64BIT
>>>> if it needs a 64-bit phys_addr_t.
>>>>
>>>>     
>>>>         
>>> You say this is a bugfix but you don't describe the bug.  This makes it
>>> rather hard to make the 2.6.2[5678] decisions.
>>>
>>> Ditto on [patch 2/3].
>>>   
>>>       
>> 1/3 is not a bugfix in itself, but a pre-requisite for 2/3.
>>
>> 2/3 replaces an ad-hoc Xen fix with a general fix to prevent address
>> truncation when using PFN_PHYS() on any PFN above the 4G mark.  The Xen
>> crash is the only bug I know of that's directly attributable to this,
>> and it was already addressed in older kernels with the casts in the Xen
>> code that this patch removes.
>>
>> So I don't think there's any strong need to push this to earlier kernels.
>>
>>     
>
> Still confused.  The above implies that 2.6.27 doesn't need fixing
> either, because the typecasts already avoid the crash.
>   

Yes, that's true.  It's a bit more of a comprehensive and correct fix; 
I think it's fairly low risk at this point in the -rc series, but it
could be deferred (or just defer 3/3, which really is cosmetic).

    J

Re: [PATCH 1 of 3] add phys_addr_t for holding physical addresses

[Benjamin Herrenschmidt]

 .../...

> diff --git a/include/asm-x86/page_32.h b/include/asm-x86/page_32.h
> --- a/include/asm-x86/page_32.h
> +++ b/include/asm-x86/page_32.h
> @@ -33,7 +33,6 @@
>  typedef u64	pudval_t;
>  typedef u64	pgdval_t;
>  typedef u64	pgprotval_t;
> -typedef u64	phys_addr_t;
 
 .../...

Might sound a stupid question, but why have a CONFIG_ option and
a global definition based on it rather than each arch defining it
as part of the base types ? I don't have a firm preference for one
or the other at this point, I can see pro and cons to both approach,
so I'm curious to see what others think about it.

Cheers,
Ben.

Re: [PATCH 1 of 3] add phys_addr_t for holding physical addresses

[Jeremy Fitzhardinge]

Benjamin Herrenschmidt wrote:
>  .../...
>
>   
>> diff --git a/include/asm-x86/page_32.h b/include/asm-x86/page_32.h
>> --- a/include/asm-x86/page_32.h
>> +++ b/include/asm-x86/page_32.h
>> @@ -33,7 +33,6 @@
>>  typedef u64	pudval_t;
>>  typedef u64	pgdval_t;
>>  typedef u64	pgprotval_t;
>> -typedef u64	phys_addr_t;
>>     
>  
>  .../...
>
> Might sound a stupid question, but why have a CONFIG_ option and
> a global definition based on it rather than each arch defining it
> as part of the base types ? I don't have a firm preference for one
> or the other at this point, I can see pro and cons to both approach,
> so I'm curious to see what others think about it.

My thinking is that:

There's only two possible types it can have: u32 and u64.  If we leave
it to per-arch definitions, they'll come up with a variety of different
ways of spelling those types (like u64 itself, but I gather that's being
fixed).

Furthermore, with only a couple of exceptions, the size is the same as
the bitness of the architecture, so there's no need to set a config in
most cases.

So, avoiding lots of duplication, basically.

    J

[PATCH 2 of 3] make PFN_PHYS explicitly return phys_addr_t

[Jeremy Fitzhardinge]

PFN_PHYS, as its name suggests, turns a pfn into a physical address.
However, it is a macro which just operates on its argument without
modifying its type.  pfns are typed unsigned long, but an unsigned
long may not be long enough to hold a physical address (32-bit systems
with more than 32 bits of physcial address).

Make sure we cast to phys_addr_t to return a complete result.

Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
Cc: Eduardo Habkost <ehabkost@redhat.com>
---
 arch/m32r/mm/discontig.c   |    4 ++--
 include/asm-x86/xen/page.h |    4 ++--
 include/linux/pfn.h        |    6 +++++-
 3 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/arch/m32r/mm/discontig.c b/arch/m32r/mm/discontig.c
--- a/arch/m32r/mm/discontig.c
+++ b/arch/m32r/mm/discontig.c
@@ -111,9 +111,9 @@
 				initrd_start, INITRD_SIZE);
 		} else {
 			printk("initrd extends beyond end of memory "
-				"(0x%08lx > 0x%08lx)\ndisabling initrd\n",
+				"(0x%08lx > 0x%08llx)\ndisabling initrd\n",
 				INITRD_START + INITRD_SIZE,
-				PFN_PHYS(max_low_pfn));
+			        (unsigned long long)PFN_PHYS(max_low_pfn));
 
 			initrd_start = 0;
 		}
diff --git a/include/asm-x86/xen/page.h b/include/asm-x86/xen/page.h
--- a/include/asm-x86/xen/page.h
+++ b/include/asm-x86/xen/page.h
@@ -76,13 +76,13 @@
 static inline xmaddr_t phys_to_machine(xpaddr_t phys)
 {
 	unsigned offset = phys.paddr & ~PAGE_MASK;
-	return XMADDR(PFN_PHYS((u64)pfn_to_mfn(PFN_DOWN(phys.paddr))) | offset);
+	return XMADDR(PFN_PHYS(pfn_to_mfn(PFN_DOWN(phys.paddr))) | offset);
 }
 
 static inline xpaddr_t machine_to_phys(xmaddr_t machine)
 {
 	unsigned offset = machine.maddr & ~PAGE_MASK;
-	return XPADDR(PFN_PHYS((u64)mfn_to_pfn(PFN_DOWN(machine.maddr))) | offset);
+	return XPADDR(PFN_PHYS(mfn_to_pfn(PFN_DOWN(machine.maddr))) | offset);
 }
 
 /*
diff --git a/include/linux/pfn.h b/include/linux/pfn.h
--- a/include/linux/pfn.h
+++ b/include/linux/pfn.h
@@ -1,9 +1,13 @@
 #ifndef _LINUX_PFN_H_
 #define _LINUX_PFN_H_
+
+#ifndef __ASSEMBLY__
+#include <linux/types.h>
+#endif
 
 #define PFN_ALIGN(x)	(((unsigned long)(x) + (PAGE_SIZE - 1)) & PAGE_MASK)
 #define PFN_UP(x)	(((x) + PAGE_SIZE-1) >> PAGE_SHIFT)
 #define PFN_DOWN(x)	((x) >> PAGE_SHIFT)
-#define PFN_PHYS(x)	((x) << PAGE_SHIFT)
+#define PFN_PHYS(x)	((phys_addr_t)(x) << PAGE_SHIFT)
 
 #endif

[PATCH 3 of 3] redefine resource_size_t as phys_addr_t

[Jeremy Fitzhardinge]

There's no good reason why a resource_size_t shouldn't just be a
physical address, so simply redefine it in terms of phys_addr_t.

Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Paul Mackerras <paulus@samba.org>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
---
 arch/powerpc/platforms/Kconfig.cputype |    1 -
 arch/powerpc/sysdev/ppc4xx_pci.c       |   16 ++++++----------
 arch/x86/Kconfig                       |    1 -
 arch/x86/kernel/e820.c                 |    4 +---
 drivers/pci/setup-bus.c                |    9 ++++-----
 include/linux/types.h                  |    8 ++------
 6 files changed, 13 insertions(+), 26 deletions(-)

diff --git a/arch/powerpc/platforms/Kconfig.cputype b/arch/powerpc/platforms/Kconfig.cputype
--- a/arch/powerpc/platforms/Kconfig.cputype
+++ b/arch/powerpc/platforms/Kconfig.cputype
@@ -135,7 +135,6 @@
 config PHYS_64BIT
 	bool 'Large physical address support' if E500
 	depends on 44x || E500
-	select RESOURCES_64BIT
 	default y if 44x
 	---help---
 	  This option enables kernel support for larger than 32-bit physical
diff --git a/arch/powerpc/sysdev/ppc4xx_pci.c b/arch/powerpc/sysdev/ppc4xx_pci.c
--- a/arch/powerpc/sysdev/ppc4xx_pci.c
+++ b/arch/powerpc/sysdev/ppc4xx_pci.c
@@ -41,13 +41,10 @@
 #define U64_TO_U32_LOW(val)	((u32)((val) & 0x00000000ffffffffULL))
 #define U64_TO_U32_HIGH(val)	((u32)((val) >> 32))
 
-#ifdef CONFIG_RESOURCES_64BIT
-#define RES_TO_U32_LOW(val)	U64_TO_U32_LOW(val)
-#define RES_TO_U32_HIGH(val)	U64_TO_U32_HIGH(val)
-#else
-#define RES_TO_U32_LOW(val)	(val)
-#define RES_TO_U32_HIGH(val)	(0)
-#endif
+#define RES_TO_U32_LOW(val)	\
+	((sizeof(resource_size_t) > sizeof(u32)) ? U64_TO_U32_LOW(val) : (val))
+#define RES_TO_U32_HIGH(val)	\
+	((sizeof(resource_size_t) > sizeof(u32)) ? U64_TO_U32_HIGH(val) : (0))
 
 static inline int ppc440spe_revA(void)
 {
@@ -145,12 +142,11 @@
 
 		/* Use that */
 		res->start = pci_addr;
-#ifndef CONFIG_RESOURCES_64BIT
 		/* Beware of 32 bits resources */
-		if ((pci_addr + size) > 0x100000000ull)
+		if (sizeof(resource_size_t) == sizeof(u32) &&
+		    (pci_addr + size) > 0x100000000ull)
 			res->end = 0xffffffff;
 		else
-#endif
 			res->end = res->start + size - 1;
 		break;
 	}
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -931,7 +931,6 @@
 	def_bool n
 	prompt "PAE (Physical Address Extension) Support"
 	depends on X86_32 && !HIGHMEM4G
-	select RESOURCES_64BIT
 	help
 	  PAE is required for NX support, and furthermore enables
 	  larger swapspace support for non-overcommit purposes. It
diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
--- a/arch/x86/kernel/e820.c
+++ b/arch/x86/kernel/e820.c
@@ -1276,12 +1276,10 @@
 	res = alloc_bootmem_low(sizeof(struct resource) * e820.nr_map);
 	for (i = 0; i < e820.nr_map; i++) {
 		end = e820.map[i].addr + e820.map[i].size - 1;
-#ifndef CONFIG_RESOURCES_64BIT
-		if (end > 0x100000000ULL) {
+		if (end != (resource_size_t)end) {
 			res++;
 			continue;
 		}
-#endif
 		res->name = e820_type_to_string(e820.map[i].type);
 		res->start = e820.map[i].addr;
 		res->end = end;
diff --git a/drivers/pci/setup-bus.c b/drivers/pci/setup-bus.c
--- a/drivers/pci/setup-bus.c
+++ b/drivers/pci/setup-bus.c
@@ -377,11 +377,10 @@
 	align = 0;
 	min_align = 0;
 	for (order = 0; order <= max_order; order++) {
-#ifdef CONFIG_RESOURCES_64BIT
-		resource_size_t align1 = 1ULL << (order + 20);
-#else
-		resource_size_t align1 = 1U << (order + 20);
-#endif
+		resource_size_t align1 = 1;
+
+		align1 <<= (order + 20);
+
 		if (!align)
 			min_align = align1;
 		else if (ALIGN(align + min_align, min_align) < align1)
diff --git a/include/linux/types.h b/include/linux/types.h
--- a/include/linux/types.h
+++ b/include/linux/types.h
@@ -191,17 +191,13 @@
 #ifdef __KERNEL__
 typedef unsigned __bitwise__ gfp_t;
 
-#ifdef CONFIG_RESOURCES_64BIT
-typedef u64 resource_size_t;
-#else
-typedef u32 resource_size_t;
-#endif
-
 #ifdef CONFIG_PHYS_ADDR_T_64BIT
 typedef u64 phys_addr_t;
 #else
 typedef u32 phys_addr_t;
 #endif
+
+typedef phys_addr_t resource_size_t;
 
 struct ustat {
 	__kernel_daddr_t	f_tfree;