lkml.org 
[lkml]   [2019]   [Aug]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v3 3/3] vfio_pci: make use of update_irq_devid and optimize irq ops
From
Date


> On Aug 20, 2019, at 11:24 PM, luoben <luoben@linux.alibaba.com> wrote:
>
>
>
> 在 2019/8/16 上午12:45, Thomas Gleixner 写道:
>> On Thu, 15 Aug 2019, Ben Luo wrote:
>>
>>> if (vdev->ctx[vector].trigger) {
>>> - free_irq(irq, vdev->ctx[vector].trigger);
>>> - irq_bypass_unregister_producer(&vdev->ctx[vector].producer);
>>> - kfree(vdev->ctx[vector].name);
>>> - eventfd_ctx_put(vdev->ctx[vector].trigger);
>>> - vdev->ctx[vector].trigger = NULL;
>>> + if (vdev->ctx[vector].trigger == trigger) {
>>> + irq_bypass_unregister_producer(&vdev->ctx[vector].producer);
>>>
>> What's the point of unregistering the producer, just to register it again below?
> Whether producer token changed or not, irq_bypass_register_producer() is a way (seems the
>
> only way) to update IRTE by VFIO for posted interrupt.
>
> When posted interrupt is in use, the target IRTE will be used by IOMMU directly to find the
>
> target CPU for an interrupt posted to VM, since hypervisor is bypassed.
>
> irq_bypass_register_producer() will modify IRTE based on the information retrieved from KVM,
>
>
> 0xffffffff8150a920 : modify_irte+0x0/0x180 [kernel]
> 0xffffffff8150ab94 : intel_ir_set_vcpu_affinity+0xf4/0x150 [kernel]
> 0xffffffff81125f3c : irq_set_vcpu_affinity+0x5c/0xa0 [kernel]
> 0xffffffffa0550818 : vmx_update_pi_irte+0x178/0x290 [kvm_intel] // get pi_desc etc. from KVM
> 0xffffffffa052b789 : kvm_arch_irq_bypass_add_producer+0x39/0x50 [kvm_intel] (inexact)
> 0xffffffffa024a50b : __connect+0x7b/0xa0 [kvm] (inexact)
> 0xffffffffa024a6a8 : irq_bypass_register_producer+0x108/0x140 [kvm] (inexact)
> 0xffffffffa0338386 : vfio_msi_set_vector_signal+0x1b6/0x2c0 [vfio_pci] (inexact)
>>
>>> + } else if (trigger) {
>>> + ret = update_irq_devid(irq,
>>> + vdev->ctx[vector].trigger, trigger);
>>> + if (unlikely(ret)) {
>>> + dev_info(&pdev->dev,
>>> + "update_irq_devid %d (token %p) fails: %d\n",
>>>
>> s/fails/failed/
>>
>>
>>> + irq, vdev->ctx[vector].trigger, ret);
>>> + eventfd_ctx_put(trigger);
>>> + return ret;
>>> + }
>>>
>> This lacks any form of comment why this is correct. dev_id is updated and
>> the producer with the old token is still registered.
>>
> ok, I will add comment like below:
>
> For KVM-VFIO case, once producer and consumer connected successfully, interrupt from passthrough
>
> device will be directly delivered to VM instead of triggering interrupt process in HOST. If producer and
>
> consumer are disconnected, this interrupt process will fall back to remap mode, the handler vfio_msihandler()
>
> registered in corresponding irqaction will use the dev_id to find the right way to deliver the interrupt to VM.
>
> So, it is safe to update dev_id before unregistration of irq-bypass producer, i.e. switch back from posted
>
> mode to remap mode, since only in remap mode the 'dev_id' will be used by interrupt handler. To producer
>
> and consumer, dev_id is only a token for pairing them togather.
>>
>>> + irq_bypass_unregister_producer(&vdev->ctx[vector].producer);
>>>
>> Now it's unregistered.
>>
>>
>>> + eventfd_ctx_put(vdev->ctx[vector].trigger);
>>> + vdev->ctx[vector].producer.token = trigger;
>>>
>> The token is updated and then it's newly registered below.
>>
>>
>>> + vdev->ctx[vector].trigger = trigger;
>>> - vdev->ctx[vector].producer.token = trigger;
>>> - vdev->ctx[vector].producer.irq = irq;
>>> + /* always update irte for posted mode */
>>> ret = irq_bypass_register_producer(&vdev->ctx[vector].producer);
>>> if (unlikely(ret))
>>> dev_info(&pdev->dev,
>>> "irq bypass producer (token %p) registration fails: %d\n",
>>> vdev->ctx[vector].producer.token, ret);
>>>
>> I know this code already existed, but again this looks inconsistent. If the
>> registration fails then a subsequent update will try to unregister a not
>> registered producer. Does not make any sense to me.
>>
> irq_bypass_register_producer() also fails on duplicated registration, so maybe an unconditional try to
>
> unregistration is a easy way to keep consistent.
>
> Maybe it's better to change these function names to irq_bypass_try_register_producer() and
>
> irq_bypass_try_unregister_producer() :)
>
>
Hi Ben,
The point is that we shouldn’t blindly try to register again if we fails to unregister a posted interrupt producer. By this way, the fast path (posted interrupt) may get disabled, but it’s safer than blindly ignoring the failure of ungistration.
Thanks,
Gerry
>
> Thanks,
>
> Ben

\
 
 \ /
  Last update: 2019-08-20 17:28    [W:0.081 / U:2.256 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site