Make PFN_PHYS return a properly-formed physical address

Quoted 10 lines · On Thu, 07 Aug 2008 14:38:08 -0700 Jeremy Fitzhardinge <[email protected]> wrote:

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). This means that the resulting address could be truncated if it doesn't fit within an unsigned long. This isn't generally a problem because most users end up using it for "low" memory, but there's no reason why PFN_PHYS couldn't be used for any possible pfn.

Please copy a mailing list on patches. So you can get your titties toasted off ;)

Quoted 4 lines

Fortunately, resource_size_t is the right size, and has approximately the right meaning. It's 64-bits on platforms where that's appropriate, but 32-bits where the extra bits are not needed.

aww maaan. Hack or what?

Review

Quoted 6 lines

#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)

(quoted diff) +1 -1

-#define PFN_PHYS(x)	((x) << PAGE_SHIFT)
+#define PFN_PHYS(x)	((resource_size_t)(x) << PAGE_SHIFT)

Busted on PAE with CONFIG_RESOURCES_64BIT=n, surely?

Can we please do this properly, whatever that is? Even a dumb always-return-u64 would be better?

Quoted 6 lines

printk("initrd extends beyond end of memory "

(quoted diff) +1 -1

-				"(0x%08lx > 0x%08lx)\ndisabling initrd\n",
+				"(0x%08lx > 0x%08llx)\ndisabling initrd\n",

				INITRD_START + INITRD_SIZE,
				PFN_PHYS(max_low_pfn));

that'll generate a compile warning if m32r can set CONFIG_RESOURCES_64BIT=n.

Quoted 15 lines · Andrew Morton

Quoted 10 lines · On Thu, 07 Aug 2008 14:38:08 -0700 Jeremy Fitzhardinge <[email protected]> wrote:

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). This means that the resulting address could be truncated if it doesn't fit within an unsigned long. This isn't generally a problem because most users end up using it for "low" memory, but there's no reason why PFN_PHYS couldn't be used for any possible pfn.

Quoted 3 lines

Please copy a mailing list on patches. So you can get your titties toasted off ;)

Oops. Forgot.

Quoted 5 lines

Quoted 4 lines

Fortunately, resource_size_t is the right size, and has approximately the right meaning. It's 64-bits on platforms where that's appropriate, but 32-bits where the extra bits are not needed.

Quoted 2 lines

aww maaan. Hack or what?

I don't know. Is it? It's what linux/ioport.h:struct resource uses to hold "start" and "end", which presumably means its intended to hold arbitrary physical addresses.

Quoted 9 lines

Review

Quoted 6 lines

#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)

(quoted diff) +1 -1

-#define PFN_PHYS(x)	((x) << PAGE_SHIFT)
+#define PFN_PHYS(x)	((resource_size_t)(x) << PAGE_SHIFT)

Busted on PAE with CONFIG_RESOURCES_64BIT=n, surely?

Not an option:

config X86_PAE

	def_bool n
	prompt "PAE (Physical Address Extension) Support"
	depends on X86_32 && !HIGHMEM4G
	select RESOURCES_64BIT

And if you don't enable RESOURCES_64BIT, then I guess it's reasonable for PFN_PHYS to discount the possibility of high pages?

Quoted 3 lines

Can we please do this properly, whatever that is? Even a dumb always-return-u64 would be better?

I had that originally, but someone (hpa?) suggested resource_size_t. The sad thing is that most users don't really care; they're either 64-bit anyway, or immediately truncate the result to 32-bit.

"Properly" would be to define a phys_addr_t which can always represent a physical address. We have one in x86-land, but I hesitate to add it for everyone else.

Quoted 9 lines

Quoted 6 lines

printk("initrd extends beyond end of memory "

(quoted diff) +1 -1

-				"(0x%08lx > 0x%08lx)\ndisabling initrd\n",
+				"(0x%08lx > 0x%08llx)\ndisabling initrd\n",

				INITRD_START + INITRD_SIZE,
				PFN_PHYS(max_low_pfn));

that'll generate a compile warning if m32r can set CONFIG_RESOURCES_64BIT=n.

(u64) cast, I guess.

Yes, but resource_size_t is for IO addressing, not for memory addressing.

Lots of X86_32 machines can happily support 32-bit physical addresses for IO while needing >32 bit addresses for physical memory.

Quoted 10 lines

Quoted 10 lines

Review

Quoted 7 lines

#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)

