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

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

[Zhenzhong Duan]

Pirqs are not freed when driver unload, then new pirqs are allocated when
driver reload. 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/xen_pt_config_init.c |    6 ++++--
 hw/xen_pt_msi.c         |    6 ++++--
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/hw/xen_pt_config_init.c b/hw/xen_pt_config_init.c
index 0a5f82c..68d1195 100644
--- a/hw/xen_pt_config_init.c
+++ b/hw/xen_pt_config_init.c
@@ -1123,8 +1123,8 @@ static int xen_pt_msgctrl_reg_write(XenPCIPassthroughState *s,
             msi->mapped = true;
         }
         msi->flags |= PCI_MSI_FLAGS_ENABLE;
-    } else {
-        msi->flags &= ~PCI_MSI_FLAGS_ENABLE;
+    } else if (msi->mapped) {
+        xen_pt_msi_disable(s);
     }
 
     /* pass through MSI_ENABLE bit */
@@ -1397,6 +1397,8 @@ static int xen_pt_msixctrl_reg_write(XenPCIPassthroughState *s,
     if ((*val & PCI_MSIX_FLAGS_ENABLE)
         && !(*val & PCI_MSIX_FLAGS_MASKALL)) {
         xen_pt_msix_update(s);
+    } else if (!(*val & PCI_MSIX_FLAGS_ENABLE) && s->msix->enabled) {
+        xen_pt_msix_disable(s);
     }
 
     debug_msix_enabled_old = s->msix->enabled;
diff --git a/hw/xen_pt_msi.c b/hw/xen_pt_msi.c
index db757cd..733c991 100644
--- a/hw/xen_pt_msi.c
+++ b/hw/xen_pt_msi.c
@@ -282,7 +282,8 @@ void xen_pt_msi_disable(XenPCIPassthroughState *s)
                      msi->initialized);
 
     /* clear msi info */
-    msi->flags = 0;
+    msi->flags &= ~PCI_MSI_FLAGS_ENABLE;
+    msi->initialized = false;
     msi->mapped = false;
     msi->pirq = XEN_PT_UNASSIGNED_PIRQ;
 }
