Re: [PATCH] Make PFN_PHYS return a properly-formed physical address

Re: [PATCH] Make PFN_PHYS return a properly-formed physical address

[Jeremy Fitzhardinge]

Andrew Morton wrote:
> On Thu, 07 Aug 2008 14:38:08 -0700
> Jeremy Fitzhardinge <jeremy@goop.org> 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 ;)
>   

Oops.  Forgot.

>> 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?
>   

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.

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

> 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.

>> 			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));
>>     
>
> that'll generate a compile warning if m32r can set CONFIG_RESOURCES_64BIT=n.
>   

(u64) cast, I guess.

    J

Re: [PATCH] Make PFN_PHYS return a properly-formed physical address

[Andrew Morton]

On Thu, 07 Aug 2008 15:10:11 -0700
Jeremy Fitzhardinge <jeremy@goop.org> wrote:

> Andrew Morton wrote:
> > On Thu, 07 Aug 2008 14:38:08 -0700
> > Jeremy Fitzhardinge <jeremy@goop.org> 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 ;)
> >   
> 
> Oops.  Forgot.
> 
> >> 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?
> >   
> 
> 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.

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.


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

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.

> And if you don't enable RESOURCES_64BIT, then I guess it's reasonable 
> for PFN_PHYS to discount the possibility of high pages?
> 
> > 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.

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


> >> 			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));
> >>     
> >
> > that'll generate a compile warning if m32r can set CONFIG_RESOURCES_64BIT=n.
> >   
> 
> (u64) cast, I guess.
> 

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.

Re: [PATCH] Make PFN_PHYS return a properly-formed physical address

[Jeremy Fitzhardinge]

Andrew Morton wrote:
> 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.
>   

Really?  The resource tree treats normal memory as just another 
resource.  Is it expected that you could have usable memory not 
represented by /proc/iomem?

Hm, looks like memory hotplug assumes that resource_size_t is always 
64-bits, but the e820->resource conversion simply skips over-large 
addresses.

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

x86 and ppc were the only archs to touch it; they otherwise use the 
default of "default 64BIT".

I didn't look at the ppc use case.   I wasn't terribly concerned about 
current users of PFN_PHYS, because it presumably works OK for them.

>> "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".
>   

Yes.  We had to be particularly careful with it on x86 because of all 
the problems it's caused, but it is a generally useful thing to be able 
to talk about.

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.

> 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.
>   

Yep.

    J

Re: [PATCH] Make PFN_PHYS return a properly-formed physical address

[Andrew Morton]

On Thu, 07 Aug 2008 16:45:12 -0700
Jeremy Fitzhardinge <jeremy@goop.org> 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?
> 
> 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.

Re: [PATCH] Make PFN_PHYS return a properly-formed physical address

[Jeremy Fitzhardinge]

Andrew Morton wrote:
> On Thu, 07 Aug 2008 16:45:12 -0700
> Jeremy Fitzhardinge <jeremy@goop.org> 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?
>>
>> 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.
>   

OK, I'll revise and repost.

Hm, curiously, the two arches which care most about this issue (x86 and 
powerpc) already have phys_addr_t...

I guess we don't want CONFIG_ARCH_HAVE_PHYS_ADDR_T...

    J

[PATCH 1/2] 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>
diff -r 4909b40dbdc5 arch/powerpc/Kconfig
--- a/arch/powerpc/Kconfig	Fri Aug 08 13:40:52 2008 -0700
+++ b/arch/powerpc/Kconfig	Mon Aug 11 10:32:52 2008 -0700
@@ -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
--- a/arch/x86/Kconfig	Fri Aug 08 13:40:52 2008 -0700
+++ b/arch/x86/Kconfig	Mon Aug 11 10:32:53 2008 -0700
@@ -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
--- a/include/asm-powerpc/types.h	Fri Aug 08 13:40:52 2008 -0700
+++ b/include/asm-powerpc/types.h	Mon Aug 11 10:32:53 2008 -0700
@@ -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
--- a/include/asm-x86/page_32.h	Fri Aug 08 13:40:52 2008 -0700
+++ b/include/asm-x86/page_32.h	Mon Aug 11 10:32:53 2008 -0700
@@ -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
--- a/include/asm-x86/page_64.h	Fri Aug 08 13:40:52 2008 -0700
+++ b/include/asm-x86/page_64.h	Mon Aug 11 10:32:53 2008 -0700
@@ -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
--- a/include/linux/types.h	Fri Aug 08 13:40:52 2008 -0700
+++ b/include/linux/types.h	Mon Aug 11 10:32:53 2008 -0700
@@ -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
--- a/mm/Kconfig	Fri Aug 08 13:40:52 2008 -0700
+++ b/mm/Kconfig	Mon Aug 11 10:32:53 2008 -0700
@@ -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

