Re: Deadlocks by p2m_lock and event_lock

["Hao, Xudong" <xudong.hao@intel.com>]

I prefer to the 2nd, I made a patch and testing show it works.

diff -r 5d20d2f6ffed xen/arch/x86/hvm/emulate.c
--- a/xen/arch/x86/hvm/emulate.c	Fri Mar 09 16:54:24 2012 +0000
+++ b/xen/arch/x86/hvm/emulate.c	Wed Mar 14 15:11:52 2012 -0400
@@ -60,20 +60,23 @@
     ioreq_t *p = get_ioreq(curr);
     unsigned long ram_gfn = paddr_to_pfn(ram_gpa);
     p2m_type_t p2mt;
-    mfn_t ram_mfn;
+    unsigned long ram_mfn;
     int rc;
 
     /* Check for paged out page */
-    ram_mfn = get_gfn_unshare(curr->domain, ram_gfn, &p2mt);
+    ram_mfn = mfn_x(get_gfn_unshare(curr->domain, ram_gfn, &p2mt));
+    if ( mfn_valid(ram_mfn) )
+        get_page(mfn_to_page(ram_mfn), curr->domain);
+    put_gfn(curr->domain, ram_gfn); 
     if ( p2m_is_paging(p2mt) )
     {
-        put_gfn(curr->domain, ram_gfn); 
+        put_page(mfn_to_page(ram_mfn));
         p2m_mem_paging_populate(curr->domain, ram_gfn);
         return X86EMUL_RETRY;
     }
     if ( p2m_is_shared(p2mt) )
     {
-        put_gfn(curr->domain, ram_gfn); 
+        put_page(mfn_to_page(ram_mfn));
         return X86EMUL_RETRY;
     }
 
@@ -87,7 +90,7 @@
         ASSERT(p_data != NULL); /* cannot happen with a REP prefix */
         if ( dir == IOREQ_READ )
             memset(p_data, ~0, size);
-        put_gfn(curr->domain, ram_gfn); 
+        put_page(mfn_to_page(ram_mfn));
         return X86EMUL_UNHANDLEABLE;
     }
 
@@ -108,7 +111,7 @@
             unsigned int bytes = vio->mmio_large_write_bytes;
             if ( (addr >= pa) && ((addr + size) <= (pa + bytes)) )
             {
-                put_gfn(curr->domain, ram_gfn); 
+                put_page(mfn_to_page(ram_mfn));
                 return X86EMUL_OKAY;
             }
         }
@@ -120,7 +123,7 @@
             {
                 memcpy(p_data, &vio->mmio_large_read[addr - pa],
                        size);
-                put_gfn(curr->domain, ram_gfn); 
+                put_page(mfn_to_page(ram_mfn));
                 return X86EMUL_OKAY;
             }
         }
@@ -134,7 +137,7 @@
         vio->io_state = HVMIO_none;
         if ( p_data == NULL )
         {
-            put_gfn(curr->domain, ram_gfn);
+            put_page(mfn_to_page(ram_mfn));
             return X86EMUL_UNHANDLEABLE;
         }
         goto finish_access;
@@ -144,11 +147,11 @@
              (addr == (vio->mmio_large_write_pa +
                        vio->mmio_large_write_bytes)) )
         {
-            put_gfn(curr->domain, ram_gfn);
+            put_page(mfn_to_page(ram_mfn));
             return X86EMUL_RETRY;
         }
     default:
-        put_gfn(curr->domain, ram_gfn);
+        put_page(mfn_to_page(ram_mfn));
         return X86EMUL_UNHANDLEABLE;
     }
 
@@ -156,7 +159,7 @@
     {
         gdprintk(XENLOG_WARNING, "WARNING: io already pending (%d)?\n",
                  p->state);
-        put_gfn(curr->domain, ram_gfn); 
+        put_page(mfn_to_page(ram_mfn));
         return X86EMUL_UNHANDLEABLE;
     }
 
@@ -208,7 +211,7 @@
 
     if ( rc != X86EMUL_OKAY )
     {
-        put_gfn(curr->domain, ram_gfn); 
+        put_page(mfn_to_page(ram_mfn));
         return rc;
     }
 
@@ -244,7 +247,7 @@
         }
     }
 
-    put_gfn(curr->domain, ram_gfn); 
+    put_page(mfn_to_page(ram_mfn));
     return X86EMUL_OKAY;
 }


Thanks,
-Xudong


