Re: Deadlocks by p2m_lock and event_lock

["Andres Lagar-Cavilla" <andres@lagarcavilla.org>]

>
>> -----Original Message-----
>> From: Tim Deegan [mailto:tim@xen.org]
>> Sent: Friday, March 09, 2012 7:20 PM
>> To: Hao, Xudong
>> Cc: JBeulich@suse.com; Andres Lagar-Cavilla;
>> xen-devel@lists.xensource.com;
>> 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.
Ack
Thanks,
Andres
>