From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH 4/4] KVM: Add support for in-kernel LAPIC model Date: Tue, 08 May 2007 11:19:47 +0300 Message-ID: <464032A3.2010303@qumranet.com> References: <20070502212713.16738.8133.stgit@novell1.haskins.net> <20070502214330.16738.51436.stgit@novell1.haskins.net> <463EFCC5.8050408@qumranet.com> <463F0914.BA47.005A.0@novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org To: Gregory Haskins Return-path: In-Reply-To: <463F0914.BA47.005A.0-Et1tbQHTxzrQT0dZR+AlfA@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org Errors-To: kvm-devel-bounces-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org List-Id: kvm.vger.kernel.org Gregory Haskins wrote: > >>> + >>> +static struct kvm_irqdevice *get_irq_dev(struct kvm_kernint *s) >>> +{ >>> + struct kvm_irqdevice *dev; >>> + >>> + if (kvm_lapic_enabled(s- >vcpu)) >>> + dev = &s- >apic_irq; >>> + else >>> + dev = s- >ext_irq; >>> + >>> + if (!dev) >>> + kvm_crash_guest(s- >vcpu- >kvm); >>> >>> >> Can this happen? Doesn't there always have to be an external interrupt >> controller when the lapic is disabled? >> > > Well, if a non-BSP processor calls this, ext_irq could be null. Its a pathological condition, thus the harsh punishment to the guest. If its an AP, it *better* be using its LAPIC. If its not, something is busted. > > > Yes, I agree. >>> >>> >> ... for example: >> #define hrtimer timer_list >> >> I'd just drop it for now; it makes the code hard to read. We can re- add >> it later. >> > > By "drop it", I assume you mean drop the abstraction and just use native HRTIMER directly for now? > > Yes, we can worry about compatibility when this merged. >> [I didn't audit the lapic code] >> >> Where's vcpu- >cr8 gone? >> > > I know you requested that this entry remain in the VCPU structure. However, I couldnt make this work reasonably. The APIC wants to maintain this value itself so the two can very easily get out of sync. I suppose there could be ways to make the APIC use the vcpu->cr8 variable as storage for TPR (albeit messy), but this idea falls apart when we start looking at optimizations like TPR shadowing. > > Based on all that, I felt it was best to just maintain CR8 as the TPR register in the model. > > I don't understand. Isn't the tpr read-only from the point of view of the lapic? A simple set_cr8 helper should do the trick. -- error compiling committee.c: too many arguments to function ------------------------------------------------------------------------- This SF.net email is sponsored by DB2 Express Download DB2 Express C - the FREE version of DB2 express and take control of your XML. No limits. Just data. Click to get it now. http://sourceforge.net/powerbar/db2/