|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] Deadlocks by p2m_lock and event_lock
The get_page() and put_page() should be used in pairs. You cannot call put_page() separately when get_page() is not called before. best regards yang > -----Original Message----- > From: xen-devel-bounces@xxxxxxxxxxxxx > [mailto:xen-devel-bounces@xxxxxxxxxxxxx] On Behalf Of Hao, Xudong > Sent: Wednesday, March 14, 2012 3:13 PM > To: andres@xxxxxxxxxxxxxxxx > Cc: Keir Fraser; xen-devel@xxxxxxxxxxxxxxxxxxx; Tim Deegan; JBeulich@xxxxxxxx; > Zhang, Xiantao > Subject: Re: [Xen-devel] Deadlocks by p2m_lock and event_lock > > 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@xxxxxxxxxxxxxxxx] > > Sent: Wednesday, March 14, 2012 2:46 AM > > To: Hao, Xudong > > Cc: Tim Deegan; Keir Fraser; xen-devel@xxxxxxxxxxxxxxxxxxx; Zhang, Xiantao; > > JBeulich@xxxxxxxx > > 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@xxxxxxx] > > >> Sent: Saturday, March 10, 2012 12:56 AM > > >> To: Andres Lagar-Cavilla > > >> Cc: Hao, Xudong; Keir Fraser; xen-devel@xxxxxxxxxxxxxxxxxxx; Zhang, > > >> Xiantao; JBeulich@xxxxxxxx > > >> 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. > > > > > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@xxxxxxxxxxxxx > http://lists.xen.org/xen-devel _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |