Re: UVM patch for PR port-xen/45975

[Chuck Silvers <chuq%chuq.com@localhost>]

On Fri, Feb 17, 2012 at 04:42:04PM +0100, Manuel Bouyer wrote:
> Hello,
> there is an issue with SMP Xen guests (see port-xen/45975), where uvm
> can put a page back to free list before clearing its mappings.
> This happens when uvm_km_pgremove_intrsafe() is used: is has to be called
> before pmap_kremove() because it uses pmap_extract(), and (I guess for
> efficiency), all pages are freed before calling pmap_kremove() for the
> whole range.
> This is a problem for Xen, because one of these pages can be grabbed by
> another CPU for a PDP, and Xen won't allow to pin a page as PDP if it
> still has R/W mappings. As a CPU can be preempted by the hypervisor,
> there is potentially plenty of time for another CPU to grab the
> page, initialize it and try to use it as PDP between 
> uvm_km_pgremove_intrsafe()
> and pmap_kremove().
> 
> To fix this I propose the attached patch:
> - change uvm_km_pgremove_intrsafe()'s semantic to also unmap the page
>   (all uvm_km_pgremove_intrsafe() use are followed by a pmap_kremove(),
>   just move the call in uvm_km_pgremove_intrsafe()).
> - If __PMAP_NEEDS_UNMAP_BEFORE_FREE is defined, uvm_km_pgremove_intrsafe()
>   does a pmap_kremove() for each page, after pmap_extract() but before
>   uvm_pagefree(). If not defined, uvm_km_pgremove_intrsafe() does
>   a tail pmap_kremove() call at the end
> - __PMAP_NEEDS_UNMAP_BEFORE_FREE is defined for XEN in x86/include/pmap.h
> 
> Does anyone see a problem with this ?

hi manuel,

this is fine in general, but there are a few details:

 - it would be better to remove the mappings before freeing the page
   on all platforms, not just xen.  that's just a good practice.
   if you're concerned about a potential performance impact,
   you could process the pages in batches of 16 or so using a little
   array on the stack, which should handle most operations in one batch.
   I did a quick experiment just now and found that most calls
   to uvm_km_pgremove_intrsafe() are for 4 pages or less.
   (I wonder why those aren't using some higher-level cache?)
   about 10% of the calls were for 16 pages, and none were larger.

 - the calls to pmap_update() should be moved along with the calls to
   pmap_kremove().

 - a non-xen kernel with the patch dies during boot with:

panic: kernel diagnostic assertion "!pmap_extract(pmap_kernel(), loopva, NULL)" 
failed: file "/home/chs/netbsd/src/sys/uvm/uvm_km.c", line 749 
loopva=0xffff80001d1df000 loopsize=0x3000 vmem=0xffffffff80f01ae8
fatal breakpoint trap in supervisor mode
trap type 1 code 0 rip ffffffff802599e5 cs 8 rflags 246 cr2  0 cpl 8 rsp 
ffffffff810c1af0
Stopped in pid 0.1 (system) at  netbsd:breakpoint+0x5:  leave
db{0}> t
breakpoint() at netbsd:breakpoint+0x5
vpanic() at netbsd:vpanic+0x1f2
kern_assert() at netbsd:kern_assert+0x48
uvm_km_kmem_alloc() at netbsd:uvm_km_kmem_alloc+0x19f
kmem_intr_alloc() at netbsd:kmem_intr_alloc+0x5b
kmem_intr_zalloc() at netbsd:kmem_intr_zalloc+0xf
mpacpi_find_interrupts() at netbsd:mpacpi_find_interrupts+0x102
acpi_md_callback() at netbsd:acpi_md_callback+0x1a
acpi_attach() at netbsd:acpi_attach+0x163
config_attach_loc() at netbsd:config_attach_loc+0x182
mainbus_attach() at netbsd:mainbus_attach+0x200
config_attach_loc() at netbsd:config_attach_loc+0x182
cpu_configure() at netbsd:cpu_configure+0x26
main() at netbsd:main+0x2b0




-Chuck