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: Wed, 3 Apr 2013 07:03:56 +0300 Message-ID: <20130403040356.GA27672@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> <5151AD5E.9050303@redhat.com> <20130402130853.GD3889@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Paolo Bonzini , "kvm@vger.kernel.org" , "mtosatti@redhat.com" , "Zhang, Xiantao" To: "Zhang, Yang Z" Return-path: Received: from mx1.redhat.com ([209.132.183.28]:64178 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751313Ab3DCED7 (ORCPT ); Wed, 3 Apr 2013 00:03:59 -0400 Content-Disposition: inline In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: On Wed, Apr 03, 2013 at 12:21:05AM +0000, Zhang, Yang Z wrote: > Gleb Natapov wrote on 2013-04-02: > > On Fri, Mar 29, 2013 at 03:25:16AM +0000, Zhang, Yang Z wrote: > >> Paolo Bonzini wrote on 2013-03-26: > >>> Il 22/03/2013 06:24, Yang Zhang ha scritto: > >>>> +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); > >>>> +} > >>> > >>> This is the only case where you're passing the struct rtc_status instead > >>> of the struct kvm_ioapic. Please use the latter, and make it the first > >>> argument. > >>> > >>>> @@ -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; > >>> > >>> I think you should either add a > >>> > >>> BUG_ON(ioapic->rtc_status.pending_eoi != 0); > >>> or use "ioapic->rtc_status.pending_eoi += ret" (or both). > >>> > >> There may malicious guest to write EOI more than once. And the pending_eoi > > will be negative. But it should not be a bug. Just WARN_ON is enough. And we > > already do it in ack_eoi. So don't need to do duplicated thing here. > >> > > Since we track vcpus that already called EOI and decrement pending_eoi > > only once for each vcpu malicious guest cannot trigger it, but we > > already do WARN_ON() in rtc_irq_ack_eoi(), so I am not sure we need > > another one here. += will be correct (since pending_eoi == 0 here), but > > confusing since it makes an impression that pending_eoi may not be zero. > Yes, I also make the wrong impression. > With previous implementation, the pening_eoi may not be zero: Calculate the destination vcpu via parse IOAPIC entry, and if using lowest priority deliver mode, set all possible vcpus in dest_map even it doesn't receive it finally. At same time, a malicious guest can send IPI with same vector of RTC to those vcpus who is in dest_map but not have RTC interrupt. Then the pending_eoi will be negative. > Now, we set the dest_map with the vcpus who really received the interrupt. The above case cannot happen. So as you and Paolo suggested, it is better to use +=. > I am not suggesting that it is better to use +=. We can add BUG_ON(ioapic->rtc_status.pending_eoi != 0); but no need to resend patches just for that. -- Gleb.