From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marcelo Tosatti Subject: Re: [patch 4/4] QEMU/KVM: dont read any bits from userspace APIC emulation if its done in-kernel Date: Mon, 7 Apr 2008 14:35:52 -0300 Message-ID: <20080407173552.GA2914@dmt> References: <20080405182246.761430758@localhost.localdomain> <20080405182320.161492875@localhost.localdomain> <47F8955C.7050601@qumranet.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: kvm-devel@lists.sourceforge.net To: Avi Kivity Return-path: Content-Disposition: inline In-Reply-To: <47F8955C.7050601@qumranet.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: kvm-devel-bounces@lists.sourceforge.net Errors-To: kvm-devel-bounces@lists.sourceforge.net List-Id: kvm.vger.kernel.org On Sun, Apr 06, 2008 at 12:18:20PM +0300, Avi Kivity wrote: > Marcelo Tosatti wrote: > >Fixes loadvm/savem on SMP. > > > >Signed-off-by: Marcelo Tosatti > > > >Index: kvm-userspace.io/qemu/hw/apic.c > >=================================================================== > >--- kvm-userspace.io.orig/qemu/hw/apic.c > >+++ kvm-userspace.io/qemu/hw/apic.c > >@@ -248,8 +248,11 @@ void cpu_set_apic_base(CPUState *env, ui > > #ifdef DEBUG_APIC > > printf("cpu_set_apic_base: %016" PRIx64 "\n", val); > > #endif > >- s->apicbase = (val & 0xfffff000) | > >- (s->apicbase & (MSR_IA32_APICBASE_BSP | > >MSR_IA32_APICBASE_ENABLE)); > >+ if (kvm_enabled() && qemu_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)) { > > s->apicbase &= ~MSR_IA32_APICBASE_ENABLE; > > > > > > Can you explain how the existing code fails? It fails because apic_load (qemu/hw/apic.c), which runs after cpu_load, will set the apic base value saved by QEMU. And for some reason cpu_set_apic_base() uses the MSR_IA32_APICBASE_BSP/MSR_IA32_APICBASE_ENABLE bits from s->apicbase instead of what the caller passed. That breaks if in-kernel APIC emulation is being used. So this is the same bug which caused vmport to disable the APIC for SMP guests on X11. I think it is better to revert that partial fix and instead make sure that cpu_get_apic_base() will return the proper value. This also fixes https://sourceforge.net/tracker/?func=detail&atid=893831&aid=1936539&group_id=180599 QEMU/KVM: properly copy the in-kernel apicbase value The MSR_IA32_APICBASE_ENABLE/MSR_IA32_APICBASE_BSP bits in s->apicbase are not initialized if in-kernel APIC emulation is used, so save the actual value passed by cpu_set_apic_base() caller. Signed-off-by: Marcelo Tosatti Index: kvm-userspace.io/qemu/hw/apic.c =================================================================== --- kvm-userspace.io/qemu/hw/apic.c +++ kvm-userspace.io/qemu/hw/apic.c @@ -248,8 +248,11 @@ void cpu_set_apic_base(CPUState *env, ui #ifdef DEBUG_APIC printf("cpu_set_apic_base: %016" PRIx64 "\n", val); #endif - s->apicbase = (val & 0xfffff000) | - (s->apicbase & (MSR_IA32_APICBASE_BSP | MSR_IA32_APICBASE_ENABLE)); + if (kvm_enabled() && qemu_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)) { s->apicbase &= ~MSR_IA32_APICBASE_ENABLE; Index: marcelo/git/kvm-userspace.io/qemu/qemu-kvm-x86.c =================================================================== --- marcelo.orig/git/kvm-userspace.io/qemu/qemu-kvm-x86.c +++ marcelo/git/kvm-userspace.io/qemu/qemu-kvm-x86.c @@ -248,13 +248,8 @@ void kvm_arch_load_regs(CPUState *env) sregs.cr3 = env->cr[3]; sregs.cr4 = env->cr[4]; - if (kvm_irqchip_in_kernel(kvm_context)) { - sregs.cr8 = kvm_get_cr8(kvm_context, env->cpu_index); - sregs.apic_base = kvm_get_apic_base(kvm_context, env->cpu_index); - } else { - sregs.cr8 = cpu_get_apic_tpr(env); - sregs.apic_base = cpu_get_apic_base(env); - } + sregs.cr8 = cpu_get_apic_tpr(env); + sregs.apic_base = cpu_get_apic_base(env); sregs.efer = env->efer; ------------------------------------------------------------------------- This SF.net email is sponsored by the 2008 JavaOne(SM) Conference Register now and save $200. Hurry, offer ends at 11:59 p.m., Monday, April 7! Use priority code J8TLD2. http://ad.doubleclick.net/clk;198757673;13503038;p?http://java.sun.com/javaone