[PATCH] qemu-xen-trad: free all the pirqs for msi/msix when driver unload

[PATCH] qemu-xen-trad: free all the pirqs for msi/msix when driver unload

[Zhenzhong Duan]

Pirqs are not freed when driver unloads, then new pirqs are allocated when
driver reloads. This could exhaust pirqs if do it in a loop.

This patch fixes the bug by freeing pirqs when ENABLE bit is cleared in
msi/msix control reg.

There is also other way of fixing it such as reuse pirqs between driver reload,
but this way is better.
Xen-devel: http://marc.info/?l=xen-devel&m=136800120304275&w=2

Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
---
 hw/pass-through.c |    8 +++++++-
 hw/pt-msi.c       |    5 +++--
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/hw/pass-through.c b/hw/pass-through.c
index 304c438..4821182 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_disable(ptdev);
     }
 
     ptdev->msix->enabled = !!(*value & PCI_MSIX_ENABLE);
diff --git a/hw/pt-msi.c b/hw/pt-msi.c
index b03b989..3f5f94b 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;
 }
@@ -447,7 +448,7 @@ static void pci_msix_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
     {
         const volatile uint32_t *vec_ctrl;
 
-        if ( entry->io_mem[offset] == val )
+        if ( entry->io_mem[offset] == val && entry->pirq != -1)
             return;
 
         /*
-- 
1.7.3

Re: [PATCH] qemu-xen-trad: free all the pirqs for msi/msix when driver unload

[George Dunlap]

On Mon, Jul 1, 2013 at 4:34 AM, Zhenzhong Duan
<zhenzhong.duan@oracle.com> wrote:
> Pirqs are not freed when driver unloads, then new pirqs are allocated when
> driver reloads. This could exhaust pirqs if do it in a loop.
>
> This patch fixes the bug by freeing pirqs when ENABLE bit is cleared in
> msi/msix control reg.
>
> There is also other way of fixing it such as reuse pirqs between driver reload,
> but this way is better.
> Xen-devel: http://marc.info/?l=xen-devel&m=136800120304275&w=2
>
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>

Thanks Zhenzhong.

>From what I understand following the conversation, I think this is
probably the right way to solve the problem, but given that it's only
really a problem when you load and unload drivers, which is the
uncommon case, I think at this point we should probably hold off on
this one until 4.3.1.

Stefano, thoughts?

 -George

Re: [PATCH] qemu-xen-trad: free all the pirqs for msi/msix when driver unload

[Stefano Stabellini]

On Mon, 1 Jul 2013, George Dunlap wrote:
> On Mon, Jul 1, 2013 at 4:34 AM, Zhenzhong Duan
> <zhenzhong.duan@oracle.com> wrote:
> > Pirqs are not freed when driver unloads, then new pirqs are allocated when
> > driver reloads. This could exhaust pirqs if do it in a loop.
> >
> > This patch fixes the bug by freeing pirqs when ENABLE bit is cleared in
> > msi/msix control reg.
> >
> > There is also other way of fixing it such as reuse pirqs between driver reload,
> > but this way is better.
> > Xen-devel: http://marc.info/?l=xen-devel&m=136800120304275&w=2
> >
> > Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>
> 
> Thanks Zhenzhong.
> 
> >From what I understand following the conversation, I think this is
> probably the right way to solve the problem, but given that it's only
> really a problem when you load and unload drivers, which is the
> uncommon case, I think at this point we should probably hold off on
> this one until 4.3.1.
> 
> Stefano, thoughts?

I think that's OK. I'll wait to apply the qemu-xen patch until after the
release.

Re: [PATCH] qemu-xen-trad: free all the pirqs for msi/msix when driver unload

[Stefano Stabellini]

On Mon, 1 Jul 2013, Zhenzhong Duan wrote:
> Pirqs are not freed when driver unloads, then new pirqs are allocated when
> driver reloads. This could exhaust pirqs if do it in a loop.
> 
> This patch fixes the bug by freeing pirqs when ENABLE bit is cleared in
> msi/msix control reg.
> 
> There is also other way of fixing it such as reuse pirqs between driver reload,
> but this way is better.
> Xen-devel: http://marc.info/?l=xen-devel&m=136800120304275&w=2
> 
> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@oracle.com>

Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>


>  hw/pass-through.c |    8 +++++++-
>  hw/pt-msi.c       |    5 +++--
>  2 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/pass-through.c b/hw/pass-through.c
> index 304c438..4821182 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_disable(ptdev);
>      }
>  
>      ptdev->msix->enabled = !!(*value & PCI_MSIX_ENABLE);
> diff --git a/hw/pt-msi.c b/hw/pt-msi.c
> index b03b989..3f5f94b 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;
>  }
> @@ -447,7 +448,7 @@ static void pci_msix_writel(void *opaque, target_phys_addr_t addr, uint32_t val)
>      {
>          const volatile uint32_t *vec_ctrl;
>  
> -        if ( entry->io_mem[offset] == val )
> +        if ( entry->io_mem[offset] == val && entry->pirq != -1)
>              return;
>  
>          /*
> -- 
> 1.7.3
>