From mboxrd@z Thu Jan  1 00:00:00 1970
From: Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [PATCH v6 6/6] KVM: Use eoi to track RTC interrupt delivery status
Date: Fri, 29 Mar 2013 09:35:57 +0100
Message-ID: <5155526D.6010405@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> <A9667DDFB95DB7438FA9D7D576C3D87E099F6A3D@SHSMSX101.ccr.corp.intel.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: 7bit
Cc: "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
	"gleb@redhat.com" <gleb@redhat.com>,
	"mtosatti@redhat.com" <mtosatti@redhat.com>,
	"Zhang, Xiantao" <xiantao.zhang@intel.com>
To: "Zhang, Yang Z" <yang.z.zhang@intel.com>
Return-path: <kvm-owner@vger.kernel.org>
Received: from mail-qa0-f48.google.com ([209.85.216.48]:41890 "EHLO
	mail-qa0-f48.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
	with ESMTP id S1755375Ab3C2IgD (ORCPT <rfc822;kvm@vger.kernel.org>);
	Fri, 29 Mar 2013 04:36:03 -0400
Received: by mail-qa0-f48.google.com with SMTP id hu16so314565qab.14
        for <kvm@vger.kernel.org>; Fri, 29 Mar 2013 01:36:02 -0700 (PDT)
In-Reply-To: <A9667DDFB95DB7438FA9D7D576C3D87E099F6A3D@SHSMSX101.ccr.corp.intel.com>
Sender: kvm-owner@vger.kernel.org
List-ID: <kvm.vger.kernel.org>

Il 29/03/2013 04:25, Zhang, Yang Z ha scritto:
> 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.

Even WARN_ON is too much if it is guest-triggerable.  But then it is
better to make it "+=", I think.

Paolo