On Fri, Mar 22, 2013 at 10:50:55AM +0000, Zhang, Yang Z wrote:
> Gleb Natapov wrote on 2013-03-22:
> > On Fri, Mar 22, 2013 at 08:51:47AM +0000, Zhang, Yang Z wrote:
> >> Gleb Natapov wrote on 2013-03-22:
> >>> 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 <yang.z.zh...@intel.com>
> >>>>>>>>>>
> >>>>>>>>>> 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 <yang.z.zh...@intel.com>
> >>>>>>>>>> ---
> >>>>>>>>>> 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.
> >> I see your point. But if no windows guest running, RTC is hardly used
> >> by other guests and the overheard can be ignore.
> >>
> > Anyone can use RTC is Linux guest. Don't know about others.
> I see.
> Since pass IRQ_STATUS to ioapic need to change many functions, how about add
> a variable in rtc_status:
> struct rtc_status {
> bool IRQ_STATUS
> };
>
> And set it in kvm_vm_ioctl():
> case KVM_IRQ_LINE_STATUS:
> if(irq == RTC_GSI && ioapic)
> ioapic->rtc_status.IRQ_STATUS = true;
>
I do not like that kind of hacks. Just put functions change in separate,
easy to review patch.
--
Gleb.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
