From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1Mvtsi-0005PB-40 for qemu-devel@nongnu.org; Thu, 08 Oct 2009 10:22:12 -0400 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1Mvtsd-0005J6-9x for qemu-devel@nongnu.org; Thu, 08 Oct 2009 10:22:11 -0400 Received: from [199.232.76.173] (port=37922 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Mvtsc-0005Ig-SG for qemu-devel@nongnu.org; Thu, 08 Oct 2009 10:22:06 -0400 Received: from mx1.redhat.com ([209.132.183.28]:7620) by monty-python.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1Mvtsc-0006dw-FR for qemu-devel@nongnu.org; Thu, 08 Oct 2009 10:22:06 -0400 Date: Thu, 8 Oct 2009 11:22:02 -0300 From: Glauber Costa Subject: Re: [Qemu-devel] Re: [PATCH v2 4/9] provide in-kernel apic Message-ID: <20091008142202.GR8092@mothafucka.localdomain> References: <1254953315-5761-1-git-send-email-glommer@redhat.com> <1254953315-5761-2-git-send-email-glommer@redhat.com> <1254953315-5761-3-git-send-email-glommer@redhat.com> <1254953315-5761-4-git-send-email-glommer@redhat.com> <1254953315-5761-5-git-send-email-glommer@redhat.com> <4ACDEF42.6020706@us.ibm.com> <4ACDF297.9010303@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4ACDF297.9010303@redhat.com> List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Avi Kivity Cc: Anthony Liguori , qemu-devel@nongnu.org, kvm-devel On Thu, Oct 08, 2009 at 04:09:27PM +0200, Avi Kivity wrote: > On 10/08/2009 03:55 PM, Anthony Liguori wrote: >> Glauber Costa wrote: >>> This patch provides kvm with an in-kernel apic. We are currently not >>> enabling it. >>> The code is heavily based on what's in qemu-kvm.git. >>> >>> Signed-off-by: Glauber Costa >>> --- >>> hw/apic.c | 135 >>> +++++++++++++++++++++++++++++++++++++++++++++++++++-- >>> kvm.h | 3 + >>> target-i386/kvm.c | 18 +++++++ >>> 3 files changed, 152 insertions(+), 4 deletions(-) >>> >>> diff --git a/hw/apic.c b/hw/apic.c >>> index c89008e..5635607 100644 >>> --- a/hw/apic.c >>> +++ b/hw/apic.c >>> @@ -299,7 +299,11 @@ void cpu_set_apic_base(CPUState *env, uint64_t val) >>> #endif >>> if (!s) >>> return; >>> - s->apicbase = (val & 0xfffff000) | >>> + >>> + if (kvm_enabled() && kvm_irqchip_in_kernel()) >>> + s->apicbase = val; >>> + else >>> + s->apicbase = (val & 0xfffff000) | >>> (s->apicbase & (MSR_IA32_APICBASE_BSP | >>> MSR_IA32_APICBASE_ENABLE)); >>> /* if disabled, cannot be enabled again */ >>> if (!(val & MSR_IA32_APICBASE_ENABLE)) { >>> @@ -497,6 +501,13 @@ void apic_init_reset(CPUState *env) >>> s->wait_for_sipi = 1; >>> >>> env->halted = !(s->apicbase & MSR_IA32_APICBASE_BSP); >>> + >>> +#ifdef KVM_CAP_MP_STATE >>> + if (kvm_enabled() && kvm_irqchip_in_kernel()) >>> + env->mp_state >>> + = env->halted ? KVM_MP_STATE_UNINITIALIZED : >>> KVM_MP_STATE_RUNNABLE; >>> +#endif >> >> I don't think CAP_MP_STATE should be treated as an optional feature. >> >>> +static int kvm_kernel_lapic_load_from_user(APICState *s) >>> +{ >>> + int r = 0; >>> +#if defined(KVM_CAP_IRQCHIP) && defined(TARGET_I386) >>> + struct kvm_lapic_state apic; >>> + struct kvm_lapic_state *klapic = &apic; >>> + int i; >>> + >>> + if (!(kvm_enabled() && kvm_irqchip_in_kernel())) >>> + return 0; >>> + >>> + memset(klapic, 0, sizeof apic); >>> + kapic_set_reg(klapic, 0x2, s->id << 24); >>> + kapic_set_reg(klapic, 0x8, s->tpr); >>> + kapic_set_reg(klapic, 0xd, s->log_dest << 24); >>> + kapic_set_reg(klapic, 0xe, s->dest_mode << 28 | 0x0fffffff); >>> + kapic_set_reg(klapic, 0xf, s->spurious_vec); >>> + for (i = 0; i < 8; i++) { >>> + kapic_set_reg(klapic, 0x10 + i, s->isr[i]); >>> + kapic_set_reg(klapic, 0x18 + i, s->tmr[i]); >>> + kapic_set_reg(klapic, 0x20 + i, s->irr[i]); >>> + } >>> + kapic_set_reg(klapic, 0x28, s->esr); >>> + kapic_set_reg(klapic, 0x30, s->icr[0]); >>> + kapic_set_reg(klapic, 0x31, s->icr[1]); >>> + for (i = 0; i < APIC_LVT_NB; i++) >>> + kapic_set_reg(klapic, 0x32 + i, s->lvt[i]); >>> + kapic_set_reg(klapic, 0x38, s->initial_count); >>> + kapic_set_reg(klapic, 0x3e, s->divide_conf); >>> + >>> + r = kvm_set_lapic(s->cpu_env, klapic); >>> +#endif >>> + return r; >>> +} >> >> You should probably just setup VMState such that it directly saves >> kvm_lapic_state and then have the pre/post functions call the kernel >> ioctls to sync it. There's not a whole lot of point switching the >> state between two different structures. > > It ensures the two models are compatible. Since they're the same device > from the point of view of the guest, there's no reason for them to have > different representations or to be incompatible. live migration between something that has in-kernel irqchip and something that has not is likely to be a completely untested thing. And this is the only reason we might think of it as the same device. I don't see any value in supporting this combination