Re: [Xen-devel] Deadlocks by p2m_lock and event_lock

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

> -----Original Message-----
> From: Tim Deegan [mailto:tim@xxxxxxx]
> Sent: Friday, March 09, 2012 7:20 PM
> To: Hao, Xudong
> Cc: JBeulich@xxxxxxxx; Andres Lagar-Cavilla; xen-devel@xxxxxxxxxxxxxxxxxxx;
> Keir Fraser; Zhang, Xiantao
> Subject: Re: [Xen-devel] Deadlocks by p2m_lock and event_lock
> 
> Hi,
> 
> At 10:58 +0000 on 09 Mar (1331290728), Hao, Xudong wrote:
> > ====CPU0===
> > map_domain_pirq()    Grab event_lock
> >   /
> > Pci_enable_msi()
> >   /
> > msix_capability_init()
> >   /
> > p2m_change_entry_type_global()   Trying to acquire p2m_lock
> >
> > ====CPU9===
> > hvm_hap_nested_page_fault() -> get_gfn_type_access()   Grab p2m_lock
> >   /
> > handle_mmio()
> >   /
> > ...
> >   /
> > notify_via_xen_event_channel()    Trying to acquire event_lock
> >
> >
> > The event_lock is used anywhere in Xen, I only have a patch of workaround
> this issue for proposal, but not for the final fix. Any good suggestion?
> >
> > diff -r f61120046915 xen/arch/x86/irq.c
> > --- a/xen/arch/x86/irq.c    Wed Mar 07 11:50:31 2012 +0100
> > +++ b/xen/arch/x86/irq.c    Sat Mar 10 02:06:18 2012 +0800
> > @@ -1875,10 +1875,12 @@ int map_domain_pirq(
> >          if ( !cpu_has_apic )
> >              goto done;
> >
> > +        spin_unlock(&d->event_lock);
> >          pdev = pci_get_pdev(msi->seg, msi->bus, msi->devfn);
> >          ret = pci_enable_msi(msi, &msi_desc);
> >          if ( ret )
> >              goto done;
> > +        spin_lock(&d->event_lock);
> >
> >          spin_lock_irqsave(&desc->lock, flags);
> >
> > Best Regards,
> > Xudong Hao
> 
> 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.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel