From mboxrd@z Thu Jan 1 00:00:00 1970 From: Zhenzhong Duan Subject: Re: [PATCH] xen: reuse the same pirq allocated when driver load first time Date: Wed, 05 Jun 2013 13:27:22 +0800 Message-ID: <51AECC3A.7060803@oracle.com> References: <20130513182055.GC14177@phenom.dumpdata.com> <20130514142013.GA10173@konrad-lan.dumpdata.com> <5195944A.3050608@oracle.com> <20130520175706.GA27973@phenom.dumpdata.com> <20130520203855.GA30616@phenom.dumpdata.com> <519B474E.4000202@citrix.com> <20130521134059.GE492@phenom.dumpdata.com> Reply-To: zhenzhong.duan@oracle.com Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============7229036289421333676==" Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Stefano Stabellini Cc: "xen-devel@lists.xensource.com" , Konrad Rzeszutek Wilk , Feng Jin , "linux-kernel@vger.kernel.org" , Yuval Shaia , Chien Yen , Ingo Molnar , David Vrabel , "H. Peter Anvin" , Thomas Gleixner List-Id: xen-devel@lists.xenproject.org This is a multi-part message in MIME format. --===============7229036289421333676== Content-Type: multipart/alternative; boundary="------------080805020105030000050103" This is a multi-part message in MIME format. --------------080805020105030000050103 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Stefano Stabellini wrote: > On Tue, 21 May 2013, Stefano Stabellini wrote: > >> On Tue, 21 May 2013, Konrad Rzeszutek Wilk wrote: >> >>>> Looking at the hypervisor code I couldn't see anything obviously wrong. >>>> >>> I think the culprit is "physdev_unmap_pirq": >>> >>> if ( is_hvm_domain(d) ) >>> { >>> spin_lock(&d->event_lock); >>> gdprintk(XENLOG_WARNING,"d%d, pirq: %d is %x %s, irq: %d\n", >>> d->domain_id, pirq, domain_pirq_to_emuirq(d, pirq), >>> domain_pirq_to_emuirq(d, pirq) == IRQ_UNBOUND ? "unbound" : "", >>> domain_pirq_to_irq(d, pirq)); >>> >>> if ( domain_pirq_to_emuirq(d, pirq) != IRQ_UNBOUND ) >>> ret = unmap_domain_pirq_emuirq(d, pirq); >>> spin_unlock(&d->event_lock); >>> if ( domid == DOMID_SELF || ret ) >>> goto free_domain; >>> >>> It always tells me unbound: >>> >>> (XEN) physdev.c:237:d14 14, pirq: 54 is ffffffff >>> (XEN) irq.c:1873:d14 14, nr_pirqs: 56 >>> (XEN) physdev.c:237:d14 14, pirq: 53 is ffffffff >>> (XEN) irq.c:1873:d14 14, nr_pirqs: 56 >>> (XEN) physdev.c:237:d14 14, pirq: 52 is ffffffff >>> (XEN) irq.c:1873:d14 14, nr_pirqs: 56 >>> (XEN) physdev.c:237:d14 14, pirq: 51 is ffffffff >>> (XEN) irq.c:1873:d14 14, nr_pirqs: 56 >>> (XEN) physdev.c:237:d14 14, pirq: 50 is ffffffff >>> (XEN) irq.c:1873:d14 14, nr_pirqs: 56 >>> (a bit older debug code, so the 'unbound' does not show up here). >>> >>> Which means that the call to unmap_domain_pirq_emuirq does not happen. >>> The checks in unmap_domain_pirq_emuirq also look to be depend >>> on the code being IRQ_UNBOUND. >>> >>> In other words, all of that code looks to only clear things when >>> they are !IRQ_UNBOUND. >>> >>> But the other logic (IRQ_UNBOUND) looks to be missing a removal >>> in the radix tree: >>> >>> if ( emuirq != IRQ_PT ) >>> radix_tree_delete(&d->arch.hvm_domain.emuirq_pirq, emuirq); >>> >>> And I think that is what is causing the leak - the radix tree >>> needs to be pruned? Or perhaps the allocate_pirq should check >>> the radix tree for IRQ_UNBOUND ones and re-use them? >>> >> I think that you are looking in the wrong place. >> The issue is that QEMU doesn't call pt_msi_disable in >> pt_msgctrl_reg_write if (!val & PCI_MSI_FLAGS_ENABLE). >> >> The code above is correct as is because it is trying to handle emulated >> IRQs and MSIs, not real passthrough MSIs. They latter are not added to >> that radix tree, see physdev_hvm_map_pirq and physdev_map_pirq. >> > > > This patch fixes the issue, I have only tested MSI (MSI-X completely > untested). > > > diff --git a/hw/pass-through.c b/hw/pass-through.c > index 304c438..079e465 100644 > --- a/hw/pass-through.c > +++ b/hw/pass-through.c > @@ -3866,7 +3866,11 @@ static int pt_msgctrl_reg_write(struct pt_dev *ptdev, > ptdev->msi->flags |= PCI_MSI_FLAGS_ENABLE; > } > else > - ptdev->msi->flags &= ~PCI_MSI_FLAGS_ENABLE; > + { > + if (ptdev->msi->flags & PT_MSI_MAPPED) { > + pt_msi_disable(ptdev); > + } > + } > > /* pass through MSI_ENABLE bit when no MSI-INTx translation */ > if (!ptdev->msi_trans_en) { > @@ -4013,6 +4017,8 @@ static int pt_msixctrl_reg_write(struct pt_dev *ptdev, > pt_disable_msi_translate(ptdev); > } > pt_msix_update(ptdev); > + } else if (!(*value & PCI_MSIX_ENABLE) && ptdev->msix->enabled) { > + pt_msix_delete(ptdev); > Hi Stefano, I made a test with this patch, os reboot when driver reload. If use pt_msix_disable instead of pt_msix_delete, driver could be reloaded. But I still see some error in qemu.log and xen console. Seems four IRQs are not freed when unmap. --------------first load--------------------------- pt_msix_update_one: pt_msix_update_one requested pirq = 103 pt_msix_update_one: Update msix entry 0 with pirq 67 gvec 0 pt_msix_update_one: pt_msix_update_one requested pirq = 102 pt_msix_update_one: Update msix entry 1 with pirq 66 gvec 0 pt_msix_update_one: pt_msix_update_one requested pirq = 101 pt_msix_update_one: Update msix entry 2 with pirq 65 gvec 0 pt_msix_update_one: pt_msix_update_one requested pirq = 100 pt_msix_update_one: Update msix entry 3 with pirq 64 gvec 0 ------------- first unload--------------------------- pt_msix_disable: Unbind msix with pirq 67, gvec 0 pt_msix_disable: Unmap msix with pirq 67 pt_msix_disable: Error: Unmapping of MSI-X failed. [00:04.0] pt_msix_disable: Unbind msix with pirq 66, gvec 0 pt_msix_disable: Unmap msix with pirq 66 pt_msix_disable: Error: Unmapping of MSI-X failed. [00:04.0] pt_msix_disable: Unbind msix with pirq 65, gvec 0 pt_msix_disable: Unmap msix with pirq 65 pt_msix_disable: Error: Unmapping of MSI-X failed. [00:04.0] pt_msix_disable: Unbind msix with pirq 64, gvec 0 pt_msix_disable: Unmap msix with pirq 64 pt_msix_disable: Error: Unmapping of MSI-X failed. [00:04.0] --------------second load--------------------------- pt_msix_update_one: pt_msix_update_one requested pirq = 99 pt_msix_update_one: Update msix entry 0 with pirq 63 gvec 0 pt_msix_update_one: pt_msix_update_one requested pirq = 98 pt_msix_update_one: Update msix entry 1 with pirq 62 gvec 0 pt_msix_update_one: pt_msix_update_one requested pirq = 97 pt_msix_update_one: Update msix entry 2 with pirq 61 gvec 0 pt_msix_update_one: pt_msix_update_one requested pirq = 96 pt_msix_update_one: Update msix entry 3 with pirq 60 gvec 0 xm debug-keys i (XEN) IRQ: 222 affinity:00000000,00000000,00000000,00000000,00002000 vec:a9 type=PCI-MSI status=00000042 mapped, unbound (XEN) IRQ: 223 affinity:00000000,00000000,00000000,00000000,00002000 vec:c1 type=PCI-MSI status=00000042 mapped, unbound (XEN) IRQ: 224 affinity:00000000,00000000,00000000,00000000,00002000 vec:22 type=PCI-MSI status=00000042 mapped, unbound (XEN) IRQ: 225 affinity:00000000,00000000,00000000,00000000,00000001 vec:33 type=PCI-MSI status=00000002 mapped, unbound (XEN) IRQ: 226 affinity:00000000,00000000,00000000,00000000,00000400 vec:b2 type=PCI-MSI status=00000010 in-flight=0 domain-list=7: 99(----), (XEN) IRQ: 227 affinity:00000000,00000000,00000000,00000000,00000001 vec:63 type=PCI-MSI status=00000050 in-flight=0 domain-list=7: 98(----), (XEN) IRQ: 228 affinity:00000000,00000000,00000000,00000000,00000001 vec:6b type=PCI-MSI status=00000050 in-flight=0 domain-list=7: 97(----), (XEN) IRQ: 229 affinity:00000000,00000000,00000000,00000000,00000004 vec:8c type=PCI-MSI status=00000051 in-flight=0 domain-list=7: 96(----), > } > > ptdev->msix->enabled = !!(*value & PCI_MSIX_ENABLE); > diff --git a/hw/pt-msi.c b/hw/pt-msi.c > index b03b989..65fa7d6 100644 > --- a/hw/pt-msi.c > +++ b/hw/pt-msi.c > @@ -213,7 +213,8 @@ void pt_msi_disable(struct pt_dev *dev) > > out: > /* clear msi info */ > - dev->msi->flags &= ~(MSI_FLAG_UNINIT | PT_MSI_MAPPED | PCI_MSI_FLAGS_ENABLE); > + dev->msi->flags &= ~(PT_MSI_MAPPED | PCI_MSI_FLAGS_ENABLE); > + dev->msi->flags |= MSI_FLAG_UNINIT; > dev->msi->pirq = -1; > dev->msi_trans_en = 0; > } > --------------080805020105030000050103 Content-Type: text/html; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Stefano Stabellini wrote: 
On Tue, 21 May 2013, Stefano Stabellini wrote:
  
On Tue, 21 May 2013, Konrad Rzeszutek Wilk wrote:
    
Looking at the hypervisor code I couldn't see anything obviously wrong.
        
I think the culprit is "physdev_unmap_pirq":

   if ( is_hvm_domain(d) )                                                     
    {                                                                           
        spin_lock(&d->event_lock);                                              
        gdprintk(XENLOG_WARNING,"d%d, pirq: %d is %x %s, irq: %d\n",            
            d->domain_id, pirq, domain_pirq_to_emuirq(d, pirq),                 
            domain_pirq_to_emuirq(d, pirq) == IRQ_UNBOUND ? "unbound" : "",        
            domain_pirq_to_irq(d, pirq));                                       
                                                                                
        if ( domain_pirq_to_emuirq(d, pirq) != IRQ_UNBOUND )                    
            ret = unmap_domain_pirq_emuirq(d, pirq);                            
        spin_unlock(&d->event_lock);                                            
        if ( domid == DOMID_SELF || ret )                                       
            goto free_domain;                                             

It always tells me unbound:

(XEN) physdev.c:237:d14 14, pirq: 54 is ffffffff
(XEN) irq.c:1873:d14 14, nr_pirqs: 56
(XEN) physdev.c:237:d14 14, pirq: 53 is ffffffff
(XEN) irq.c:1873:d14 14, nr_pirqs: 56
(XEN) physdev.c:237:d14 14, pirq: 52 is ffffffff
(XEN) irq.c:1873:d14 14, nr_pirqs: 56
(XEN) physdev.c:237:d14 14, pirq: 51 is ffffffff
(XEN) irq.c:1873:d14 14, nr_pirqs: 56
(XEN) physdev.c:237:d14 14, pirq: 50 is ffffffff
(XEN) irq.c:1873:d14 14, nr_pirqs: 56
(a bit older debug code, so the 'unbound' does not show up here).

Which means that the call to unmap_domain_pirq_emuirq does not happen.
The checks in unmap_domain_pirq_emuirq also look to be depend
on the code being IRQ_UNBOUND.

In other words, all of that code looks to only clear things when
they are !IRQ_UNBOUND.

But the other logic (IRQ_UNBOUND) looks to be missing a removal
in the radix tree:

  if ( emuirq != IRQ_PT )                                                     
        radix_tree_delete(&d->arch.hvm_domain.emuirq_pirq, emuirq);             
                                                                        
And I think that is what is causing the leak - the radix tree
needs to be pruned? Or perhaps the allocate_pirq should check
the radix tree for IRQ_UNBOUND ones and re-use them?
      
I think that you are looking in the wrong place.
The issue is that QEMU doesn't call pt_msi_disable in
pt_msgctrl_reg_write if (!val & PCI_MSI_FLAGS_ENABLE).

The code above is correct as is because it is trying to handle emulated
IRQs and MSIs, not real passthrough MSIs. They latter are not added to
that radix tree, see physdev_hvm_map_pirq and physdev_map_pirq.
    


This patch fixes the issue, I have only tested MSI (MSI-X completely
untested).


diff --git a/hw/pass-through.c b/hw/pass-through.c
index 304c438..079e465 100644
--- a/hw/pass-through.c
+++ b/hw/pass-through.c
@@ -3866,7 +3866,11 @@ static int pt_msgctrl_reg_write(struct pt_dev *ptdev,
         ptdev->msi->flags |= PCI_MSI_FLAGS_ENABLE;
     }
     else
-        ptdev->msi->flags &= ~PCI_MSI_FLAGS_ENABLE;
+    {
+        if (ptdev->msi->flags & PT_MSI_MAPPED) {
+            pt_msi_disable(ptdev);
+        }
+    }
 
     /* pass through MSI_ENABLE bit when no MSI-INTx translation */
     if (!ptdev->msi_trans_en) {
@@ -4013,6 +4017,8 @@ static int pt_msixctrl_reg_write(struct pt_dev *ptdev,
             pt_disable_msi_translate(ptdev);
         }
         pt_msix_update(ptdev);
+    } else if (!(*value & PCI_MSIX_ENABLE) && ptdev->msix->enabled) {
+        pt_msix_delete(ptdev);
Hi Stefano,
I made a test with this patch, os reboot when driver reload. If use pt_msix_disable instead of pt_msix_delete, driver could be reloaded.
But I still see some error in qemu.log and xen console. Seems four IRQs are not freed when unmap.
--------------first load---------------------------
pt_msix_update_one: pt_msix_update_one requested pirq = 103
pt_msix_update_one: Update msix entry 0 with pirq 67 gvec 0
pt_msix_update_one: pt_msix_update_one requested pirq = 102
pt_msix_update_one: Update msix entry 1 with pirq 66 gvec 0
pt_msix_update_one: pt_msix_update_one requested pirq = 101
pt_msix_update_one: Update msix entry 2 with pirq 65 gvec 0
pt_msix_update_one: pt_msix_update_one requested pirq = 100
pt_msix_update_one: Update msix entry 3 with pirq 64 gvec 0
------------- first unload---------------------------
pt_msix_disable: Unbind msix with pirq 67, gvec 0
pt_msix_disable: Unmap msix with pirq 67
pt_msix_disable: Error: Unmapping of MSI-X failed. [00:04.0]
pt_msix_disable: Unbind msix with pirq 66, gvec 0
pt_msix_disable: Unmap msix with pirq 66
pt_msix_disable: Error: Unmapping of MSI-X failed. [00:04.0]
pt_msix_disable: Unbind msix with pirq 65, gvec 0
pt_msix_disable: Unmap msix with pirq 65
pt_msix_disable: Error: Unmapping of MSI-X failed. [00:04.0]
pt_msix_disable: Unbind msix with pirq 64, gvec 0
pt_msix_disable: Unmap msix with pirq 64
pt_msix_disable: Error: Unmapping of MSI-X failed. [00:04.0]
--------------second load---------------------------
pt_msix_update_one: pt_msix_update_one requested pirq = 99
pt_msix_update_one: Update msix entry 0 with pirq 63 gvec 0
pt_msix_update_one: pt_msix_update_one requested pirq = 98
pt_msix_update_one: Update msix entry 1 with pirq 62 gvec 0
pt_msix_update_one: pt_msix_update_one requested pirq = 97
pt_msix_update_one: Update msix entry 2 with pirq 61 gvec 0
pt_msix_update_one: pt_msix_update_one requested pirq = 96
pt_msix_update_one: Update msix entry 3 with pirq 60 gvec 0

xm debug-keys i
(XEN)    IRQ: 222 affinity:00000000,00000000,00000000,00000000,00002000 vec:a9 type=PCI-MSI         status=00000042 mapped, unbound
(XEN)    IRQ: 223 affinity:00000000,00000000,00000000,00000000,00002000 vec:c1 type=PCI-MSI         status=00000042 mapped, unbound
(XEN)    IRQ: 224 affinity:00000000,00000000,00000000,00000000,00002000 vec:22 type=PCI-MSI         status=00000042 mapped, unbound
(XEN)    IRQ: 225 affinity:00000000,00000000,00000000,00000000,00000001 vec:33 type=PCI-MSI         status=00000002 mapped, unbound
(XEN)    IRQ: 226 affinity:00000000,00000000,00000000,00000000,00000400 vec:b2 type=PCI-MSI         status=00000010 in-flight=0 domain-list=7: 99(----),
(XEN)    IRQ: 227 affinity:00000000,00000000,00000000,00000000,00000001 vec:63 type=PCI-MSI         status=00000050 in-flight=0 domain-list=7: 98(----),
(XEN)    IRQ: 228 affinity:00000000,00000000,00000000,00000000,00000001 vec:6b type=PCI-MSI         status=00000050 in-flight=0 domain-list=7: 97(----),
(XEN)    IRQ: 229 affinity:00000000,00000000,00000000,00000000,00000004 vec:8c type=PCI-MSI         status=00000051 in-flight=0 domain-list=7: 96(----),

     }
 
     ptdev->msix->enabled = !!(*value & PCI_MSIX_ENABLE);
diff --git a/hw/pt-msi.c b/hw/pt-msi.c
index b03b989..65fa7d6 100644
--- a/hw/pt-msi.c
+++ b/hw/pt-msi.c
@@ -213,7 +213,8 @@ void pt_msi_disable(struct pt_dev *dev)
 
 out:
     /* clear msi info */
-    dev->msi->flags &= ~(MSI_FLAG_UNINIT | PT_MSI_MAPPED | PCI_MSI_FLAGS_ENABLE);
+    dev->msi->flags &= ~(PT_MSI_MAPPED | PCI_MSI_FLAGS_ENABLE);
+    dev->msi->flags |= MSI_FLAG_UNINIT;
     dev->msi->pirq = -1;
     dev->msi_trans_en = 0;
 }
--------------080805020105030000050103-- --===============7229036289421333676== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============7229036289421333676==--