> -----Original Message-----
> From: Andres Lagar-Cavilla [mailto:andres@lagarcavilla.org]
> Sent: Wednesday, March 14, 2012 2:46 AM
> To: Hao, Xudong
> Cc: Tim Deegan; Keir Fraser; xen-devel@lists.xensource.com; Zhang, Xiantao;
> JBeulich@suse.com
> Subject: RE: [Xen-devel] Deadlocks by p2m_lock and event_lock
> 
> > Hi, Tim and Andres
> > The patch fix part of this issue. In handle_mmio, function
> > hvmemul_do_io() is called and p2m lock was held again by calling
> > get_gfn_unshare(), still trigger a deadlocks.
> 
> Typically hvmemul_do_io gets the zero gfn, because in many cases that's the
> 'rma_gpa' it is passed. However, in the case of mmio, and particularly stdvga,
> ram_gpa is the data to be copied to the framebuffer. So it is in principle ok to
> get_gfn in hvmemul_do_io.
> 
> There are two solutions
> 1. msix_capability_init does not call p2m_change_entry_type_global. See my
> previous email. If we want to resync the
> EPT/NPT/traditional/VTD/IOMMU/superduper TLBs, we can just do that
> explicitly. I hope.
> 
> 2. hvmemul_do_io does gets a ref on the mfn underlying ram_gpa, and holds
> that for the critical section, instead of the p2m lock. One way to achieve this is
> 
>     /* Check for paged out page */
>     ram_mfn = get_gfn_unshare(curr->domain, ram_gfn, &p2mt);
>     if ( this or that )
>     { ... handle ... }
>     if ( mfn_valid(ram_mfn) )
>         get_page(mfn_to_page(ram_mfn, curr->domain));
>     put_gfn(curr->domain, ram_gfn)
> 
>     /* replace all put_gfn in all exit paths by put_page */
> 
> This will ensure the target page is live and sane while not holding the p2m lock.
> Xudong, did that make sense? Do you think you could try that and report back?
> 
> Thanks!
> Andres
> 
> >
> > (XEN) Xen call trace:
> > (XEN)    [<ffff82c4801261a3>] _spin_lock+0x1b/0xa8
> > (XEN)    [<ffff82c4801070d3>] notify_via_xen_event_channel+0x21/0x106
> > (XEN)    [<ffff82c4801b6883>] hvm_buffered_io_send+0x1f1/0x21b
> > (XEN)    [<ffff82c4801bbd3a>] stdvga_intercept_mmio+0x491/0x4c7
> > (XEN)    [<ffff82c4801b5d58>] hvm_io_intercept+0x218/0x244
> > (XEN)    [<ffff82c4801aa931>] hvmemul_do_io+0x55a/0x716
> > (XEN)    [<ffff82c4801aab1a>] hvmemul_do_mmio+0x2d/0x2f
> > (XEN)    [<ffff82c4801ab239>] hvmemul_write+0x181/0x1a2
> > (XEN)    [<ffff82c4801963f0>] x86_emulate+0xcad3/0xfbdf
> > (XEN)    [<ffff82c4801a9d2e>] hvm_emulate_one+0x120/0x1af
> > (XEN)    [<ffff82c4801b63cb>] handle_mmio+0x4e/0x1d1
> > (XEN)    [<ffff82c4801afd72>] hvm_hap_nested_page_fault+0x210/0x37f
> > (XEN)    [<ffff82c4801d2419>] vmx_vmexit_handler+0x1523/0x17d0
> >
> > Thanks,
> > -Xudong
> >
> >> -----Original Message-----
> >> From: Tim Deegan [mailto:tim@xen.org]
> >> Sent: Saturday, March 10, 2012 12:56 AM
> >> To: Andres Lagar-Cavilla
> >> Cc: Hao, Xudong; Keir Fraser; xen-devel@lists.xensource.com; Zhang,
> >> Xiantao; JBeulich@suse.com
> >> Subject: Re: [Xen-devel] Deadlocks by p2m_lock and event_lock
> >>
> >> At 08:29 -0800 on 09 Mar (1331281767), Andres Lagar-Cavilla wrote:
> >> > >> I don't know about the event lock, but it seems unwise to call
> >> > >> in to handle_mmio with a gfn lock held.  How about fixing the
> >> > >> other
> >> path?
> >> > >>
> >> > >> diff -r 04673ecb9d78 xen/arch/x86/hvm/hvm.c
> >> > >> --- a/xen/arch/x86/hvm/hvm.c	Thu Mar 08 16:40:05 2012 +0000
> >> > >> +++ b/xen/arch/x86/hvm/hvm.c	Fri Mar 09 11:15:25 2012 +0000
> >> > >> @@ -1324,10 +1324,11 @@ int
> hvm_hap_nested_page_fault(unsigned l
> >> > >>      if ( (p2mt == p2m_mmio_dm) ||
> >> > >>           (access_w && (p2mt == p2m_ram_ro)) )
> >> > >>      {
> >> > >> +        put_gfn(p2m->domain, gfn);
> >> > >>          if ( !handle_mmio() )
> >> > >>              hvm_inject_exception(TRAP_gp_fault, 0, 0);
> >> > >>          rc = 1;
> >> > >> -        goto out_put_gfn;
> >> > >> +        goto out;
> >> > >>      }
> >> > >>
> >> > >>  #ifdef __x86_64__
> >> > >> @@ -1379,6 +1380,7 @@ int hvm_hap_nested_page_fault(unsigned l
> >> > >>
> >> > >>  out_put_gfn:
> >> > >>      put_gfn(p2m->domain, gfn);
> >> > >> +out:
> >> > >>      if ( paged )
> >> > >>          p2m_mem_paging_populate(v->domain, gfn);
> >> > >>      if ( req_ptr )
> >> > >
> >> > > Yes, that's fine to release the p2m lock earlier than handle_mmio.
> >> >
> >> > Ack
> >>
> >> OK, applied.
> >>
> >> Tim.
> >
>