From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Hao, Xudong" Subject: Re: Deadlocks by p2m_lock and event_lock Date: Wed, 14 Mar 2012 07:12:55 +0000 Message-ID: <403610A45A2B5242BD291EDAE8B37D300FCE891C@SHSMSX102.ccr.corp.intel.com> References: <403610A45A2B5242BD291EDAE8B37D300FCE3236@SHSMSX102.ccr.corp.intel.com> <20120309112010.GB83422@ocelot.phlegethon.org> <403610A45A2B5242BD291EDAE8B37D300FCE32B2@SHSMSX102.ccr.corp.intel.com> <763b511f59616a274ff142d62f55f7bf.squirrel@webmail.lagarcavilla.org> <20120309165544.GE83422@ocelot.phlegethon.org> <403610A45A2B5242BD291EDAE8B37D300FCE7398@SHSMSX102.ccr.corp.intel.com> <9b7df6f9a91ccea040afce6b1acb4a53.squirrel@webmail.lagarcavilla.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <9b7df6f9a91ccea040afce6b1acb4a53.squirrel@webmail.lagarcavilla.org> Content-Language: en-US List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: "andres@lagarcavilla.org" Cc: Keir Fraser , "xen-devel@lists.xensource.com" , Tim Deegan , "JBeulich@suse.com" , "Zhang, Xiantao" List-Id: xen-devel@lists.xenproject.org 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) [] _spin_lock+0x1b/0xa8 > > (XEN) [] notify_via_xen_event_channel+0x21/0x106 > > (XEN) [] hvm_buffered_io_send+0x1f1/0x21b > > (XEN) [] stdvga_intercept_mmio+0x491/0x4c7 > > (XEN) [] hvm_io_intercept+0x218/0x244 > > (XEN) [] hvmemul_do_io+0x55a/0x716 > > (XEN) [] hvmemul_do_mmio+0x2d/0x2f > > (XEN) [] hvmemul_write+0x181/0x1a2 > > (XEN) [] x86_emulate+0xcad3/0xfbdf > > (XEN) [] hvm_emulate_one+0x120/0x1af > > (XEN) [] handle_mmio+0x4e/0x1d1 > > (XEN) [] hvm_hap_nested_page_fault+0x210/0x37f > > (XEN) [] 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. > > >