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

[Andrew Morton <akpm@linux-foundation.org>]

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.