Re: [PATCH 1/2] add phys_addr_t for holding physical addresses

[Benjamin Herrenschmidt]

On Mon, 2008-08-11 at 12:38 -0700, Jeremy Fitzhardinge 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.

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.

Ben.

> Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
> Cc: Paul Mackerras <paulus@samba.org>
> Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> diff -r 4909b40dbdc5 arch/powerpc/Kconfig
> --- a/arch/powerpc/Kconfig	Fri Aug 08 13:40:52 2008 -0700
> +++ b/arch/powerpc/Kconfig	Mon Aug 11 10:32:52 2008 -0700
> @@ -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
> --- a/arch/x86/Kconfig	Fri Aug 08 13:40:52 2008 -0700
> +++ b/arch/x86/Kconfig	Mon Aug 11 10:32:53 2008 -0700
> @@ -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
> --- a/include/asm-powerpc/types.h	Fri Aug 08 13:40:52 2008 -0700
> +++ b/include/asm-powerpc/types.h	Mon Aug 11 10:32:53 2008 -0700
> @@ -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
> --- a/include/asm-x86/page_32.h	Fri Aug 08 13:40:52 2008 -0700
> +++ b/include/asm-x86/page_32.h	Mon Aug 11 10:32:53 2008 -0700
> @@ -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
> --- a/include/asm-x86/page_64.h	Fri Aug 08 13:40:52 2008 -0700
> +++ b/include/asm-x86/page_64.h	Mon Aug 11 10:32:53 2008 -0700
> @@ -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
> --- a/include/linux/types.h	Fri Aug 08 13:40:52 2008 -0700
> +++ b/include/linux/types.h	Mon Aug 11 10:32:53 2008 -0700
> @@ -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
> --- a/mm/Kconfig	Fri Aug 08 13:40:52 2008 -0700
> +++ b/mm/Kconfig	Mon Aug 11 10:32:53 2008 -0700
> @@ -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
> 

Re: [PATCH 1/2] add phys_addr_t for holding physical addresses

[Jeremy Fitzhardinge]

Benjamin Herrenschmidt wrote:
> On Mon, 2008-08-11 at 12:38 -0700, Jeremy Fitzhardinge 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.
>>     
>
> 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.

    J

Re: [PATCH 1/2] add phys_addr_t for holding physical addresses

[Benjamin Herrenschmidt]

On Mon, 2008-08-11 at 15:15 -0700, Jeremy Fitzhardinge wrote:
> Benjamin Herrenschmidt wrote:
> > On Mon, 2008-08-11 at 12:38 -0700, Jeremy Fitzhardinge 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.
> >>     
> >
> > 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....

Ben.

Re: [PATCH 1/2] add phys_addr_t for holding physical addresses

[Jeremy Fitzhardinge]

Benjamin Herrenschmidt wrote:
> 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.

    J

Re: [PATCH 1/2] add phys_addr_t for holding physical addresses

[Benjamin Herrenschmidt]

On Mon, 2008-08-11 at 15:50 -0700, Jeremy Fitzhardinge wrote:
> Benjamin Herrenschmidt wrote:
> > 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..

Ben.

Re: [PATCH 1/2] add phys_addr_t for holding physical addresses

[Jeremy Fitzhardinge]

Benjamin Herrenschmidt wrote:
> On Mon, 2008-08-11 at 15:50 -0700, Jeremy Fitzhardinge wrote:
>   
>> Benjamin Herrenschmidt wrote:
>>     
>>> 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.

    J

Re: [PATCH 1/2] add phys_addr_t for holding physical addresses

[Benjamin Herrenschmidt]

> 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.

I have no objection there as far as powerpc is concerned.

Ben.

[PATCH 2/2] 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   |    2 +-
 include/asm-x86/xen/page.h |    4 ++--
 include/linux/pfn.h        |    6 +++++-
 3 files changed, 8 insertions(+), 4 deletions(-)

diff -r 6b03eb6361dd arch/m32r/mm/discontig.c
--- a/arch/m32r/mm/discontig.c	Mon Aug 11 11:19:24 2008 -0700
+++ b/arch/m32r/mm/discontig.c	Mon Aug 11 11:20:07 2008 -0700
@@ -112,9 +112,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 -r 6b03eb6361dd include/asm-x86/xen/page.h
--- a/include/asm-x86/xen/page.h	Mon Aug 11 11:19:24 2008 -0700
+++ b/include/asm-x86/xen/page.h	Mon Aug 11 11:20:07 2008 -0700
@@ -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 -r 6b03eb6361dd include/linux/pfn.h
--- a/include/linux/pfn.h	Mon Aug 11 11:19:24 2008 -0700
+++ b/include/linux/pfn.h	Mon Aug 11 11:20:07 2008 -0700
@@ -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