Skip to contentSkip to conversation

define and use phys_addr_t

PATCH New Jeremy Fitzhardinge 3 participants 10 messages Aug 19, 2008 – Aug 24, 2008

Subsystems (5)
Memory management x86 PowerPC PCI +1 more subsystems
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 define and use phys_addr_t

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

#1 lore raw

PATCH 1/1 make PFN_PHYS explicitly return phys_addr_t

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.

---
 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(-)
arch/m32r/mm/discontig.c +2 -2
@@ -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;
 		}
include/asm-x86/xen/page.h +2 -2
@@ -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);
 }
 
 /*
include/linux/pfn.h +5 -1
@@ -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
#2 lore raw

PATCH 1/1 add phys_addr_t for holding physical addresses

In reply to 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.

Cc Paul Mackerras <[email protected]>
Cc Benjamin Herrenschmidt <[email protected]>
---
 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(-)
arch/powerpc/Kconfig +3 -0
@@ -21,6 +21,9 @@
 
 config PPC_MERGE
 	def_bool y
+
+config ARCH_PHYS_ADDR_T_64BIT
+       def_bool PPC64 || PHYS_64BIT
 
 config MMU
 	bool
arch/powerpc/include/asm/types.h +0 -7
@@ -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
arch/x86/Kconfig +3 -0
@@ -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)"
include/asm-x86/page_32.h +0 -2
@@ -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;
include/asm-x86/page_64.h +0 -1
@@ -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;
 
include/linux/types.h +6 -0
@@ -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;
mm/Kconfig +3 -0
@@ -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
maintainer
#3 lore raw

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

Quoted 5 lines · On Tue, 19 Aug 2008 13:02:50 -0700 Jeremy Fitzhardinge <[email protected]> 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].

#4 lore raw

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

Quoted 11 lines · Andrew Morton
Quoted 6 lines · On Tue, 19 Aug 2008 13:02:50 -0700 Jeremy Fitzhardinge <[email protected]> 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.

Quoted 5 lines

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

maintainer
#5 lore raw

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

Quoted 20 lines · On Fri, 22 Aug 2008 14:11:16 -0700 Jeremy Fitzhardinge <[email protected]> wrote:
Quoted 11 lines · Andrew Morton
Quoted 6 lines · On Tue, 19 Aug 2008 13:02:50 -0700 Jeremy Fitzhardinge <[email protected]> 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.

Quoted 5 lines

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.

Quoted 8 lines

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.

#6 lore raw

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

Quoted 32 lines · Andrew Morton
Quoted 28 lines · On Fri, 22 Aug 2008 14:11:16 -0700 Jeremy Fitzhardinge <[email protected]> wrote:
Quoted 12 lines · Andrew Morton
Quoted 7 lines · On Tue, 19 Aug 2008 13:02:50 -0700 Jeremy Fitzhardinge <[email protected]> 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.

Quoted 6 lines

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.

Quoted 7 lines
Quoted 3 lines

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

#7 lore raw

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

In reply to Jeremy Fitzhardinge

.../...

include/asm-x86/page_32.h line 36

Quoted 8 lines
include/asm-x86/page_32.h +0 -1
@@ -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.

#8 lore raw

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

Quoted 15 lines · Benjamin Herrenschmidt

.../...

include/asm-x86/page_32.h line 36

Quoted 9 lines
include/asm-x86/page_32.h +0 -1
@@ -33,7 +33,6 @@
 typedef u64	pudval_t;
 typedef u64	pgdval_t;
 typedef u64	pgprotval_t;
-typedef u64	phys_addr_t;
 

.../...

Quoted 5 lines

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

#9 lore raw

PATCH 1/1 redefine resource_size_t as phys_addr_t

In reply to 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.

Cc Ingo Molnar <[email protected]>
Cc Paul Mackerras <[email protected]>
Cc Benjamin Herrenschmidt <[email protected]>
---
 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(-)
arch/powerpc/platforms/Kconfig.cputype +0 -1
@@ -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
arch/powerpc/sysdev/ppc4xx_pci.c +6 -10
@@ -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;
 	}
arch/x86/Kconfig +0 -1
@@ -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
arch/x86/kernel/e820.c +1 -3
@@ -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;
drivers/pci/setup-bus.c +4 -5
@@ -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)
include/linux/types.h +2 -6
@@ -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;