From mboxrd@z Thu Jan  1 00:00:00 1970
From: "Andres Lagar-Cavilla" <andres@lagarcavilla.org>
Subject: Re: Deadlocks by p2m_lock and event_lock
Date: Fri, 9 Mar 2012 08:29:27 -0800
Message-ID: <763b511f59616a274ff142d62f55f7bf.squirrel@webmail.lagarcavilla.org>
References: <403610A45A2B5242BD291EDAE8B37D300FCE3236@SHSMSX102.ccr.corp.intel.com>
	<20120309112010.GB83422@ocelot.phlegethon.org>
	<403610A45A2B5242BD291EDAE8B37D300FCE32B2@SHSMSX102.ccr.corp.intel.com>
Reply-To: andres@lagarcavilla.org
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <403610A45A2B5242BD291EDAE8B37D300FCE32B2@SHSMSX102.ccr.corp.intel.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: "Hao, Xudong" <xudong.hao@intel.com>
Cc: Keir Fraser <keir@xen.org>, "xen-devel@lists.xensource.com" <xen-devel@lists.xensource.com>, Tim Deegan <tim@xen.org>, "Zhang, Xiantao" <xiantao.zhang@intel.com>, "JBeulich@suse.com" <jbeulich@suse.com>
List-Id: xen-devel@lists.xenproject.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
>