@@ -446,7 +447,8 @@ static void pci_msix_write(void *opaque, hwaddr addr,
     if (offset != PCI_MSIX_ENTRY_VECTOR_CTRL) {
         const volatile uint32_t *vec_ctrl;
 
-        if (get_entry_value(entry, offset) == val) {
+        if (get_entry_value(entry, offset) == val
+            && entry->pirq != XEN_PT_UNASSIGNED_PIRQ) {
             return;
         }
 
-- 
1.7.3

Re: [PATCH] qemu-xen: 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 unload, then new pirqs are allocated when
> driver reload. 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>

It looks OK to me, I'll have to trust you to have tested this patch
properly too.



>  hw/xen_pt_config_init.c |    6 ++++--
>  hw/xen_pt_msi.c         |    6 ++++--
>  2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/xen_pt_config_init.c b/hw/xen_pt_config_init.c
> index 0a5f82c..68d1195 100644
> --- a/hw/xen_pt_config_init.c
> +++ b/hw/xen_pt_config_init.c
> @@ -1123,8 +1123,8 @@ static int xen_pt_msgctrl_reg_write(XenPCIPassthroughState *s,
>              msi->mapped = true;
>          }
>          msi->flags |= PCI_MSI_FLAGS_ENABLE;
> -    } else {
> -        msi->flags &= ~PCI_MSI_FLAGS_ENABLE;
> +    } else if (msi->mapped) {
> +        xen_pt_msi_disable(s);
>      }
>  
>      /* pass through MSI_ENABLE bit */
> @@ -1397,6 +1397,8 @@ static int xen_pt_msixctrl_reg_write(XenPCIPassthroughState *s,
>      if ((*val & PCI_MSIX_FLAGS_ENABLE)
>          && !(*val & PCI_MSIX_FLAGS_MASKALL)) {
>          xen_pt_msix_update(s);
> +    } else if (!(*val & PCI_MSIX_FLAGS_ENABLE) && s->msix->enabled) {
> +        xen_pt_msix_disable(s);
>      }
>  
>      debug_msix_enabled_old = s->msix->enabled;
> diff --git a/hw/xen_pt_msi.c b/hw/xen_pt_msi.c
> index db757cd..733c991 100644
> --- a/hw/xen_pt_msi.c
> +++ b/hw/xen_pt_msi.c
> @@ -282,7 +282,8 @@ void xen_pt_msi_disable(XenPCIPassthroughState *s)
>                       msi->initialized);
>  
>      /* clear msi info */
> -    msi->flags = 0;
> +    msi->flags &= ~PCI_MSI_FLAGS_ENABLE;
> +    msi->initialized = false;
>      msi->mapped = false;
>      msi->pirq = XEN_PT_UNASSIGNED_PIRQ;
>  }
> @@ -446,7 +447,8 @@ static void pci_msix_write(void *opaque, hwaddr addr,
>      if (offset != PCI_MSIX_ENTRY_VECTOR_CTRL) {
>          const volatile uint32_t *vec_ctrl;
>  
> -        if (get_entry_value(entry, offset) == val) {
> +        if (get_entry_value(entry, offset) == val
> +            && entry->pirq != XEN_PT_UNASSIGNED_PIRQ) {
>              return;
>          }
>  
> -- 
> 1.7.3
> 

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

[DuanZhenzhong]

Stefano Stabellini wrote:
> On Mon, 1 Jul 2013, Zhenzhong Duan wrote:
>   
>> Pirqs are not freed when driver unload, then new pirqs are allocated when
>> driver reload. 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>
>>     
>
> It looks OK to me, I'll have to trust you to have tested this patch
> properly too.
>   
Not yet, it just passed build test.
Pls hold off until I got permit to override the 4.1.30-OVM test env with 
upstream things and test.
Or is there some way of testing with 4.1.30-OVM + qemu-system-i386 ?

zduan

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

[Stefano Stabellini]

On Tue, 2 Jul 2013, DuanZhenzhong wrote:
> Stefano Stabellini wrote:
> > On Mon, 1 Jul 2013, Zhenzhong Duan wrote:
> >   
> > > Pirqs are not freed when driver unload, then new pirqs are allocated when
> > > driver reload. 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>
> > >     
> > 
> > It looks OK to me, I'll have to trust you to have tested this patch
> > properly too.
> >   
> Not yet, it just passed build test.
> Pls hold off until I got permit to override the 4.1.30-OVM test env with
> upstream things and test.

OK, thanks for letting me know.


> Or is there some way of testing with 4.1.30-OVM + qemu-system-i386 ?

Uhm.. I guess not: you need QMP support in libxl and create VMs with xl
to be able to use qemu-xen.

Your best bet would be to update the hypervisor and tools to 4.2 or 4.3. 

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

[konrad]

From: Zhenzhong Duan <zhenzhong.duan@oracle.com>

Pirqs are not freed when driver unload, then new pirqs are allocated when
driver reload. 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>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
---
 hw/xen/xen_pt_config_init.c |    6 ++++--
 hw/xen/xen_pt_msi.c         |    6 ++++--
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
index 8ccc2e4..de9a20f 100644
--- a/hw/xen/xen_pt_config_init.c
+++ b/hw/xen/xen_pt_config_init.c
@@ -1123,8 +1123,8 @@ static int xen_pt_msgctrl_reg_write(XenPCIPassthroughState *s,
             msi->mapped = true;
         }
         msi->flags |= PCI_MSI_FLAGS_ENABLE;
-    } else {
-        msi->flags &= ~PCI_MSI_FLAGS_ENABLE;
+    } else if (msi->mapped) {
+        xen_pt_msi_disable(s);
     }
 
     /* pass through MSI_ENABLE bit */
@@ -1397,6 +1397,8 @@ static int xen_pt_msixctrl_reg_write(XenPCIPassthroughState *s,
     if ((*val & PCI_MSIX_FLAGS_ENABLE)
         && !(*val & PCI_MSIX_FLAGS_MASKALL)) {
         xen_pt_msix_update(s);
+    } else if (!(*val & PCI_MSIX_FLAGS_ENABLE) && s->msix->enabled) {
+        xen_pt_msix_disable(s);
     }
 
     debug_msix_enabled_old = s->msix->enabled;
diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c
index 6fbe0cc..12b4c45 100644
--- a/hw/xen/xen_pt_msi.c
+++ b/hw/xen/xen_pt_msi.c
@@ -282,7 +282,8 @@ void xen_pt_msi_disable(XenPCIPassthroughState *s)
                      msi->initialized);
 
     /* clear msi info */
-    msi->flags = 0;
+    msi->flags &= ~PCI_MSI_FLAGS_ENABLE;
+    msi->initialized = false;
     msi->mapped = false;
     msi->pirq = XEN_PT_UNASSIGNED_PIRQ;
 }
@@ -446,7 +447,8 @@ static void pci_msix_write(void *opaque, hwaddr addr,
     if (offset != PCI_MSIX_ENTRY_VECTOR_CTRL) {
         const volatile uint32_t *vec_ctrl;
 
-        if (get_entry_value(entry, offset) == val) {
+        if (get_entry_value(entry, offset) == val
+            && entry->pirq != XEN_PT_UNASSIGNED_PIRQ) {
             return;
         }
 
-- 
1.7.7.6

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

[Stefano Stabellini]

On Tue, 8 Apr 2014, konrad@kernel.org wrote:
> From: Zhenzhong Duan <zhenzhong.duan@oracle.com>
> 
> Pirqs are not freed when driver unload, then new pirqs are allocated when
> driver reload. 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>
> Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

I'll add it to my queue.
At the moment they are busy with the QEMU 2.0 release, I would rather
wait for the next merge window.


>  hw/xen/xen_pt_config_init.c |    6 ++++--
>  hw/xen/xen_pt_msi.c         |    6 ++++--
>  2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/xen/xen_pt_config_init.c b/hw/xen/xen_pt_config_init.c
> index 8ccc2e4..de9a20f 100644
> --- a/hw/xen/xen_pt_config_init.c
> +++ b/hw/xen/xen_pt_config_init.c
> @@ -1123,8 +1123,8 @@ static int xen_pt_msgctrl_reg_write(XenPCIPassthroughState *s,
>              msi->mapped = true;
>          }
>          msi->flags |= PCI_MSI_FLAGS_ENABLE;
> -    } else {
> -        msi->flags &= ~PCI_MSI_FLAGS_ENABLE;
> +    } else if (msi->mapped) {
> +        xen_pt_msi_disable(s);
>      }
>  
>      /* pass through MSI_ENABLE bit */
> @@ -1397,6 +1397,8 @@ static int xen_pt_msixctrl_reg_write(XenPCIPassthroughState *s,
>      if ((*val & PCI_MSIX_FLAGS_ENABLE)
>          && !(*val & PCI_MSIX_FLAGS_MASKALL)) {
>          xen_pt_msix_update(s);
> +    } else if (!(*val & PCI_MSIX_FLAGS_ENABLE) && s->msix->enabled) {
> +        xen_pt_msix_disable(s);
>      }
>  
>      debug_msix_enabled_old = s->msix->enabled;
> diff --git a/hw/xen/xen_pt_msi.c b/hw/xen/xen_pt_msi.c
> index 6fbe0cc..12b4c45 100644
> --- a/hw/xen/xen_pt_msi.c
> +++ b/hw/xen/xen_pt_msi.c
> @@ -282,7 +282,8 @@ void xen_pt_msi_disable(XenPCIPassthroughState *s)
>                       msi->initialized);
>  
>      /* clear msi info */
> -    msi->flags = 0;
> +    msi->flags &= ~PCI_MSI_FLAGS_ENABLE;
> +    msi->initialized = false;
>      msi->mapped = false;
>      msi->pirq = XEN_PT_UNASSIGNED_PIRQ;
>  }
> @@ -446,7 +447,8 @@ static void pci_msix_write(void *opaque, hwaddr addr,
>      if (offset != PCI_MSIX_ENTRY_VECTOR_CTRL) {
>          const volatile uint32_t *vec_ctrl;
>  
> -        if (get_entry_value(entry, offset) == val) {
> +        if (get_entry_value(entry, offset) == val
> +            && entry->pirq != XEN_PT_UNASSIGNED_PIRQ) {
>              return;
>          }
>  
> -- 
> 1.7.7.6
> 

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

[Konrad Rzeszutek Wilk]

On Tue, Apr 08, 2014 at 06:22:09PM +0100, Stefano Stabellini wrote:
> On Tue, 8 Apr 2014, konrad@kernel.org wrote:
> > From: Zhenzhong Duan <zhenzhong.duan@oracle.com>
> > 
> > Pirqs are not freed when driver unload, then new pirqs are allocated when
> > driver reload. 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>
> > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> 
> I'll add it to my queue.

Yeey!
> At the moment they are busy with the QEMU 2.0 release, I would rather
> wait for the next merge window.

OK. When should I ping you so that in the unlikely event it gets lost
I would ping you?

Thanks!

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

[Stefano Stabellini]

On Tue, 8 Apr 2014, Konrad Rzeszutek Wilk wrote:
> On Tue, Apr 08, 2014 at 06:22:09PM +0100, Stefano Stabellini wrote:
> > On Tue, 8 Apr 2014, konrad@kernel.org wrote:
> > > From: Zhenzhong Duan <zhenzhong.duan@oracle.com>
> > > 
> > > Pirqs are not freed when driver unload, then new pirqs are allocated when
> > > driver reload. 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>
> > > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > 
> > I'll add it to my queue.
> 
> Yeey!
> > At the moment they are busy with the QEMU 2.0 release, I would rather
> > wait for the next merge window.
> 
> OK. When should I ping you so that in the unlikely event it gets lost
> I would ping you?

As soon as QEMU makes the 2.0 release.

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

[Konrad Rzeszutek Wilk]

On Tue, Apr 08, 2014 at 01:29:45PM -0400, Konrad Rzeszutek Wilk wrote:
> On Tue, Apr 08, 2014 at 06:22:09PM +0100, Stefano Stabellini wrote:
> > On Tue, 8 Apr 2014, konrad@kernel.org wrote:
> > > From: Zhenzhong Duan <zhenzhong.duan@oracle.com>
> > > 
> > > Pirqs are not freed when driver unload, then new pirqs are allocated when
> > > driver reload. 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>
> > > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> > 
> > I'll add it to my queue.
> 
> Yeey!
> > At the moment they are busy with the QEMU 2.0 release, I would rather
> > wait for the next merge window.
> 
> OK. When should I ping you so that in the unlikely event it gets lost
> I would ping you?

ping! Seems QEMU 2.0 is out
> 
> Thanks!