Messages in this thread | | | Date | Thu, 07 Aug 2008 15:10:11 -0700 | From | Jeremy Fitzhardinge <> | Subject | Re: [PATCH] Make PFN_PHYS return a properly-formed physical address |
| |
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
| |