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
