Re: [Xen-devel] [PATCH] xen: reuse the same pirq allocated when driver load first time

[Zhenzhong Duan <zhenzhong.duan@oracle.com>]

On 2013-06-27 02:08, Stefano Stabellini wrote:
> On Wed, 26 Jun 2013, Zhenzhong Duan wrote:
>> On 2013-06-26 01:51, Stefano Stabellini wrote:
>>> On Tue, 25 Jun 2013, DuanZhenzhong wrote:
>>>> Stefano Stabellini wrote:
>>>>> Trimming some of the people in CC
>>>>>
>>>>> On Mon, 24 Jun 2013, Zhenzhong Duan wrote:
>>>>>     
>>>>>> On 2013-06-20 22:21, Stefano Stabellini wrote:
>>>>>>       
>>>>>>> On Thu, 20 Jun 2013, Zhenzhong Duan wrote:
>>>>>>>         
>>>>>>>> On 2013-06-05 20:50, Stefano Stabellini wrote:
>>>>>>>>           
>>>>>>>>> On Wed, 5 Jun 2013, Zhenzhong Duan wrote:
>>>>>>>>>             
>>>>>>>>>> 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]
>>>>>>>>>>               
>>>>>>>>> Can you add some printks in Xen (the hypercall name is
>>>>>>>>> PHYSDEVOP_unmap_pirq) to figure out exactly why they are
>>>>>>>>> failing?
>>>>>>>>>             
>>>>>>>> Did some test, domain_pirq_to_emuirq(d, unmap->pirq) = IRQ_UNBOUND
>>>>>>>> in
>>>>>>>> physdev_unmap_pirq.
>>>>>>>>           
>>>>>>> That means that Linux didn't call irq_enable on the MSI-X in
>>>>>>> question:
>>>>>>>
>>>>>>> irq_enable -> __startup_pirq -> EVTCHNOP_bind_pirq
>>>>>>>
>>>>>>> EVTCHNOP_bind_pirq is implemented by evtchn_bind_pirq in Xen and
>>>>>>> calls
>>>>>>> map_domain_emuirq_pirq, so domain_pirq_to_emuirq(d, unmap->pirq)
>>>>>>> should
>>>>>>> be IRQ_PT.
>>>>>>>
>>>>>>> I don't know if that's a normal condition, but in any case it should
>>>>>>> not create any problems to physdev_unmap_pirq, in fact the folloing
>>>>>>> check:
>>>>>>>
>>>>>>>        if ( domid == DOMID_SELF || ret )
>>>>>>>                goto free_domain;
>>>>>>>
>>>>>>> should fail so Xen should continue and execute unmap_domain_pirq.
>>>>>>> That's
>>>>>>> what we want.
>>>>>>>         
>>>>>>   From linux side, request_irq->  request_threaded_irq-> __setup_irq->
>>>>>> irq_startup->  startup_pirq-> EVTCHNOP_bind_pirq
>>>>>> If irq_enable isn't called, how does the driver receive interrupt, I
>>>>>> did
>>>>>> see
>>>>>> four interrupts in /proc/interrupt and driver works ok.
>>>>>>       
>>>>> Good to know
>>>>>
>>>>>     
>>>>>> Could you have a look if there is something wrong in xen side of
>>>>>> clearing
>>>>>> the
>>>>>> mapping?
>>>>>>       
>>>>> What I am saying is that the error you are getting:
>>>>>
>>>>> 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]
>>>>>
>>>>> cannot be caused by domain_pirq_to_emuirq(d, pirq) returning
>>>>> IRQ_UNBOUND.
>>>>> So, why are you getting this error? What is failing?
>>>>> I am ready to believe the problem is in Xen but Without understanding
>>>>> why you are getting the error it's hard to find a solution.
>>>>>     
>>>> I found the reason, you are looking at xen-unstable, I was working with
>>>> 4.1.30-OVM, it has patch of CVE-2012-4536 / XSA-21.
>>>> That patch set ret to -EINVAL initially. After remove that line, unmap
>>>> succeed.
>>>> But we still need below patch to let driver reload succeed everytime.
>>>> Without
>>>> that, 1st reload failed, 2nd succeed, 3 failed, ...
>>>>
>>>> diff -up --new-file ./hw/pt-msi.c.old1 ./hw/pt-msi.c
>>>> --- ./hw/pt-msi.c.old1  2013-06-26 01:36:08.000000000 +0800
>>>> +++ ./hw/pt-msi.c       2013-06-26 01:37:41.000000000 +0800
>>>> @@ -469,7 +469,7 @@ static void pci_msix_writel(void *opaque
>>>>           return;
>>>>       }
>>>>
>>>> -    if ( offset != 3 && entry->io_mem[offset] != val )
>>>> +    if ( offset != 3 && (entry->io_mem[offset] != val || entry->pirq ==
>>>> -1))
>>>>           entry->flags = 1;
>>>>       entry->io_mem[offset] = val;
>>> Interesting. I don't think this is the proper fix though.
>>> Does the appended patch change anything?
>>>
>>> diff --git a/hw/pt-msi.c b/hw/pt-msi.c
>>> index 71fa6f0..cd5d9c7 100644
>>> --- a/hw/pt-msi.c
>>> +++ b/hw/pt-msi.c
>>> @@ -302,7 +302,7 @@ static int pt_msix_update_one(struct pt_dev *dev, int
>>> entry_nr)
>>>        uint32_t gflags = __get_msi_gflags(entry->io_mem[2], gaddr);
>>>        int ret;
>>>    -    if ( !entry->flags )
>>> +    if ( !entry->flags && ptdev->msix->enabled )
>>>            return 0;
>>>          if (!gvec) {
>> Tested, not work.
>> If you look at msix_capability_init in kernel, line 707,722,
>> dev->msix->enabled is already set when pt_msix_update is called.
> Yeah, but it shouldn't be already set in QEMU. In fact in QEMU
> dev->msix->enabled is modified in pt_msixctrl_reg_write after calling to
> pt_msix_update.
It does.
line 707,  PCI_MSIX_FLAGS_MASKALL | PCI_MSIX_FLAGS_ENABLE is set
                  this will set dev->msix->enabled first time
line 722, PCI_MSIX_FLAGS_ENABLE set
                  this trigger call of pt_msix_update
                  then dev->msix->enabled was set a second time
> I was assuming that you needed to add "|| entry->pirq == -1" because you
> needed to pass the check:
>
>      if ( !entry->flags )
>          return 0;
>
> at the beginning of pt_msix_update_one. Am I getting it right?
Right, as entry->pirq is set to -1 when driver unload.
> If that is case that I thought that we just needed to make sure that
> when ptdev->msix->enabled is still zero then we go through the test in
> pt_msix_update_one. Where is the mistake?
If you want to use dev->msix->enabled for checking, below patch could work.
But in this case, dev->msix->enabled doesn't represent PCI_MSIX_ENABLE 
any more,
but a combination of PCI_MSIX_ENABLE and ~PCI_MSIX_MASK

zduan
***********************************************************
diff -up ./hw/pass-through.c.old2 ./hw/pass-through.c
--- ./hw/pass-through.c.old2    2013-06-27 11:05:30.000000000 +0800
+++ ./hw/pass-through.c 2013-06-27 11:07:35.000000000 +0800
@@ -4027,7 +4027,7 @@ static int pt_msixctrl_reg_write(struct
          pt_msix_disable(ptdev);
      }

-    ptdev->msix->enabled = !!(*value & PCI_MSIX_ENABLE);
+    ptdev->msix->enabled = (*value & PCI_MSIX_ENABLE) && !(*value & 
PCI_MSIX_MASK);

      return 0;
  }
diff -up ./hw/pt-msi.c.old2 ./hw/pt-msi.c
--- ./hw/pt-msi.c.old2  2013-06-27 11:26:12.000000000 +0800
+++ ./hw/pt-msi.c       2013-06-27 11:27:13.000000000 +0800
@@ -294,7 +294,7 @@ static int pt_msix_update_one(struct pt_
      uint32_t gflags = __get_msi_gflags(entry->io_mem[2], gaddr);
      int ret;

-    if ( !entry->flags )
+    if ( !entry->flags && dev->msix->enabled )
          return 0;

      if (!gvec) {