[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
RE: [PATCH v4 3/5] vfio/pci: Disable INTx in vfio_realize error path
From: |
Duan, Zhenzhong |
Subject: |
RE: [PATCH v4 3/5] vfio/pci: Disable INTx in vfio_realize error path |
Date: |
Fri, 30 Jun 2023 01:19:48 +0000 |
>-----Original Message-----
>From: Joao Martins <joao.m.martins@oracle.com>
>Subject: Re: [PATCH v4 3/5] vfio/pci: Disable INTx in vfio_realize error path
>
>
>
>On 29/06/2023 16:13, Cédric Le Goater wrote:
>> On 6/29/23 13:24, Joao Martins wrote:
>>> On 29/06/2023 09:40, Zhenzhong Duan wrote:
>>>> When vfio realize fails, INTx isn't disabled if it has been enabled.
>>>> This may confuse host side with unhandled interrupt report.
>>>>
>>>> Add a new label to be used for vfio_intx_enable() failed case.
>>>>
>>>> Fixes: a9994687cb9b ("vfio/display: core & wireup")
>>>> Fixes: b290659fc3dd ("hw/vfio/display: add ramfb support")
>>>> Fixes: c62a0c7ce34e ("vfio/display: add xres + yres properties")
>>>
>>> Sounds to me the correct Fixes tag is the same as first patch i.e.:
>>>
>>> Fixes: c5478fea27ac ("vfio/pci: Respond to KVM irqchip change
>>> notifier")
OK, will use it.
Previously I thought I should pick commit a9994687cb9b which firstly
introduced the timer leak with a jump label out_teardown, then
b290659fc3dd and c62a0c7ce34e which used out_teardown.
>>>
>>>> Signed-off-by: Zhenzhong Duan <zhenzhong.duan@intel.com>
>>>
>>> Looks good, but see some clarifications below.
>>>
>>>> ---
>>>> hw/vfio/pci.c | 4 +++-
>>>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/vfio/pci.c b/hw/vfio/pci.c index
>>>> ab6645ba60af..54a8179d1c64 100644
>>>> --- a/hw/vfio/pci.c
>>>> +++ b/hw/vfio/pci.c
>>>> @@ -3167,7 +3167,7 @@ static void vfio_realize(PCIDevice *pdev,
>>>> Error **errp)
>>>>
>>>> kvm_irqchip_add_change_notifier(&vdev->irqchip_change_notifier);
>>>> ret = vfio_intx_enable(vdev, errp);
>>>> if (ret) {
>>>> - goto out_deregister;
>>>> + goto out_intx_disable;
>>>> }
>>>> }
>>>> @@ -3220,6 +3220,8 @@ static void vfio_realize(PCIDevice *pdev,
>>>> Error **errp)
>>>> return;
>>>> out_deregister:
>>>> + vfio_disable_interrupts(vdev);
>>>
>>> You are calling vfio_disable_interrupts() when what you want is
>>> vfio_intx_disable() ? But I guess your thinking was to call
>>> vfio_disable_interrupt() which eventually calls vfio_intx_disable()
>>> in case INTx was really setup, thus saving the duplicated check. The
>>> MSIx/MSI in realize() I don't think they will be enabled at this point.
Yes.
>>> Let me know if I misunderstood.
>>>
>>>> +out_intx_disable:
>>>
>>> Maybe 'out_intx_teardown' or 'out_intx_deregister' because you are
>>> not really disabling INTx.
>>
>> or simply extract from vfio_disable_interrupts() :
>>
>> if (vdev->interrupt == VFIO_INT_INTx) {
>> vfio_intx_disable(vdev);
>> }
>>
>> and add the above code before cleaning up the intx routing notifier
>> without any new goto labels.
>>
>An even better option indeed.
Will do.
Thanks
Zhenzhong
- [PATCH v4 1/5] vfio/pci: Fix a segfault in vfio_realize, (continued)
[PATCH v4 4/5] vfio/pci: Free resources when vfio_migration_realize fails, Zhenzhong Duan, 2023/06/29
[PATCH v4 5/5] vfio/migration: Refactor and fix print of "Migration disabled", Zhenzhong Duan, 2023/06/29