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

[DuanZhenzhong <zhenzhong.duan@oracle.com>]

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;

zduan