Re: [Qemu-devel] [PATCH v3 12/13] q35: ioapic: add support for split irqchip and irqfd

[Jan Kiszka <jan.kiszka@web.de>]

On 2016-04-18 10:55, Peter Xu wrote:
> On Sun, Apr 17, 2016 at 12:45:03PM +0300, Michael S. Tsirkin wrote:
>> On Sat, Apr 16, 2016 at 07:44:12PM -0700, Jan Kiszka wrote:
>>> On 2016-04-14 20:31, Peter Xu wrote:
> [...]
>>>> diff --git a/hw/intc/ioapic.c b/hw/intc/ioapic.c
>>>> index 84e8948..b993bd0 100644
>>>> --- a/hw/intc/ioapic.c
>>>> +++ b/hw/intc/ioapic.c
>>>> @@ -182,34 +182,23 @@ static void ioapic_update_kvm_routes(IOAPICCommonState *s)
>>>>  {
>>>>  #ifdef CONFIG_KVM
>>>>      int i;
>>>> +    int ret;
>>>>  
>>>>      if (kvm_irqchip_is_split()) {
>>>>          for (i = 0; i < IOAPIC_NUM_PINS; i++) {
>>>> -            uint64_t entry = s->ioredtbl[i];
>>>> -            uint8_t trig_mode;
>>>> -            uint8_t delivery_mode;
>>>> -            uint8_t dest;
>>>> -            uint8_t dest_mode;
>>>> -            uint64_t pin_polarity;
>>>> -            MSIMessage msg;
>>>> -
>>>> -            trig_mode = ((entry >> IOAPIC_LVT_TRIGGER_MODE_SHIFT) & 1);
>>>> -            dest = entry >> IOAPIC_LVT_DEST_SHIFT;
>>>> -            dest_mode = (entry >> IOAPIC_LVT_DEST_MODE_SHIFT) & 1;
>>>> -            pin_polarity = (entry >> IOAPIC_LVT_TRIGGER_MODE_SHIFT) & 1;
>>>> -            delivery_mode =
>>>> -                (entry >> IOAPIC_LVT_DELIV_MODE_SHIFT) & IOAPIC_DM_MASK;
>>>> -
>>>> -            msg.address = APIC_DEFAULT_ADDRESS;
>>>> -            msg.address |= dest_mode << 2;
>>>> -            msg.address |= dest << 12;
>>>> -
>>>> -            msg.data = entry & IOAPIC_VECTOR_MASK;
>>>> -            msg.data |= delivery_mode << APIC_DELIVERY_MODE_SHIFT;
>>>> -            msg.data |= pin_polarity << APIC_POLARITY_SHIFT;
>>>> -            msg.data |= trig_mode << APIC_TRIG_MODE_SHIFT;
>>>> -
>>>> -            kvm_irqchip_update_msi_route(kvm_state, i, msg, NULL);
>>>> +            MSIMessage src, dst;
>>>> +            struct ioapic_entry_info info;
>>>> +            ioapic_entry_parse(s->ioredtbl[i], &info);
>>>> +            src.address = info.addr;
>>>> +            src.data = info.data;
>>>> +            /* We update kernel irqchip routes with translated
>>>> +             * results. */
>>>> +            ret = s->int_remap(s->iommu, &src, &dst);
>>>> +            if (ret) {
>>>> +                DPRINTF("Int remap failed: %d, drop interrupt\n", ret);
>>>> +                continue;
>>>> +            }
>>>> +            kvm_irqchip_update_msi_route(kvm_state, i, dst, NULL);
>>>
>>> The need to hook here makes me wonder if we can't inject IOAPIC
>>> interrupts via KVM_SIGNAL_MSI (abstracted by kvm_irqchip_send_msi, but
>>> that will pick the fast-path on kernels supporting split irqchip)
>>> instead of open-coding the route changes. If we translated the IOAPIC
>>> outputs always into MSIs, the need for special-casing split irqchip
>>> would be gone, and the need for hooking here for IR just as well.
> 
> Hi, Jan,
> 
> IIUC, this can be achieved by removing lines in ioapic_service():
> 
> -#ifdef CONFIG_KVM
> -                if (kvm_irqchip_is_split()) {
> -                    if (trig_mode == IOAPIC_TRIGGER_EDGE) {
> -                        kvm_set_irq(kvm_state, i, 1);
> -                        kvm_set_irq(kvm_state, i, 0);
> -                    } else {
> -                        if (!coalesce) {
> -                            kvm_set_irq(kvm_state, i, 1);
> -                        }
> -                    }
> -                    continue;
> -                }
> -#else
> -                (void)coalesce;
> -#endif
> 
> So that QEMU will automatically select the correct way to notify the
> interrupt depending on whether "apic" or "kvm-apic" is used.
> 
> I suppose this is a good way to do if we are to support split
> irqchip only. However, what if we move on to support irqfd and
> vhost? If so, we may still need to update kernel entries into
> translated ones, right? Or... did I miss anything?

Right, in-kernel irq sources depend on an already remapped channel
because they deliver directly to the in-kernel APICs. Thus, you will
have to establish routes for those irqfds that reflects th (cached) IRTEs.

Jan