Re: [Xen-devel] [PATCH]: Fix Xen domU boot with batched mprotect

[Jeremy Fitzhardinge <jeremy@goop.org>]

Jan Beulich wrote:
>>>> Jeremy Fitzhardinge <jeremy@goop.org> 15.10.08 18:23 >>>
>>>>         
>> virt_addr_valid() is supposed to be usable in this circumstace.  The 
>> comment says "virt_to_page(kaddr) returns a valid pointer if and only if 
>> virt_addr_valid(kaddr) returns true", which implies that 
>> virt_addr_valid() returns a meaningful result on all addresses - and if 
>> not, it should be fixed.
>>     
>
> Where did you find this comment? I had no luck grep-ing for it...
>   

It's in tip.git, which has quite a few changes in this area.

http://git.kernel.org/?p=linux/kernel/git/x86/linux-2.6-tip.git;a=blob;f=include/asm-x86/page.h;h=d4f1d5791fc186f29a9a60d4fe182d80f05038e4;hb=HEAD
http://git.kernel.org/?p=linux/kernel/git/x86/linux-2.6-tip.git;a=blob;f=arch/x86/mm/ioremap.c;h=ae71e11eb3e5e4ddeceadc9128d3afea564f27e0;hb=HEAD

> In any case, if that's the expectation, then on i386 virt_addr_valid()
> must be implemented as something like
>
> #define virt_addr_valid(kaddr) ((kaddr) >= PAGE_OFFSET && (kaddr) < high_memory && pfn_valid(__pa(kaddr) >> PAGE_SHIFT))
>
> x86-64 would need something similar, except that high_memory obviously
> must be replaced (or that part could perhaps be left out altogether), and
> the un-mapped addresses above the kernel mapping would need to be
> filtered out.
>
> Btw., if you look at other architectures, you'll see that most of them use
> the same (as you say broken) construct.
>
> Otoh, if that cited statement really holds, then virt_addr_valid() isn't
> really expected to do what its name implies: In particular, there are
> valid address ranges in kernel space which it wouldn't be permitted to
> return true on without significantly complicating the virt_to_page()
> implementation (e.g. x86-64's vmalloc and modules areas).
>   

The current x86-64 implementation is:

bool __virt_addr_valid(unsigned long x)
{
	if (x >= __START_KERNEL_map) {
		x -= __START_KERNEL_map;
		if (x >= KERNEL_IMAGE_SIZE)
			return false;
		x += phys_base;
	} else {
		if (x < PAGE_OFFSET)
			return false;
		x -= PAGE_OFFSET;
		if (system_state == SYSTEM_BOOTING ?
				x > MAXMEM : !phys_addr_valid(x)) {
			return false;
		}
	}

	return pfn_valid(x >> PAGE_SHIFT);
}

and 32-bit is similar (but simpler, since it doesn't need to worry about a separate kernel mapping).


>
> yields a positive indication from virt_addr_valid() on all tested addresses:
>
> <4>null: 00000000 00040000 1:1
> <4>half: 7fffffff 000bffff 1:1
> <4>hm-p: ed7ff000 0002d7ff 1:1
> <4>hm-1: ed7fffff 0002d7ff 1:1
> <4>hm: ed800000 0002d800 1:1
> <4>hm+1: ed800001 0002d800 1:1
> <4>hm+p: ed801000 0002d801 1:1
> <4>km: f56fa000 000356fa 1:1
> <4>hv: f5800000 00035800 1:1
>   

It would be interesting to try that with tip.git's version of 
__virt_addr_valid().  In the Xen case, all we need is a guarantee that 
virt_addr_valid() returns true iff __pa(addr) returns a proper result, 
so that we can use the resulting pfn as an index into pfn->mfn.  I 
believe this is what the current implementation does.

    J