|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] 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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |