From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gleb Natapov Subject: Re: [PATCH v6 6/6] KVM: Use eoi to track RTC interrupt delivery status Date: Fri, 22 Mar 2013 10:44:35 +0200 Message-ID: <20130322084435.GR9382@redhat.com> References: <1363929845-29005-1-git-send-email-yang.z.zhang@intel.com> <1363929845-29005-7-git-send-email-yang.z.zhang@intel.com> <20130322075047.GN9382@redhat.com> <20130322081441.GP9382@redhat.com> <20130322083045.GQ9382@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: "kvm@vger.kernel.org" , "mtosatti@redhat.com" , "Zhang, Xiantao" To: "Zhang, Yang Z" Return-path: Received: from mx1.redhat.com ([209.132.183.28]:18589 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754124Ab3CVIor (ORCPT ); Fri, 22 Mar 2013 04:44:47 -0400 Content-Disposition: inline In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: On Fri, Mar 22, 2013 at 08:37:22AM +0000, Zhang, Yang Z wrote: > Gleb Natapov wrote on 2013-03-22: > > On Fri, Mar 22, 2013 at 08:25:21AM +0000, Zhang, Yang Z wrote: > >> Gleb Natapov wrote on 2013-03-22: > >>> On Fri, Mar 22, 2013 at 08:05:27AM +0000, Zhang, Yang Z wrote: > >>>> Gleb Natapov wrote on 2013-03-22: > >>>>> On Fri, Mar 22, 2013 at 01:24:05PM +0800, Yang Zhang wrote: > >>>>>> From: Yang Zhang > >>>>>> > >>>>>> Current interrupt coalescing logci which only used by RTC has conflict > >>>>>> with Posted Interrupt. > >>>>>> This patch introduces a new mechinism to use eoi to track interrupt: > >>>>>> When delivering an interrupt to vcpu, the pending_eoi set to number of > >>>>>> vcpu that received the interrupt. And decrease it when each vcpu writing > >>>>>> eoi. No subsequent RTC interrupt can deliver to vcpu until all vcpus > >>>>>> write eoi. > >>>>>> > >>>>>> Signed-off-by: Yang Zhang > >>>>>> --- > >>>>>> virt/kvm/ioapic.c | 40 +++++++++++++++++++++++++++++++++++++++- > >>>>>> 1 files changed, 39 insertions(+), 1 deletions(-) > >>>>>> diff --git a/virt/kvm/ioapic.c b/virt/kvm/ioapic.c > >>>>>> index c991e58..df16daf 100644 > >>>>>> --- a/virt/kvm/ioapic.c > >>>>>> +++ b/virt/kvm/ioapic.c > >>>>>> @@ -114,6 +114,29 @@ static void rtc_irq_restore(struct kvm_ioapic > >>> *ioapic) > >>>>>> ioapic->rtc_status.pending_eoi = pending_eoi; > >>>>>> } > >>>>>> +static void rtc_irq_ack_eoi(struct kvm_vcpu *vcpu, > >>>>>> + struct rtc_status *rtc_status, int irq) > >>>>>> +{ > >>>>>> + if (irq != RTC_GSI) > >>>>>> + return; > >>>>>> + > >>>>>> + if (test_and_clear_bit(vcpu->vcpu_id, rtc_status->dest_map)) > >>>>>> + --rtc_status->pending_eoi; > >>>>>> + > >>>>>> + WARN_ON(rtc_status->pending_eoi < 0); > >>>>>> +} > >>>>>> + > >>>>>> +static bool rtc_irq_check(struct kvm_ioapic *ioapic, int irq) > >>>>>> +{ > >>>>>> + if (irq != RTC_GSI) > >>>>>> + return false; > >>>>>> + > >>>>>> + if (ioapic->rtc_status.pending_eoi > 0) > >>>>>> + return true; /* coalesced */ > >>>>>> + > >>>>>> + return false; > >>>>>> +} > >>>>>> + > >>>>>> static int ioapic_service(struct kvm_ioapic *ioapic, unsigned int idx) > >>>>>> { > >>>>>> union kvm_ioapic_redirect_entry *pent; > >>>>>> @@ -229,6 +252,7 @@ static int ioapic_deliver(struct kvm_ioapic *ioapic, > > int > >>>>> irq) > >>>>>> { > >>>>>> union kvm_ioapic_redirect_entry *entry = &ioapic->redirtbl[irq]; > >>>>>> struct kvm_lapic_irq irqe; > >>>>>> + int ret; > >>>>>> > >>>>>> ioapic_debug("dest=%x dest_mode=%x delivery_mode=%x " > >>>>>> "vector=%x trig_mode=%x\n", > >>>>>> @@ -244,7 +268,14 @@ static int ioapic_deliver(struct kvm_ioapic *ioapic, > >>> int > >>>>> irq) > >>>>>> irqe.level = 1; > >>>>>> irqe.shorthand = 0; > >>>>>> - return kvm_irq_delivery_to_apic(ioapic->kvm, NULL, &irqe, NULL); > >>>>>> + if (irq == RTC_GSI) { > >>>>>> + ret = kvm_irq_delivery_to_apic(ioapic->kvm, NULL, &irqe, > >>>>>> + ioapic->rtc_status.dest_map); > >>>>>> + ioapic->rtc_status.pending_eoi = ret; > >>>>> We should track status only if IRQ_STATUS ioctl was used to inject an > >>>>> interrupt. > >>>> We already know RTC will use IRQ_STATUS ioctl. Why check it again? > >>>> > >>> QEMU does. QEMU is not the only userspace. > >> And this will break other userspace. > >> > > How? > If other userspace has the reinjection logic for RTC, but it not uses IRQ_STATUS, then it cannot get the right coalescing info. If it also use IRQ_STATUS to get coalescing info, then we don't need the IRQ_STATUS check. > If userspace does not care about irq status it does not use IRQ_STATUS ioctl and we should not go extra mile to provide one. Not everyone cares about running Windows as a guest. -- Gleb.