From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Michael S. Tsirkin" Subject: Re: [PATCHv1 dont apply] RFC: kvm eoi PV using shared memory Date: Mon, 16 Apr 2012 14:09:20 +0300 Message-ID: <20120416110919.GA11605@redhat.com> References: <20120410132756.GA14101@redhat.com> <20120415161857.GA8710@redhat.com> <20120416100807.GO11918@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: kvm@vger.kernel.org To: Gleb Natapov Return-path: Received: from mx1.redhat.com ([209.132.183.28]:33645 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752042Ab2DPLJJ (ORCPT ); Mon, 16 Apr 2012 07:09:09 -0400 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id q3GB985U007564 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 16 Apr 2012 07:09:08 -0400 Content-Disposition: inline In-Reply-To: <20120416100807.GO11918@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: Thanks very much for the review. I'll address the comments. Some questions on your comments below. On Mon, Apr 16, 2012 at 01:08:07PM +0300, Gleb Natapov wrote: > > @@ -37,6 +38,8 @@ > > #define MSR_KVM_SYSTEM_TIME_NEW 0x4b564d01 > > #define MSR_KVM_ASYNC_PF_EN 0x4b564d02 > > #define MSR_KVM_STEAL_TIME 0x4b564d03 > > +#define MSR_KVM_EOI_EN 0x4b564d04 > > +#define MSR_KVM_EOI_DISABLED 0x0L > This is valid gpa. Follow others MSR example i.e align the address to, > lets say dword, and use lsb as enable bit. We only need a single byte, since this is per-CPU - it's better to save the memory, so no alignment is required. An explicit disable msr would also address this, right? > > +static void apic_update_isr(struct kvm_lapic *apic) > > +{ > > + int vector; > > + if (!eoi_enabled(apic->vcpu) || > > + !apic->vcpu->arch.eoi.pending || > > + eoi_get_pending(apic->vcpu)) > > + return; > > + apic->vcpu->arch.eoi.pending = false; > > + vector = apic_find_highest_isr(apic); > > + if (vector == -1) > > + return; > > + apic_clear_vector(vector, apic->regs + APIC_ISR); > > +} > > + > We should just call apic_set_eoi() on exit if eoi.pending && !eoi_get_pending(). > This removes the need for the function and its calls. It's a bit of a waste: that one does all kind extra things which we know we don't need, some of the atomics. And it's datapath so extra stuff is not free. Probably a good idea to replace the call on MSR disable - I think apic_update_ppr is a better thing to call there. Is there anything else that I missed? > We already have > call to kvm_lapic_sync_from_vapic() on exit path which should be > extended to do the above. It already does this. It calls apic_set_tpr which calls apic_update_ppr which calls apic_update_isr. -- MST