(quoted diff) +1 -1

-#define PFN_PHYS(x)	((x) << PAGE_SHIFT)
+#define PFN_PHYS(x)	((resource_size_t)(x) << PAGE_SHIFT)

Busted on PAE with CONFIG_RESOURCES_64BIT=n, surely?

Quoted 9 lines

Not an option:

config X86_PAE

	def_bool n
	prompt "PAE (Physical Address Extension) Support"
	depends on X86_32 && !HIGHMEM4G
	select RESOURCES_64BIT

err, OK, that was a bit arbitrary of us.

Oh well, scrub the above assertion.

Then again, do all architectures disallow 32-bit resource_size_t on 64-bit? And there's ppc32's CONFIG_HIGHMEM option to think about.

Quoted 2 lines

"Properly" would be to define a phys_addr_t which can always represent a physical address. We have one in x86-land, but I hesitate to add it for everyone else.

hm. It is a distinct and singular concept - it makes sense to have a specific type to represet "a physical address for memory".

nope ;) We don't know what type u64 has - some architectures use `unsigned long' (we might fix this soon).

For now, a full cast to `unsigned long long' is needed.

Shall we go with just using plain u64 (or unsigned long long if we want a really consistent type) in the meantime, and then waffle about introducing a new type everywhere?

Or we could redefine resource_size_t to be big enough to refer to any resource, including all memory. It's close to being that anyway.

Quoted 2 lines · On Thu, 07 Aug 2008 16:45:12 -0700 Jeremy Fitzhardinge <[email protected]> wrote:

Shall we go with just using plain u64 (or unsigned long long if we want a really consistent type) in the meantime, and then waffle about introducing a new type everywhere?

Quoted 2 lines

Or we could redefine resource_size_t to be big enough to refer to any resource, including all memory. It's close to being that anyway.

We could do

	typedef resource_size_t jeremy_thing_t;

for now and worry about it later if the need arises, perhaps.

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 <[email protected]>

Cc Paul Mackerras <[email protected]>

Cc Benjamin Herrenschmidt <[email protected]>

diff -r 4909b40dbdc5 arch/powerpc/Kconfig

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

diff -r 4909b40dbdc5 arch/x86/Kconfig

arch/x86/Kconfig +3 -0

@@ -1002,6 +1002,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 -r 4909b40dbdc5 include/asm-powerpc/types.h

include/asm-powerpc/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

diff -r 4909b40dbdc5 include/asm-x86/page_32.h

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;

diff -r 4909b40dbdc5 include/asm-x86/page_64.h

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;
 

diff -r 4909b40dbdc5 include/linux/types.h

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;

diff -r 4909b40dbdc5 mm/Kconfig

mm/Kconfig +3 -0

@@ -190,6 +190,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

Quoted 5 lines · 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.

I've been wondering for some time why can't we make phys_addr_t and resource_size_t the same thing... I don't like having two ARCH_* thing especially since I believe the one for resources is already what we want.

Quoted 8 lines · Benjamin Herrenschmidt

Quoted 5 lines · 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.

Quoted 5 lines

I've been wondering for some time why can't we make phys_addr_t and resource_size_t the same thing... I don't like having two ARCH_* thing especially since I believe the one for resources is already what we want.

I made the same argument, but Andrew thinks they're conceptually distinct. It is theoretically possible you might have a system with >4G memory, but all io resources < 4G, so you'd have resource_size_t 32-bits, while having 64-bit physical addresses. You can configure such a thing, but I don't know if it's 1) useful or 2) used.

Are we sure resource_size_t is -never- used to represent memory ? I though it was on some platforms....

Quoted 2 lines · Benjamin Herrenschmidt

Are we sure resource_size_t is -never- used to represent memory ? I though it was on some platforms....

On x86 it's optionally used to put memory in the resource tree, but if the memory is larger than can be held in resource_size_t it simply skips it. Don't know about elsewhere.

Quoted 7 lines · Jeremy Fitzhardinge

Quoted 3 lines · Benjamin Herrenschmidt

Are we sure resource_size_t is -never- used to represent memory ? I though it was on some platforms....

On x86 it's optionally used to put memory in the resource tree, but if the memory is larger than can be held in resource_size_t it simply skips it. Don't know about elsewhere.

That sounds like a good enough reason to not separate the two concepts..

The resource_size_t situation is obscure. I'd be happy to just remove its config, use my patch and typedef phys_addr_t resource_size_t.