Re: [Qemu-devel] [PATCH] ivshmem: use irqfd to interrupt among VMs

[liu ping fan]

On Thu, Nov 22, 2012 at 7:40 PM, Jan Kiszka <address@hidden> wrote:
> On 2012-11-22 03:48, liu ping fan wrote:
>> On Wed, Nov 21, 2012 at 8:43 PM, Jan Kiszka <address@hidden> wrote:
>>> On 2012-11-21 07:02, Liu Ping Fan wrote:
[...]
>>>>
>>>> +static int ivshmem_vector_use(PCIDevice *dev, unsigned vector,
>>>> +                                     MSIMessage msg)
>>>> +{
>>>> +    IVShmemState *s = DO_UPCAST(IVShmemState, dev, dev);
>>>> +    int virq;
>>>> +    EventNotifier *n = &s->peers[s->vm_id].eventfds[vector];
>>>> +
>>>> +    virq = kvm_irqchip_add_msi_route(kvm_state, msg);
>>>> +    if (virq >= 0 && kvm_irqchip_add_irqfd_notifier(kvm_state, n, virq) 
>>>> >= 0) {
>>>> +        s->vector_irqfd[vector].virq = virq;
>>>> +        s->vector_irqfd[vector].used = true;
>>>> +        qemu_chr_add_handlers(s->eventfd_chr[vector], NULL, NULL, NULL, 
>>>> NULL);
>>>> +    } else if (virq >= 0) {
>>>> +        kvm_irqchip_release_virq(kvm_state, virq);
>>>> +    }
>>>> +    return 0;
>>>
>>> You drop the errors here. Better refactor the code to a scheme like this:
>>>
>> In msix_fire_vector_notifier(), there is "assert(ret >= 0);".  But I
>> think ivshmem can work even if irqfd can not be established, and fall
>> back to the origin scheme. So here just ignore err. Is it proper?
>
> If you have an error here, something seriously went wrong. So better let
> the user know than falling back to "slow" mode silently. That's what
> other users do as well.
>
Refer to vfio_msix_vector_use(),  I think to avoid silently, we can
error_report() to notify user.  And because as msi_route is limited
resource, so we should hold with fail, and keep the Qemu still in
work.
Is it OK?
>>
>>> err = service();
>>> if (err) {
>>>     roll_back();
>>>     return err;
>>>     /* or: goto roll_back_... */
>>> }
>>>
>>>> +}
>>>> +
[...]
>>
>> And I think, this is the way to push interrupt subsystem out of big
>> lock for KVM.  For TCG code, we can use something like
>> kvm_irqchip_add_msi_route(). How do you think about it?
>
> Cannot follow what your idea is.
>
I mean to add RCU style interrupt dispatching table for TCG.

> Also not that MSI on x86 is special anyway as it is not routed in any
> user-space device model (so far - emulated interrupt remapping will
> change this) but goes directly to the target VCPU via the corresponding
> IOCTL.
>
Oh, yes, MSI is so special. The user space dispatch table can only
shortcut IRQ to something like "ioapic",   not directly to vcpu

Regards,
Pingfan
> Jan
>
> --
> Siemens AG, Corporate Technology, CT RTC ITP SDP-DE
> Corporate Competence Center Embedded Linux