|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] Deadlocks by p2m_lock and event_lock
> Works by tested. > > Thanks, Thanks to you! Andres > -Xudong > > >> -----Original Message----- >> From: Andres Lagar-Cavilla [mailto:andres@xxxxxxxxxxxxxxxx] >> Sent: Wednesday, March 14, 2012 11:11 PM >> 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 >> >> Can you give this a try? (Tim, Keir, Jan, if Xudong reports success, can >> you >> please apply?) >> >> Thanks, >> Andres >> >> # HG changeset patch >> # User Andres Lagar-Cavilla <andres@xxxxxxxxxxxxxxxx> # Date 1331737660 >> 14400 # Node ID fe10f0433f6279091c193127d95d4d39b44a72ed >> # Parent 5d20d2f6ffed0a49f030f04a8870f1926babbcbf >> x86/mm: Fix deadlock between p2m and event channel locks. >> >> The hvm io emulation code holds the p2m lock for the duration of the >> emulation, >> which may include sending an event to qemu. On a separate path, >> map_domain_pirq grabs the event channel and p2m locks in opposite order. >> >> Fix this by ensuring liveness of the ram_gfn used by io emulation, with >> a page >> ref. >> >> Reported-by: "Hao, Xudong" <xudong.hao@xxxxxxxxx> >> Signed-off-by: "Hao, Xudong" <xudong.hao@xxxxxxxxx> >> Signed-off-by: Andres Lagar-Cavilla <andres@xxxxxxxxxxxxxxxx> >> >> diff -r 5d20d2f6ffed -r fe10f0433f62 xen/arch/x86/hvm/emulate.c >> --- a/xen/arch/x86/hvm/emulate.c >> +++ b/xen/arch/x86/hvm/emulate.c >> @@ -77,6 +77,17 @@ static int hvmemul_do_io( >> return X86EMUL_RETRY; >> } >> >> + /* Maintain a ref on the mfn to ensure liveness. Put the gfn >> + * to avoid potential deadlock wrt event channel lock, later. */ >> + if ( mfn_valid(mfn_x(ram_mfn)) ) >> + if ( !get_page(mfn_to_page(mfn_x(ram_mfn)), >> + curr->domain) ) >> + { >> + put_gfn(curr->domain, ram_gfn); >> + return X86EMUL_RETRY; >> + } >> + put_gfn(curr->domain, ram_gfn); >> + >> /* >> * Weird-sized accesses have undefined behaviour: we discard writes >> * and read all-ones. >> @@ -87,7 +98,8 @@ static int hvmemul_do_io( >> 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); >> + if ( mfn_valid(mfn_x(ram_mfn)) ) >> + put_page(mfn_to_page(mfn_x(ram_mfn))); >> return X86EMUL_UNHANDLEABLE; >> } >> >> @@ -108,7 +120,8 @@ static int hvmemul_do_io( >> unsigned int bytes = vio->mmio_large_write_bytes; >> if ( (addr >= pa) && ((addr + size) <= (pa + bytes)) ) >> { >> - put_gfn(curr->domain, ram_gfn); >> + if ( mfn_valid(mfn_x(ram_mfn)) ) >> + put_page(mfn_to_page(mfn_x(ram_mfn))); >> return X86EMUL_OKAY; >> } >> } >> @@ -120,7 +133,8 @@ static int hvmemul_do_io( >> { >> memcpy(p_data, &vio->mmio_large_read[addr - pa], >> size); >> - put_gfn(curr->domain, ram_gfn); >> + if ( mfn_valid(mfn_x(ram_mfn)) ) >> + put_page(mfn_to_page(mfn_x(ram_mfn))); >> return X86EMUL_OKAY; >> } >> } >> @@ -134,7 +148,8 @@ static int hvmemul_do_io( >> vio->io_state = HVMIO_none; >> if ( p_data == NULL ) >> { >> - put_gfn(curr->domain, ram_gfn); >> + if ( mfn_valid(mfn_x(ram_mfn)) ) >> + put_page(mfn_to_page(mfn_x(ram_mfn))); >> return X86EMUL_UNHANDLEABLE; >> } >> goto finish_access; >> @@ -144,11 +159,13 @@ static int hvmemul_do_io( >> (addr == (vio->mmio_large_write_pa + >> vio->mmio_large_write_bytes)) ) >> { >> - put_gfn(curr->domain, ram_gfn); >> + if ( mfn_valid(mfn_x(ram_mfn)) ) >> + put_page(mfn_to_page(mfn_x(ram_mfn))); >> return X86EMUL_RETRY; >> } >> default: >> - put_gfn(curr->domain, ram_gfn); >> + if ( mfn_valid(mfn_x(ram_mfn)) ) >> + put_page(mfn_to_page(mfn_x(ram_mfn))); >> return X86EMUL_UNHANDLEABLE; >> } >> >> @@ -156,7 +173,8 @@ static int hvmemul_do_io( >> { >> gdprintk(XENLOG_WARNING, "WARNING: io already pending >> (%d)?\n", >> p->state); >> - put_gfn(curr->domain, ram_gfn); >> + if ( mfn_valid(mfn_x(ram_mfn)) ) >> + put_page(mfn_to_page(mfn_x(ram_mfn))); >> return X86EMUL_UNHANDLEABLE; >> } >> >> @@ -208,7 +226,8 @@ static int hvmemul_do_io( >> >> if ( rc != X86EMUL_OKAY ) >> { >> - put_gfn(curr->domain, ram_gfn); >> + if ( mfn_valid(mfn_x(ram_mfn)) ) >> + put_page(mfn_to_page(mfn_x(ram_mfn))); >> return rc; >> } >> >> @@ -244,7 +263,8 @@ static int hvmemul_do_io( >> } >> } >> >> - put_gfn(curr->domain, ram_gfn); >> + if ( mfn_valid(mfn_x(ram_mfn)) ) >> + put_page(mfn_to_page(mfn_x(ram_mfn))); >> return X86EMUL_OKAY; >> } >> >> >> >> >> > 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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |