[PATCH] Fix qemu traditional with PCI passthrough.

[PATCH] Fix qemu traditional with PCI passthrough.

[Konrad Rzeszutek Wilk]

Hey Ian and Stefano,

This patch was posted way back last year in July 
(see http://lists.xen.org/archives/html/xen-devel/2013-07/msg00004.html) and
was mentioned to:

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

I think this patch just got lost in the Xen 4.4 release. Dusting it
off and reposting.

The issue at hand is simple - you boot an PVHVM guest with a PCI
passthrough device and in a loop do:

#!/bin/bash

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


and you find yourself in distressed to see that after a while it
cannot allocate any IRQs. I've tested it and it fixes the issue.


Now there was also an qemu-xen version of this patch posted:
http://lists.xen.org/archives/html/xen-devel/2013-07/msg00008.html

and I just reposted it, see:
http://mid.gmane.org/1396975053-16435-1-git-send-email-konrad@kernel.org
(or "[PATCH] Fix qemu-xen with PCI passthrough.")


 hw/pass-through.c | 8 +++++++-
 hw/pt-msi.c       | 5 +++--
 2 files changed, 10 insertions(+), 3 deletions(-)

Zhenzhong Duan (1):
      qemu-xen-trad: free all the pirqs for msi/msix when driver unloads

Thanks!

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

[Konrad Rzeszutek Wilk]

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

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>
Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@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.8.5.3

Re: [PATCH] Fix qemu traditional with PCI passthrough. [and 2 more messages]

[Ian Jackson]

Ian Jackson writes ("Re: [PATCH] Fix qemu traditional with PCI passthrough."):
> Konrad Rzeszutek Wilk writes ("[PATCH] Fix qemu traditional with PCI passthrough."):
> > I think this patch just got lost in the Xen 4.4 release. Dusting it
> > off and reposting.
> 
> Quite likely.
> 
> I'll wait for Stefano to comment on the qemu-xen patch but I don't see
> a problem with applying this to staging, and it should probably go on
> my backport list.

I see this is in our qemu-upstream tree (as
d0395cc49b2ec6d1723c01f1daf2394b9264ca29).  I have applied it to
qemu-xen-traditional and added it to my backport list.

Thanks,
Ian.

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

[Ian Jackson]

Konrad Rzeszutek Wilk writes ("[Xen-devel] [PATCH] qemu-xen-trad: free all the pirqs for msi/msix when driver unloads"):
> From: Zhenzhong Duan <zhenzhong.duan@oracle.com>
> 
> 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.

I have backported this to 4.4 and 4.3.  It did not apply cleanly to
4.2 (and I have not investgated why).

Thanks,
Ian.

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

[Zhenzhong Duan]

There is a patch dependency missed.

commit adf74189dd58014744a4b8c9d64407d629da5e2f
Author: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Date:   Mon Dec 10 12:43:33 2012 +0000

     qemu-xen-trad/pt_msi_disable: do not clear all MSI flags

     "qemu-xen-trad: fix msi_translate with PV event delivery" added a
     pt_msi_disable() call into pt_msgctrl_reg_write, clearing the MSI flags
     as a consequence. MSIs get enabled again soon after by calling
     pt_msi_setup.

     However the MSI flags are only setup once in the pt_msgctrl_reg_init
     function, so from the QEMU point of view the device has lost some
     important properties, like for example PCI_MSI_FLAGS_64BIT.

     This patch fixes the bug by clearing only the MSI
     enabled/mapped/initialized flags in pt_msi_disable.

     Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
     Tested-by: G.R. <firemeteor@users.sourceforge.net>
     Xen-devel: http://marc.info/?l=xen-devel&m=135489879503075

diff --git a/hw/pt-msi.c b/hw/pt-msi.c
index 73f737d..b03b989 100644
--- a/hw/pt-msi.c
+++ b/hw/pt-msi.c
@@ -213,7 +213,7 @@ void pt_msi_disable(struct pt_dev *dev)

  out:
      /* clear msi info */
-    dev->msi->flags = 0;
+    dev->msi->flags &= ~(MSI_FLAG_UNINIT | PT_MSI_MAPPED | 
PCI_MSI_FLAGS_ENABLE);
      dev->msi->pirq = -1;
      dev->msi_trans_en = 0;
  }

