[PATCH] Fix qemu-xen with PCI passthrough.

[PATCH] Fix qemu-xen with PCI passthrough.

[konrad]

Hey Anthony and Stefano,

This patch was posted in the past (see
http://lists.xen.org/archives/html/xen-devel/2013-07/msg00008.html)
and Stefano said: ""It looks OK to me, I'll have to trust you to have
tested this patch properly too.".

I don't recall if Zhenzhong did, but I just did with Xen-unstable
and the latest of qemu-xen. Without this patch, this loop

while (true)
do
 rmmod igbvf
 killall dhclient
 modprobe igbvf
 dhclient eth1
done

ends up shorlty exhausting the list of PIRQs and the driver cannot load
anymore. With this patch the loop continues on.

This has been tested with an 82576 VF.

Please pull!

 hw/xen/xen_pt_config_init.c |    6 ++++--
 hw/xen/xen_pt_msi.c         |    6 ++++--
 2 files changed, 8 insertions(+), 4 deletions(-)

Zhenzhong Duan (1):
      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

[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!

[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.