From patchwork Wed Nov 26 17:01:48 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Nadav Amit X-Patchwork-Id: 5386881 Return-Path: X-Original-To: patchwork-kvm@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork1.web.kernel.org (Postfix) with ESMTP id C2C9C9F319 for ; Wed, 26 Nov 2014 17:01:58 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 9C6252013D for ; Wed, 26 Nov 2014 17:01:57 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 7D4292010E for ; Wed, 26 Nov 2014 17:01:56 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751026AbaKZRBx (ORCPT ); Wed, 26 Nov 2014 12:01:53 -0500 Received: from mail-wi0-f169.google.com ([209.85.212.169]:64111 "EHLO mail-wi0-f169.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750856AbaKZRBw convert rfc822-to-8bit (ORCPT ); Wed, 26 Nov 2014 12:01:52 -0500 Received: by mail-wi0-f169.google.com with SMTP id r20so15940007wiv.2 for ; Wed, 26 Nov 2014 09:01:51 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=content-type:mime-version:subject:from:in-reply-to:date:cc :content-transfer-encoding:message-id:references:to; bh=tlflZYbvukC4m04XWA/5SkyaO7nxX0N/hugaTrHjEtU=; b=h5hV2Np7nA/HI/TTcW55Pemc4mJgCqOCHNAu94zmlX1xra8MpwA+HalnaPoGxyBkMz +WWk6rSp0XrkR65ImZtMxlyaNxTZ/9LSpVN+/u1VJVM3F1aZWIA586Kp9B9mcq6p+pCo 2Oah9roYdV6BIdokHEgM9gsQpaigYGZJXLcICP0E3CxeJ8bqDNsxSopGtsAh1UsP/m2v qXrHusfcg11KsgJgTcDyPskfGZaJU51d4YeMKKvXhu1WsiOfI0JxZgV2TJbwEAnVhrO2 IFQGg59xSsyGPa9Pn2DWd+20SqgdEq/ZzxshlGWZJ6ajTyFxj1/GaCMddqz08k5/tnlU Xe7w== X-Received: by 10.194.176.100 with SMTP id ch4mr51457306wjc.101.1417021311167; Wed, 26 Nov 2014 09:01:51 -0800 (PST) Received: from nadav-mbp-2.cs.technion.ac.il ([132.68.46.89]) by mx.google.com with ESMTPSA id rx8sm7094598wjb.30.2014.11.26.09.01.49 for (version=TLSv1 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Wed, 26 Nov 2014 09:01:50 -0800 (PST) Mime-Version: 1.0 (Mac OS X Mail 8.1 \(1993\)) Subject: Re: [PATCH 14/21] KVM: x86: Software disabled APIC should still deliver NMIs From: Nadav Amit In-Reply-To: <546618F2.2060108@redhat.com> Date: Wed, 26 Nov 2014 19:01:48 +0200 Cc: kvm list Message-Id: <02C5C22B-2809-438B-BD42-8C235F1E5CEE@gmail.com> References: <1414922101-17626-1-git-send-email-namit@cs.technion.ac.il> <1414922101-17626-15-git-send-email-namit@cs.technion.ac.il> <545A1852.1000603@redhat.com> <5D182FD6-0365-4471-AD28-C587A624D151@gmail.com> <545B409E.1020500@redhat.com> <20141106164534.GA15407@potion.brq.redhat.com> <546618F2.2060108@redhat.com> To: Paolo Bonzini , =?utf-8?Q?Radim_Kr=C4=8Dm=C3=A1=C5=99?= X-Mailer: Apple Mail (2.1993) Sender: kvm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org X-Spam-Status: No, score=-6.8 required=5.0 tests=BAYES_00, DKIM_ADSP_CUSTOM_MED, DKIM_SIGNED, FREEMAIL_FROM, RCVD_IN_DNSWL_HI, T_DKIM_INVALID, T_RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=ham version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP Paolo Bonzini wrote: > > > On 06/11/2014 17:45, Radim Kr?má? wrote: >> 2014-11-06 10:34+0100, Paolo Bonzini: >>> On 05/11/2014 21:45, Nadav Amit wrote: >>>> If I understand the SDM correctly, in such scenario (all APICs are >>>> software disabled) the mode is left as the default - flat mode (see >> >> APIC doesn't have any global mode (it is just KVM's simplification), so >> when a message lands on the system bus, it just compares MDA with LDR >> and DFR ... >> >>>> section 10.6.2.2 "Logical Destination Mode”): "All processors that >>>> have their APIC software enabled (using the spurious vector >>>> enable/disable bit) must have their DFRs (Destination Format >>>> Registers) programmed identically. The default mode for DFR is flat >>>> mode.” >> >> I think the "default mode" points to reset state, which is flat DFR; >> and it is mentioned only because of the following sentence >> If you are using cluster mode, DFRs must be programmed before the APIC >> is software enabled. >> >>> That's not what either Bochs or QEMU do, though. (Though in the case of >>> Bochs I cannot find the place where reception of IPIs is prevented for >>> software-disabled APICs, so I'm not sure how much to trust it in this case). >>> >>> I'm not sure why software-disabled APICs could have different DFRs, if >>> the APICs can receive NMI IPIs. I'll ask Intel. >> >> When changing the mode, we can't switch DFR synchronously, so it has to >> happen and NMI may be needed (watchdog?) before APIC configuration. >> Explicit statement might have been a hint to be _extra_ careful when >> using logical destination for INIT, NMI, ... I wonder what they'll say. >> >> Anyway, Paolo's patch seems to be in the right direction, I'd modify it >> a bit though: >> >> LDR=0 isn't addressable in any logical mode, so we can ignore APICs that >> don't set it and decide the mode by the last nonzero one. >> This works in a situation, where one part is configured for cluster and >> the rest is still in reset state. >> >> (It gets harder if we allow nonzero LDRs with different DFR ... >> we'd need to split our logical map to handle it.) >> >> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c >> index 758f838..6da303e1 100644 >> --- a/arch/x86/kvm/lapic.c >> +++ b/arch/x86/kvm/lapic.c >> @@ -148,10 +148,6 @@ static void recalculate_apic_map(struct kvm *kvm) >> goto out; >> >> new->ldr_bits = 8; >> - /* flat mode is default */ >> - new->cid_shift = 8; >> - new->cid_mask = 0; >> - new->lid_mask = 0xff; >> new->broadcast = APIC_BROADCAST; >> >> kvm_for_each_vcpu(i, vcpu, kvm) { >> @@ -166,7 +162,7 @@ static void recalculate_apic_map(struct kvm *kvm) >> new->cid_mask = (1 << KVM_X2APIC_CID_BITS) - 1; >> new->lid_mask = 0xffff; >> new->broadcast = X2APIC_BROADCAST; >> - } else if (kvm_apic_hw_enabled(apic)) { >> + } else if (kvm_apic_get_reg(apic, APIC_LDR)) { >> if (kvm_apic_get_reg(apic, APIC_DFR) == >> APIC_DFR_CLUSTER) { >> new->cid_shift = 4; > > I merged this patch and Nadav’s. Sorry for the late and long reply, but I got an issue with the new version (and my previous version as well). Indeed, the SDM states that DFR should be the same for enabled CPUs, and that the BIOS should get all CPUs in either xAPIC or x2APIC. Yet, there is nothing that says all CPUs need to be in xAPIC/x2APIC mode. In my tests (which pass on bare-metal), I got a scenario in which some CPUs are in xAPIC mode, the BSP changed (which is currently not handled correctly by KVM) and the BSP has x2APIC enabled. All the core APICs are software-enabled. The expected behaviour is that the CPUs with x2APIC enabled would work in x2APIC mode. I think such a transitory scenario is possible on real-systems as well, perhaps during CPU hot-plug. It appears the previous version (before all of our changes) handled it better. I presume the most efficient way is to start determining the APIC logical mode from the BSP, and if it is disabled, traverse the rest of the CPUs until finding the first one with APIC enabled. Yet, I have not finished doing and checking the BSP fix and other dependent INIT signal handling fixes. In the meanwhile, would you be ok with restoring some of the previous behaviour - i.e., x2APIC is enabled if any CPU turned it on (regardless to whether APIC is software enabled), otherwise use the configuration of the last enabled APIC? -- >8 — Subject: [PATCH] KVM: x86: Traverse all CPUs during recalculate_apic_map --- arch/x86/kvm/lapic.c | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c index 9c90d31..6dc2be6 100644 --- a/arch/x86/kvm/lapic.c +++ b/arch/x86/kvm/lapic.c @@ -139,6 +139,7 @@ static void recalculate_apic_map(struct kvm *kvm) struct kvm_apic_map *new, *old = NULL; struct kvm_vcpu *vcpu; int i; + bool any_enabled = false; new = kzalloc(sizeof(struct kvm_apic_map), GFP_KERNEL); @@ -160,13 +161,21 @@ static void recalculate_apic_map(struct kvm *kvm) if (!kvm_apic_present(vcpu)) continue; + /* + * All APICs DFRs have to be configured the same mode by an OS. + * We take advatage of this while building logical id lookup + * table. After reset APICs are in software disabled mode, so if + * we find apic with different setting we assume this is the mode + * OS wants all apics to be in; build lookup table accordingly. + */ if (apic_x2apic_mode(apic)) { new->ldr_bits = 32; new->cid_shift = 16; new->cid_mask = (1 << KVM_X2APIC_CID_BITS) - 1; new->lid_mask = 0xffff; new->broadcast = X2APIC_BROADCAST; - } else if (kvm_apic_get_reg(apic, APIC_LDR)) { + break; + } else if (!any_enabled && kvm_apic_get_reg(apic, APIC_LDR)) { if (kvm_apic_get_reg(apic, APIC_DFR) == APIC_DFR_CLUSTER) { new->cid_shift = 4; @@ -179,15 +188,8 @@ static void recalculate_apic_map(struct kvm *kvm) } } - /* - * All APICs have to be configured in the same mode by an OS. - * We take advatage of this while building logical id loockup - * table. After reset APICs are in software disabled mode, so if - * we find apic with different setting we assume this is the mode - * OS wants all apics to be in; build lookup table accordingly. - */ if (kvm_apic_sw_enabled(apic)) - break; + any_enabled = true; } kvm_for_each_vcpu(i, vcpu, kvm) {