On 2014-7-2 23:06, Ian Jackson wrote:
> Konrad Rzeszutek Wilk writes ("[Xen-devel] [PATCH] qemu-xen-trad: free all the pirqs for msi/msix when driver unloads"):
>> From: Zhenzhong Duan<zhenzhong.duan@oracle.com>
>>
>> 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.
> I have backported this to 4.4 and 4.3.  It did not apply cleanly to
> 4.2 (and I have not investgated why).
>
> Thanks,
> Ian.

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

[Konrad Rzeszutek Wilk]

On Thu, Jul 03, 2014 at 11:12:18AM +0800, Zhenzhong Duan wrote:
> There is a patch dependency missed.

Is that one upstream as well? Or does it not need to be?
If it is upstream, what is the title/commit id of that one?

Thank you!
> 
> commit adf74189dd58014744a4b8c9d64407d629da5e2f
> Author: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Date:   Mon Dec 10 12:43:33 2012 +0000
> 
>     qemu-xen-trad/pt_msi_disable: do not clear all MSI flags
> 
>     "qemu-xen-trad: fix msi_translate with PV event delivery" added a
>     pt_msi_disable() call into pt_msgctrl_reg_write, clearing the MSI flags
>     as a consequence. MSIs get enabled again soon after by calling
>     pt_msi_setup.
> 
>     However the MSI flags are only setup once in the pt_msgctrl_reg_init
>     function, so from the QEMU point of view the device has lost some
>     important properties, like for example PCI_MSI_FLAGS_64BIT.
> 
>     This patch fixes the bug by clearing only the MSI
>     enabled/mapped/initialized flags in pt_msi_disable.
> 
>     Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>     Tested-by: G.R. <firemeteor@users.sourceforge.net>
>     Xen-devel: http://marc.info/?l=xen-devel&m=135489879503075
> 
> diff --git a/hw/pt-msi.c b/hw/pt-msi.c
> index 73f737d..b03b989 100644
> --- a/hw/pt-msi.c
> +++ b/hw/pt-msi.c
> @@ -213,7 +213,7 @@ void pt_msi_disable(struct pt_dev *dev)
> 
>  out:
>      /* clear msi info */
> -    dev->msi->flags = 0;
> +    dev->msi->flags &= ~(MSI_FLAG_UNINIT | PT_MSI_MAPPED |
> PCI_MSI_FLAGS_ENABLE);
>      dev->msi->pirq = -1;
>      dev->msi_trans_en = 0;
>  }
> 
> On 2014-7-2 23:06, Ian Jackson wrote:
> >Konrad Rzeszutek Wilk writes ("[Xen-devel] [PATCH] qemu-xen-trad: free all the pirqs for msi/msix when driver unloads"):
> >>From: Zhenzhong Duan<zhenzhong.duan@oracle.com>
> >>
> >>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.
> >I have backported this to 4.4 and 4.3.  It did not apply cleanly to
> >4.2 (and I have not investgated why).
> >
> >Thanks,
> >Ian.
> 

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

[Zhenzhong Duan]

On 2014-7-4 1:54, Konrad Rzeszutek Wilk wrote:
> On Thu, Jul 03, 2014 at 11:12:18AM +0800, Zhenzhong Duan wrote:
>> There is a patch dependency missed.
> Is that one upstream as well? Or does it not need to be?
> If it is upstream, what is the title/commit id of that one?
>
> Thank you!
It's upstream, see detail below
>> commit adf74189dd58014744a4b8c9d64407d629da5e2f
>> Author: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>> Date:   Mon Dec 10 12:43:33 2012 +0000
>>
>>      qemu-xen-trad/pt_msi_disable: do not clear all MSI flags
>>
>>      "qemu-xen-trad: fix msi_translate with PV event delivery" added a
>>      pt_msi_disable() call into pt_msgctrl_reg_write, clearing the MSI flags
>>      as a consequence. MSIs get enabled again soon after by calling
>>      pt_msi_setup.
>>
>>      However the MSI flags are only setup once in the pt_msgctrl_reg_init
>>      function, so from the QEMU point of view the device has lost some
>>      important properties, like for example PCI_MSI_FLAGS_64BIT.
>>
>>      This patch fixes the bug by clearing only the MSI
>>      enabled/mapped/initialized flags in pt_msi_disable.
>>
>>      Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>>      Tested-by: G.R. <firemeteor@users.sourceforge.net>
>>      Xen-devel: http://marc.info/?l=xen-devel&m=135489879503075
>>
>> diff --git a/hw/pt-msi.c b/hw/pt-msi.c
>> index 73f737d..b03b989 100644
>> --- a/hw/pt-msi.c
>> +++ b/hw/pt-msi.c
>> @@ -213,7 +213,7 @@ void pt_msi_disable(struct pt_dev *dev)
>>
>>   out:
>>       /* clear msi info */
>> -    dev->msi->flags = 0;
>> +    dev->msi->flags &= ~(MSI_FLAG_UNINIT | PT_MSI_MAPPED |
>> PCI_MSI_FLAGS_ENABLE);
>>       dev->msi->pirq = -1;
>>       dev->msi_trans_en = 0;
>>   }
>>
>> On 2014-7-2 23:06, Ian Jackson wrote:
>>> Konrad Rzeszutek Wilk writes ("[Xen-devel] [PATCH] qemu-xen-trad: free all the pirqs for msi/msix when driver unloads"):
>>>> From: Zhenzhong Duan<zhenzhong.duan@oracle.com>
>>>>
>>>> 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.
>>> I have backported this to 4.4 and 4.3.  It did not apply cleanly to
>>> 4.2 (and I have not investgated why).
>>>
>>> Thanks,
>>> Ian.

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

[Konrad Rzeszutek Wilk]

On Fri, Jul 04, 2014 at 09:29:38AM +0800, Zhenzhong Duan wrote:
> 
> On 2014-7-4 1:54, Konrad Rzeszutek Wilk wrote:
> >On Thu, Jul 03, 2014 at 11:12:18AM +0800, Zhenzhong Duan wrote:
> >>There is a patch dependency missed.
> >Is that one upstream as well? Or does it not need to be?
> >If it is upstream, what is the title/commit id of that one?
> >
> >Thank you!
> It's upstream, see detail below


[konrad@build-external qemu-xen-dir]$ pwd
/home/konrad/xtt-x86_64/xen/tools/qemu-xen-dir
[konrad@build-external qemu-xen-dir]$ git show adf74189dd58014744a4b8c9d64407d629da5e2f
fatal: bad object adf74189dd58014744a4b8c9d64407d629da5e2f
[konrad@build-external qemu-xen-dir]$ git log --grep="clear all MSI"
[konrad@build-external qemu-xen-dir]$ 

?

> >>commit adf74189dd58014744a4b8c9d64407d629da5e2f
> >>Author: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> >>Date:   Mon Dec 10 12:43:33 2012 +0000
> >>
> >>     qemu-xen-trad/pt_msi_disable: do not clear all MSI flags
> >>
> >>     "qemu-xen-trad: fix msi_translate with PV event delivery" added a
> >>     pt_msi_disable() call into pt_msgctrl_reg_write, clearing the MSI flags
> >>     as a consequence. MSIs get enabled again soon after by calling
> >>     pt_msi_setup.
> >>
> >>     However the MSI flags are only setup once in the pt_msgctrl_reg_init
> >>     function, so from the QEMU point of view the device has lost some
> >>     important properties, like for example PCI_MSI_FLAGS_64BIT.
> >>
> >>     This patch fixes the bug by clearing only the MSI
> >>     enabled/mapped/initialized flags in pt_msi_disable.
> >>
> >>     Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> >>     Tested-by: G.R. <firemeteor@users.sourceforge.net>
> >>     Xen-devel: http://marc.info/?l=xen-devel&m=135489879503075
> >>
> >>diff --git a/hw/pt-msi.c b/hw/pt-msi.c
> >>index 73f737d..b03b989 100644
> >>--- a/hw/pt-msi.c
> >>+++ b/hw/pt-msi.c
> >>@@ -213,7 +213,7 @@ void pt_msi_disable(struct pt_dev *dev)
> >>
> >>  out:
> >>      /* clear msi info */
> >>-    dev->msi->flags = 0;
> >>+    dev->msi->flags &= ~(MSI_FLAG_UNINIT | PT_MSI_MAPPED |
> >>PCI_MSI_FLAGS_ENABLE);
> >>      dev->msi->pirq = -1;
> >>      dev->msi_trans_en = 0;
> >>  }
> >>
> >>On 2014-7-2 23:06, Ian Jackson wrote:
> >>>Konrad Rzeszutek Wilk writes ("[Xen-devel] [PATCH] qemu-xen-trad: free all the pirqs for msi/msix when driver unloads"):
> >>>>From: Zhenzhong Duan<zhenzhong.duan@oracle.com>
> >>>>
> >>>>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.
> >>>I have backported this to 4.4 and 4.3.  It did not apply cleanly to
> >>>4.2 (and I have not investgated why).
> >>>
> >>>Thanks,
> >>>Ian.
> 

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

[Zhenzhong Duan]

On 2014-7-8 4:40, Konrad Rzeszutek Wilk wrote:
> On Fri, Jul 04, 2014 at 09:29:38AM +0800, Zhenzhong Duan wrote:
>> On 2014-7-4 1:54, Konrad Rzeszutek Wilk wrote:
>>> On Thu, Jul 03, 2014 at 11:12:18AM +0800, Zhenzhong Duan wrote:
>>>> There is a patch dependency missed.
>>> Is that one upstream as well? Or does it not need to be?
>>> If it is upstream, what is the title/commit id of that one?
>>>
>>> Thank you!
>> It's upstream, see detail below
>
> [konrad@build-external qemu-xen-dir]$ pwd
> /home/konrad/xtt-x86_64/xen/tools/qemu-xen-dir
> [konrad@build-external qemu-xen-dir]$ git show adf74189dd58014744a4b8c9d64407d629da5e2f
> fatal: bad object adf74189dd58014744a4b8c9d64407d629da5e2f
> [konrad@build-external qemu-xen-dir]$ git log --grep="clear all MSI"
> [konrad@build-external qemu-xen-dir]$
>
> ?
Sorry, I mean qemu-xen-traditional upstream git

zduan
>>>> commit adf74189dd58014744a4b8c9d64407d629da5e2f
>>>> Author: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>>>> Date:   Mon Dec 10 12:43:33 2012 +0000
>>>>
>>>>      qemu-xen-trad/pt_msi_disable: do not clear all MSI flags
>>>>
>>>>      "qemu-xen-trad: fix msi_translate with PV event delivery" added a
>>>>      pt_msi_disable() call into pt_msgctrl_reg_write, clearing the MSI flags
>>>>      as a consequence. MSIs get enabled again soon after by calling
>>>>      pt_msi_setup.
>>>>
>>>>      However the MSI flags are only setup once in the pt_msgctrl_reg_init
>>>>      function, so from the QEMU point of view the device has lost some
>>>>      important properties, like for example PCI_MSI_FLAGS_64BIT.
>>>>
>>>>      This patch fixes the bug by clearing only the MSI
>>>>      enabled/mapped/initialized flags in pt_msi_disable.
>>>>
>>>>      Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>>>>      Tested-by: G.R. <firemeteor@users.sourceforge.net>
>>>>      Xen-devel: http://marc.info/?l=xen-devel&m=135489879503075
>>>>
>>>> diff --git a/hw/pt-msi.c b/hw/pt-msi.c
>>>> index 73f737d..b03b989 100644
>>>> --- a/hw/pt-msi.c
>>>> +++ b/hw/pt-msi.c
>>>> @@ -213,7 +213,7 @@ void pt_msi_disable(struct pt_dev *dev)
>>>>
>>>>   out:
>>>>       /* clear msi info */
>>>> -    dev->msi->flags = 0;
>>>> +    dev->msi->flags &= ~(MSI_FLAG_UNINIT | PT_MSI_MAPPED |
>>>> PCI_MSI_FLAGS_ENABLE);
>>>>       dev->msi->pirq = -1;
>>>>       dev->msi_trans_en = 0;
>>>>   }
>>>>
>>>> On 2014-7-2 23:06, Ian Jackson wrote:
>>>>> Konrad Rzeszutek Wilk writes ("[Xen-devel] [PATCH] qemu-xen-trad: free all the pirqs for msi/msix when driver unloads"):
>>>>>> From: Zhenzhong Duan<zhenzhong.duan@oracle.com>
>>>>>>
>>>>>> 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.
>>>>> I have backported this to 4.4 and 4.3.  It did not apply cleanly to
>>>>> 4.2 (and I have not investgated why).
>>>>>
>>>>> Thanks,
>>>>> Ian.

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

[Konrad Rzeszutek Wilk]

On Tue, Jul 08, 2014 at 09:12:39AM +0800, Zhenzhong Duan wrote:
> 
> On 2014-7-8 4:40, Konrad Rzeszutek Wilk wrote:
> >On Fri, Jul 04, 2014 at 09:29:38AM +0800, Zhenzhong Duan wrote:
> >>On 2014-7-4 1:54, Konrad Rzeszutek Wilk wrote:
> >>>On Thu, Jul 03, 2014 at 11:12:18AM +0800, Zhenzhong Duan wrote:
> >>>>There is a patch dependency missed.
> >>>Is that one upstream as well? Or does it not need to be?
> >>>If it is upstream, what is the title/commit id of that one?
> >>>
> >>>Thank you!
> >>It's upstream, see detail below
> >
> >[konrad@build-external qemu-xen-dir]$ pwd
> >/home/konrad/xtt-x86_64/xen/tools/qemu-xen-dir
> >[konrad@build-external qemu-xen-dir]$ git show adf74189dd58014744a4b8c9d64407d629da5e2f
> >fatal: bad object adf74189dd58014744a4b8c9d64407d629da5e2f
> >[konrad@build-external qemu-xen-dir]$ git log --grep="clear all MSI"
> >[konrad@build-external qemu-xen-dir]$
> >
> >?
> Sorry, I mean qemu-xen-traditional upstream git

Ok, is that patch upstream? Should it be upstream? If so, had it been
posted in the past?

Thank you!
> 
> zduan
> >>>>commit adf74189dd58014744a4b8c9d64407d629da5e2f
> >>>>Author: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> >>>>Date:   Mon Dec 10 12:43:33 2012 +0000
> >>>>
> >>>>     qemu-xen-trad/pt_msi_disable: do not clear all MSI flags
> >>>>
> >>>>     "qemu-xen-trad: fix msi_translate with PV event delivery" added a
> >>>>     pt_msi_disable() call into pt_msgctrl_reg_write, clearing the MSI flags
> >>>>     as a consequence. MSIs get enabled again soon after by calling
> >>>>     pt_msi_setup.
> >>>>
> >>>>     However the MSI flags are only setup once in the pt_msgctrl_reg_init
> >>>>     function, so from the QEMU point of view the device has lost some
> >>>>     important properties, like for example PCI_MSI_FLAGS_64BIT.
> >>>>
> >>>>     This patch fixes the bug by clearing only the MSI
> >>>>     enabled/mapped/initialized flags in pt_msi_disable.
> >>>>
> >>>>     Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> >>>>     Tested-by: G.R. <firemeteor@users.sourceforge.net>
> >>>>     Xen-devel: http://marc.info/?l=xen-devel&m=135489879503075
> >>>>
> >>>>diff --git a/hw/pt-msi.c b/hw/pt-msi.c
> >>>>index 73f737d..b03b989 100644
> >>>>--- a/hw/pt-msi.c
> >>>>+++ b/hw/pt-msi.c
> >>>>@@ -213,7 +213,7 @@ void pt_msi_disable(struct pt_dev *dev)
> >>>>
> >>>>  out:
> >>>>      /* clear msi info */
> >>>>-    dev->msi->flags = 0;
> >>>>+    dev->msi->flags &= ~(MSI_FLAG_UNINIT | PT_MSI_MAPPED |
> >>>>PCI_MSI_FLAGS_ENABLE);
> >>>>      dev->msi->pirq = -1;
> >>>>      dev->msi_trans_en = 0;
> >>>>  }
> >>>>
> >>>>On 2014-7-2 23:06, Ian Jackson wrote:
> >>>>>Konrad Rzeszutek Wilk writes ("[Xen-devel] [PATCH] qemu-xen-trad: free all the pirqs for msi/msix when driver unloads"):
> >>>>>>From: Zhenzhong Duan<zhenzhong.duan@oracle.com>
> >>>>>>
> >>>>>>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.
> >>>>>I have backported this to 4.4 and 4.3.  It did not apply cleanly to
> >>>>>4.2 (and I have not investgated why).
> >>>>>
> >>>>>Thanks,
> >>>>>Ian.
> 

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

[Zhenzhong Duan]

On 2014-8-4 22:01, Konrad Rzeszutek Wilk wrote:
> On Tue, Jul 08, 2014 at 09:12:39AM +0800, Zhenzhong Duan wrote:
>> On 2014-7-8 4:40, Konrad Rzeszutek Wilk wrote:
>>> On Fri, Jul 04, 2014 at 09:29:38AM +0800, Zhenzhong Duan wrote:
>>>> On 2014-7-4 1:54, Konrad Rzeszutek Wilk wrote:
>>>>> On Thu, Jul 03, 2014 at 11:12:18AM +0800, Zhenzhong Duan wrote:
>>>>>> There is a patch dependency missed.
>>>>> Is that one upstream as well? Or does it not need to be?
>>>>> If it is upstream, what is the title/commit id of that one?
>>>>>
>>>>> Thank you!
>>>> It's upstream, see detail below
>>> [konrad@build-external qemu-xen-dir]$ pwd
>>> /home/konrad/xtt-x86_64/xen/tools/qemu-xen-dir
>>> [konrad@build-external qemu-xen-dir]$ git show adf74189dd58014744a4b8c9d64407d629da5e2f
>>> fatal: bad object adf74189dd58014744a4b8c9d64407d629da5e2f
>>> [konrad@build-external qemu-xen-dir]$ git log --grep="clear all MSI"
>>> [konrad@build-external qemu-xen-dir]$
>>>
>>> ?
>> Sorry, I mean qemu-xen-traditional upstream git
> Ok, is that patch upstream? Should it be upstream? If so, had it been
> posted in the past?
It's already in qemu-xen upstream, see below

void xen_pt_msi_disable(XenPCIPassthroughState *s)
{
.......
     /* clear msi info */
     msi->flags &= ~PCI_MSI_FLAGS_ENABLE;
     msi->initialized = false;
     msi->mapped = false;
     msi->pirq = XEN_PT_UNASSIGNED_PIRQ;
}

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

[Konrad Rzeszutek Wilk]

On Tue, Aug 05, 2014 at 03:31:34PM +0800, Zhenzhong Duan wrote:
> 
> On 2014-8-4 22:01, Konrad Rzeszutek Wilk wrote:
> >On Tue, Jul 08, 2014 at 09:12:39AM +0800, Zhenzhong Duan wrote:
> >>On 2014-7-8 4:40, Konrad Rzeszutek Wilk wrote:
> >>>On Fri, Jul 04, 2014 at 09:29:38AM +0800, Zhenzhong Duan wrote:
> >>>>On 2014-7-4 1:54, Konrad Rzeszutek Wilk wrote:
> >>>>>On Thu, Jul 03, 2014 at 11:12:18AM +0800, Zhenzhong Duan wrote:
> >>>>>>There is a patch dependency missed.
> >>>>>Is that one upstream as well? Or does it not need to be?
> >>>>>If it is upstream, what is the title/commit id of that one?
> >>>>>
> >>>>>Thank you!
> >>>>It's upstream, see detail below
> >>>[konrad@build-external qemu-xen-dir]$ pwd
> >>>/home/konrad/xtt-x86_64/xen/tools/qemu-xen-dir
> >>>[konrad@build-external qemu-xen-dir]$ git show adf74189dd58014744a4b8c9d64407d629da5e2f
> >>>fatal: bad object adf74189dd58014744a4b8c9d64407d629da5e2f
> >>>[konrad@build-external qemu-xen-dir]$ git log --grep="clear all MSI"
> >>>[konrad@build-external qemu-xen-dir]$
> >>>
> >>>?
> >>Sorry, I mean qemu-xen-traditional upstream git
> >Ok, is that patch upstream? Should it be upstream? If so, had it been
> >posted in the past?
> It's already in qemu-xen upstream, see below
> 
> void xen_pt_msi_disable(XenPCIPassthroughState *s)
> {
> .......
>     /* clear msi info */
>     msi->flags &= ~PCI_MSI_FLAGS_ENABLE;
>     msi->initialized = false;
>     msi->mapped = false;
>     msi->pirq = XEN_PT_UNASSIGNED_PIRQ;
> }

