Thanks for dredging this back up!
On 08/14/2014 07:18 AM, Frantisek Hrbata wrote:
> +int valid_phys_addr_range(phys_addr_t addr, size_t count)
> +{
> + return addr + count <= __pa(high_memory);
> +}
Is this correct on 32-bit? It would limit /dev/mem to memory below 896MB.
> +int valid_mmap_phys_addr_range(unsigned long pfn, size_t count)
> +{
Nit: please add units to things like "count". len_bytes would be nice
for this kind of thing, especially since it's passed *with* a pfn it
would be easy to think it is a count in pages.
> + /* pgoff + count overflow is checked in do_mmap_pgoff */
> + pfn += count >> PAGE_SHIFT;
> +
> + if (pfn >> BITS_PER_LONG - PAGE_SHIFT)
> + return -EOVERFLOW;
Is this -EOVERFLOW correct? It is called like this:
> static int mmap_mem(struct file *file, struct vm_area_struct *vma)
> {
> if (!valid_mmap_phys_addr_range(vma->vm_pgoff, size))
> return -EINVAL;
So I think we need to return true/false:0/1. -EOVERFLOW would be true,
and that if() would pass.
> + return phys_addr_valid(pfn << PAGE_SHIFT);
> +}
Maybe I'm dumb, but it took me a minute to figure out what you were
trying to do with the: "(pfn >> BITS_PER_LONG - PAGE_SHIFT)". In any
case, I think it is wrong on 32-bit.
On 32-bit, BITS_PER_LONG=32, and PAGE_SIZE=12, and a paddr=0x100000000
or pfn=0x100000 (4GB) is perfectly valid with PAE enabled. But, this
code pfn>>(32-12) would result in 0x1 and return -EOVERFLOW.
I think something like this would be easier to read and actually work on
32-bit:
static inline int arch_pfn_possible(unsigned long pfn)
{
unsigned long max_arch_pfn = 1UL << (boot_cpu_data.x86_phys_bits -
PAGE_SHIFT);
return pfn < max_arch_pfn;
}
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@xxxxxxxxx. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>