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 21:56:02 +0300 Message-ID: <20120416185602.GA20294@redhat.com> References: <20120415161857.GA8710@redhat.com> <20120416100807.GO11918@redhat.com> <20120416110919.GA11605@redhat.com> <20120416112446.GQ11918@redhat.com> <20120416121824.GD11605@redhat.com> <20120416123047.GS11918@redhat.com> <20120416131328.GF11605@redhat.com> <20120416151011.GB18613@redhat.com> <20120416172414.GA19544@redhat.com> <20120416173737.GA5541@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]:57291 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752401Ab2DPSzv (ORCPT ); Mon, 16 Apr 2012 14:55:51 -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 q3GItpfp024108 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Mon, 16 Apr 2012 14:55:51 -0400 Content-Disposition: inline In-Reply-To: <20120416173737.GA5541@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Mon, Apr 16, 2012 at 08:37:37PM +0300, Gleb Natapov wrote: > On Mon, Apr 16, 2012 at 08:24:16PM +0300, Michael S. Tsirkin wrote: > > On Mon, Apr 16, 2012 at 06:10:11PM +0300, Gleb Natapov wrote: > > > > > lapic changes should be minimal. > > > > > > > > Exactly my motivation. > > > > > > > My patch removes 13 lines more :) > > > > Haven't checked whether your patch is correct yet > > but I see it's checking the eoi register on each exit. > > > Only if eoi.pending == true on exit. it's checking eoi.pending always and eoi register when that is true. > > I think it's clear this would make code a bit shorter > > (not necessarily clearer as we are relying on ) but > > as I said even though the extra overhead is likely > > negligeable I have a feeling it's a wrong approach since > > this won't scale as we add more features. > > > > Let's see what do others think. > > > What I do not like about not calling eoi here is that we > have to reason about all the paths into apic and check that > we clear isr on all of them. Not at all. Instead, check each time before we read isr or ppr. > And that is not all. eoi handler > set event request, is it OK to skip it? May be, or may be not. I think it's for reinjection of lower prio interrupt. Since eoi optimization is disabled in that case we don't need to set event request. > > > > I also find the logic easier to follow as is - > > > > it is contained in lapic.c without relying > > > > on being called from x86.c as just the right moment. > > > > > > > See the patch. It change nothing outside of lapic.c. > > > > True but you rely on x86.c to call kvm_sync_lapic_from_vapic > > at the right time before injecting interrupts. > Not before injecting interrupts, but on vmexit. sync is called on entry, not on exit, no? > > I haven't checked whether that is always the case but > > to me, this makes the code less clear and more fragile. > > > We call a lot of apic functions from x86.c. That's were interrupt > injection happens. > > > Again, it appears to be a matter of taste. > > > > -- > > MST > > -- > Gleb.