OK, and that is part of the giant '3854ca57':

commit 3854ca577dad92c4fe97b4a6ebce360e25407af7
Author: Jiang Yunhong <yunhong.jiang@intel.com>
Date:   Thu Jun 21 15:42:35 2012 +0000

    Introduce Xen PCI Passthrough, MSI
    
    A more complete history can be found here:
    git://xenbits.xensource.com/qemu-xen-unstable.git
    
    Signed-off-by: Jiang Yunhong <yunhong.jiang@intel.com>
    Signed-off-by: Shan Haitao <haitao.shan@intel.com>
    Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
    Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>

And the git tree it points to has tons of tags but nothing
really saying 'pci'. <sigh>

Seems like the only option is to spin out a tiny patch
that has the fix and document where it was (aka point to the
giant commit). 

Duan, I can do it in two-three weeks time-frame. If you want
to take a stab at it before then, don't hesitate!

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

[Zhenzhong Duan]

Hi Ian,

Could you cherry-pick commit adf74189dd58014744a4b8c9d64407d629da5e2f  
before backport my patch?
It seems 4.3 and 4.4 branch already has commit adf74189dd but 4.2 not.

thanks
zduan

On 2014-7-3 11:12, Zhenzhong Duan wrote:
> There is a patch dependency missed.
>
> commit adf74189dd58014744a4b8c9d64407d629da5e2f
> Author: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Date:   Mon Dec 10 12:43:33 2012 +0000
>
>     qemu-xen-trad/pt_msi_disable: do not clear all MSI flags
>
>     "qemu-xen-trad: fix msi_translate with PV event delivery" added a
>     pt_msi_disable() call into pt_msgctrl_reg_write, clearing the MSI 
> flags
>     as a consequence. MSIs get enabled again soon after by calling
>     pt_msi_setup.
>
>     However the MSI flags are only setup once in the pt_msgctrl_reg_init
>     function, so from the QEMU point of view the device has lost some
>     important properties, like for example PCI_MSI_FLAGS_64BIT.
>
>     This patch fixes the bug by clearing only the MSI
>     enabled/mapped/initialized flags in pt_msi_disable.
>
>     Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>     Tested-by: G.R. <firemeteor@users.sourceforge.net>
>     Xen-devel: http://marc.info/?l=xen-devel&m=135489879503075
>
> diff --git a/hw/pt-msi.c b/hw/pt-msi.c
> index 73f737d..b03b989 100644
> --- a/hw/pt-msi.c
> +++ b/hw/pt-msi.c
> @@ -213,7 +213,7 @@ void pt_msi_disable(struct pt_dev *dev)
>
>  out:
>      /* clear msi info */
> -    dev->msi->flags = 0;
> +    dev->msi->flags &= ~(MSI_FLAG_UNINIT | PT_MSI_MAPPED | 
> PCI_MSI_FLAGS_ENABLE);
>      dev->msi->pirq = -1;
>      dev->msi_trans_en = 0;
>  }
>
> On 2014-7-2 23:06, Ian Jackson wrote:
>> Konrad Rzeszutek Wilk writes ("[Xen-devel] [PATCH] qemu-xen-trad: 
>> free all the pirqs for msi/msix when driver unloads"):
>>> From: Zhenzhong Duan<zhenzhong.duan@oracle.com>
>>>
>>> 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.
>> I have backported this to 4.4 and 4.3.  It did not apply cleanly to
>> 4.2 (and I have not investgated why).
>>
>> Thanks,
>> Ian.
>

Re: [PATCH] Fix qemu traditional with PCI passthrough.

[Ian Jackson]

Konrad Rzeszutek Wilk writes ("[PATCH] Fix qemu traditional with PCI passthrough."):
> Hey Ian and Stefano,
> 
> This patch was posted way back last year in July 
> (see http://lists.xen.org/archives/html/xen-devel/2013-07/msg00004.html) and
> was mentioned to:
> 
> ">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."
> 
> I think this patch just got lost in the Xen 4.4 release. Dusting it
> off and reposting.

Quite likely.

I'll wait for Stefano to comment on the qemu-xen patch but I don't see
a problem with applying this to staging, and it should probably go on
my backport list.

Thanks,
Ian.