From mboxrd@z Thu Jan 1 00:00:00 1970 From: Radim =?utf-8?B?S3LEjW3DocWZ?= Subject: Re: [PATCH 14/21] KVM: x86: Software disabled APIC should still deliver NMIs Date: Thu, 27 Nov 2014 14:39:58 +0100 Message-ID: <20141127133957.GA383@potion.brq.redhat.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> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: Paolo Bonzini , kvm list To: Nadav Amit Return-path: Received: from mx1.redhat.com ([209.132.183.28]:42641 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751077AbaK0NkD (ORCPT ); Thu, 27 Nov 2014 08:40:03 -0500 Content-Disposition: inline In-Reply-To: <02C5C22B-2809-438B-BD42-8C235F1E5CEE@gmail.com> Sender: kvm-owner@vger.kernel.org List-ID: 2014-11-26 19:01+0200, Nadav Amit: > Sorry for the late and long reply, but I got an issue with the new ve= rsion > (and my previous version as well). Indeed, the SDM states that DFR sh= ould > be the same for enabled CPUs, and that the BIOS should get all CPUs i= n > either xAPIC or x2APIC. Yet, there is nothing that says all CPUs need= to be > in xAPIC/x2APIC mode. >=20 > In my tests (which pass on bare-metal), I got a scenario in which som= e CPUs > are in xAPIC mode, the BSP changed (which is currently not handled co= rrectly > by KVM) and the BSP has x2APIC enabled. How many (V)CPUs were you using? (We fail hard with logical destination x2APIC and 16+ VCPUs.) > All the core APICs are > software-enabled. The expected behaviour is that the CPUs with x2APIC > enabled would work in x2APIC mode. (Nice, I bet that made some Intel designers happy.) 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 would be also interpreted as xAPIC broadcast. > I think such a transitory scenario is possible on real-systems as wel= l, > 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 t= o 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 e= nabled. > Yet, I have not finished doing and checking the BSP fix and other dep= endent > INIT signal handling fixes. >=20 > 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 (regardle= ss to > whether APIC is software enabled), otherwise use the configuration of= the > last enabled APIC? I don't think this patch improves anything. (Both behaviors are wrong and I think the current one is a bit less so.= ) 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 interrupt remapping, which is an architectural requirement for x2APIC ... And for more concrete points: - Physical x2APIC isn't affected (only broadcast, which is incorrect either way) - Logical x2APIC and xAPIC don't work at the same time - Btw. logical x2APIC isn't supposed to work (see KVM_X2APIC_CID_BITS= ) - Logical xAPIC is shifted incorrectly in x2APIC mode, so they are al= l going to be inaccessible (ldr =3D 0) - Our map isn't designed to allow x2APIC and xAPIC at the same time - Your patch does not cover the case where sw-disabled x2APIC is "before" sw-enabled xAPIC, only if it is after. > -- >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 *kv= m) > 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 *kvm= ) > } > } > =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 >=20 >=20