From mboxrd@z Thu Jan 1 00:00:00 1970 From: Nadav Amit Subject: Re: [PATCH 14/21] KVM: x86: Software disabled APIC should still deliver NMIs Date: Thu, 27 Nov 2014 23:45:54 +0200 Message-ID: <12B8F1E7-A4D1-46D1-948D-EAC44B88CA0D@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> <02C5C22B-2809-438B-BD42-8C235F1E5CEE@gmail.com> <20141127133957.GA383@potion.brq.redhat.com> Mime-Version: 1.0 (Mac OS X Mail 8.1 \(1993\)) Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Paolo Bonzini , kvm list To: =?utf-8?Q?Radim_Kr=C4=8Dm=C3=A1=C5=99?= Return-path: Received: from mail-wi0-f181.google.com ([209.85.212.181]:38665 "EHLO mail-wi0-f181.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750792AbaK0Vp6 convert rfc822-to-8bit (ORCPT ); Thu, 27 Nov 2014 16:45:58 -0500 Received: by mail-wi0-f181.google.com with SMTP id r20so9526148wiv.8 for ; Thu, 27 Nov 2014 13:45:57 -0800 (PST) In-Reply-To: <20141127133957.GA383@potion.brq.redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: Radim Kr=C4=8Dm=C3=A1=C5=99 wrote: > 2014-11-26 19:01+0200, Nadav Amit: >> Sorry for the late and long reply, but I got an issue with the new v= ersion >> (and my previous version as well). Indeed, the SDM states that DFR s= hould >> 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 nee= d to be >> in xAPIC/x2APIC mode. >>=20 >> In my tests (which pass on bare-metal), I got a scenario in which so= me CPUs >> are in xAPIC mode, the BSP changed (which is currently not handled c= orrectly >> by KVM) and the BSP has x2APIC enabled. >=20 > How many (V)CPUs were you using? > (We fail hard with logical destination x2APIC and 16+ VCPUs.) 2 at the moment. What failure do you refer to? >=20 >> All the core APICs are >> software-enabled. The expected behaviour is that the CPUs with x2API= C >> enabled would work in x2APIC mode. >=20 > (Nice, I bet that made some Intel designers happy.) >=20 > There shouldn't be any message conflict when using APIC IDs <255, so = it > might be possible if the x2APIC isn't programmed to issue weird > messages, like physical to nonexistent APIC ID 0xff000000, which woul= d > be also interpreted as xAPIC broadcast. >=20 >> I think such a transitory scenario is possible on real-systems as we= ll, >> 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 disable= d, >> 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 de= pendent >> INIT signal handling fixes. >>=20 >> In the meanwhile, would you be ok with restoring some of the previou= s >> behaviour - i.e., x2APIC is enabled if any CPU turned it on (regardl= ess to >> whether APIC is software enabled), otherwise use the configuration o= f the >> last enabled APIC? >=20 > I don't think this patch improves anything. > (Both behaviors are wrong and I think the current one is a bit less s= o.) >=20 > Our x2APIC implementation is a hack that allowed faster IPI thanks to= 1 > MSR exit instead of 2 MMIO ones. No OS, that doesn't know KVM's > limitations, should have enabled it because we didn't emulate interru= pt > remapping, which is an architectural requirement for x2APIC =E2=80=A6 It is a shame - I was under the impression QEMU emulation of the Intel = IOMMU would include it as well, and I now see they only did DMAR=E2=80=A6 > And for more concrete points: > - Physical x2APIC isn't affected (only broadcast, which is incorrect > either way) >=20 > - Logical x2APIC and xAPIC don't work at the same time No, but it is important to determine what is the =E2=80=9Cconsensus=E2=80= =9D APIC mode. > - Btw. logical x2APIC isn't supposed to work (see KVM_X2APIC_CID_BIT= S) Why? It is as if there is only a single cluster. You can still send an = APIC message to multiple CPUs within the same cluster (0). > - Logical xAPIC is shifted incorrectly in x2APIC mode, so they are a= ll > going to be inaccessible (ldr =3D 0) > - Our map isn't designed to allow x2APIC and xAPIC at the same time >=20 > - Your patch does not cover the case where sw-disabled x2APIC is > "before" sw-enabled xAPIC, only if it is after. I thought I covered it. The rationale was that if any lapic is in x2API= C mode, then the are all in x2APIC mode. It is done similarly to the prev= ious version (3.18). Anyhow, I have my workarounds, so do as you find appropriately. Once I = deal with the BSP issues, I may resubmit another version. Nadav >> -- >8 =E2=80=94 >> Subject: [PATCH] KVM: x86: Traverse all CPUs during recalculate_apic= _map >>=20 >> --- >> arch/x86/kvm/lapic.c | 20 +++++++++++--------- >> 1 file changed, 11 insertions(+), 9 deletions(-) >>=20 >> 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 =3D NULL; >> struct kvm_vcpu *vcpu; >> int i; >> + bool any_enabled =3D false; >>=20 >> new =3D kzalloc(sizeof(struct kvm_apic_map), GFP_KERNEL); >>=20 >> @@ -160,13 +161,21 @@ static void recalculate_apic_map(struct kvm *k= vm) >> if (!kvm_apic_present(vcpu)) >> continue; >>=20 >> + /* >> + * 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 =3D 32; >> new->cid_shift =3D 16; >> new->cid_mask =3D (1 << KVM_X2APIC_CID_BITS) - 1; >> new->lid_mask =3D 0xffff; >> new->broadcast =3D 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) =3D=3D >> APIC_DFR_CLUSTER) { >> new->cid_shift =3D 4; >> @@ -179,15 +188,8 @@ static void recalculate_apic_map(struct kvm *kv= m) >> } >> } >>=20 >> - /* >> - * 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 =3D true; >> } >>=20 >> kvm_for_each_vcpu(i, vcpu, kvm) { >> --=20 >> 1.9.1