From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Gregory Haskins" Subject: Re: [PATCH 2/2] KVM: in-kernel-apic modification to QEMU Date: Thu, 10 May 2007 11:30:34 -0400 Message-ID: <46430237.BA47.005A.0@novell.com> References: <20070510124839.10286.18144.stgit@novell1.haskins.net> <20070510125638.10286.95798.stgit@novell1.haskins.net> <46433083.1080809@codemonkey.ws> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: kvm-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org To: "Anthony Liguori" Return-path: In-Reply-To: <46433083.1080809-rdkfGonbjUSkNkDKm+mE6A@public.gmane.org> Content-Disposition: inline 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 Hehe..ya, your points are all valid. Note that I never really intended this code to be reviewed quite yet. Its included purely to enable testing of the kernel side. I'll be the first to admit its a complete hack :). I'll be working on polishing this up next. Stay tuned. -Greg >>> On Thu, May 10, 2007 at 10:47 AM, in message <46433083.1080809-rdkfGonbjUSkNkDKm+mE6A@public.gmane.org>, Anthony Liguori wrote: > Gregory Haskins wrote: >> Signed- off- by: Gregory Haskins >> --- >> >> qemu/hw/apic.c | 22 ++++++++++++++++++++-- >> qemu/hw/pc.c | 13 ++++++++++++- >> qemu/qemu- kvm.c | 18 ++++++++++-------- >> qemu/vl.h | 2 +- >> user/kvmctl.c | 25 +++++++++++++++++++++++-- >> user/kvmctl.h | 5 +++-- >> user/main.c | 3 +-- >> 7 files changed, 70 insertions(+), 18 deletions(- ) >> >> diff -- git a/qemu/hw/apic.c b/qemu/hw/apic.c >> index 5704224..8aa3ef1 100644 >> --- a/qemu/hw/apic.c >> +++ b/qemu/hw/apic.c >> @@ - 19,6 +19,11 @@ >> */ >> #include "vl.h" >> >> +#ifdef USE_KVM >> +#include "kvmctl.h" >> +extern kvm_context_t kvm_context; >> +#endif >> > > extern's are usually a red- herring that something isn't right. > >> //#define DEBUG_APIC >> //#define DEBUG_IOAPIC >> >> @@ - 87,6 +92,7 @@ typedef struct APICState { >> } APICState; >> >> struct IOAPICState { >> + CPUState *cpu_env; >> uint8_t id; >> uint8_t ioregsel; >> >> @@ - 895,10 +901,21 @@ static void ioapic_service(IOAPICState *s) >> vector = pic_read_irq(isa_pic); >> else >> vector = entry & 0xff; >> - >> + >> +#ifdef USE_KVM >> + if (kvm_allowed) { >> + kvm_apic_bus_deliver(kvm_context, dest, trig_mode, >> + dest_mode, delivery_mode, vector); >> > > Perhaps a qemu- kvm.c wrapper that doesn't take a kvm_context is needed > here. That would eliminate the extern. > >> + cpu_interrupt(s- >cpu_env, CPU_INTERRUPT_HARD); >> + } else { >> +#endif >> apic_get_delivery_bitmask(deliver_bitmask, dest, > dest_mode); >> apic_bus_deliver(deliver_bitmask, delivery_mode, >> vector, polarity, trig_mode); >> +#ifdef USE_KVM >> + } >> +#endif >> + >> } >> } >> } >> @@ - 1052,7 +1069,7 @@ static CPUWriteMemoryFunc *ioapic_mem_write[3] = { >> ioapic_mem_writel, >> }; >> >> - IOAPICState *ioapic_init(void) >> +IOAPICState *ioapic_init(CPUState *env) >> { >> IOAPICState *s; >> int io_memory; >> @@ - 1061,6 +1078,7 @@ IOAPICState *ioapic_init(void) >> if (!s) >> return NULL; >> ioapic_reset(s); >> + s- >cpu_env = env; >> s- >id = last_apic_id++; >> >> io_memory = cpu_register_io_memory(0, ioapic_mem_read, >> diff -- git a/qemu/hw/pc.c b/qemu/hw/pc.c >> index eda49cf..3862a8f 100644 >> --- a/qemu/hw/pc.c >> +++ b/qemu/hw/pc.c >> @@ - 91,6 +91,9 @@ int cpu_get_pic_interrupt(CPUState *env) >> { >> int intno; >> >> +#ifdef USE_KVM >> + if (!kvm_allowed) { >> +#endif >> intno = apic_get_interrupt(env); >> if (intno >= 0) { >> > > Indent the inner block please :- ) > >> /* set irq request if a PIC irq is still pending */ >> @@ - 98,10 +101,15 @@ int cpu_get_pic_interrupt(CPUState *env) >> pic_update_irq(isa_pic); >> return intno; >> } >> + >> /* read the irq from the PIC */ >> if (!apic_accept_pic_intr(env)) >> return - 1; >> >> +#ifdef USE_KVM >> + } >> +#endif >> + >> intno = pic_read_irq(isa_pic); >> return intno; >> } >> @@ - 483,6 +491,9 @@ static void pc_init1(int ram_size, int vga_ram_size, int > boot_device, >> } >> register_savevm("cpu", i, 4, cpu_save, cpu_load, env); >> qemu_register_reset(main_cpu_reset, env); >> +#ifdef USE_KVM >> + if (!kvm_allowed) >> +#endif >> if (pci_enabled) { >> apic_init(env); >> } >> > > Same here. Perhaps we should always define kvm_allowed and set it to > zero when !defined(USE_KVM). We can probably just #if stuff out that's > performance critical. > >> >> - sregs.apic_base = cpu_get_apic_base(env); >> + /* These two are no longer used once the in- kernel APIC is enabled */ >> + sregs.apic_base = 0; >> + sregs.cr8 = 0; >> + >> sregs.efer = env- >efer; >> - sregs.cr8 = cpu_get_apic_tpr(env); >> >> kvm_set_sregs(kvm_context, 0, &sregs); >> >> @@ - 321,7 +323,7 @@ static void save_regs(CPUState *env) >> env- >cr[3] = sregs.cr3; >> env- >cr[4] = sregs.cr4; >> >> - cpu_set_apic_base(env, sregs.apic_base); >> + //cpu_set_apic_base(env, sregs.apic_base); > > I take it this code cannot support an in- QEMU APIC? So it breaks - no- kvm? > > Regards, > > Anthony Liguori ------------------------------------------------------------------------- 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/