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

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

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.

zduan