| Message ID | 20141106164534.GA15407@potion.brq.redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 06/11/2014 17:45, Radim Kr?má? wrote: > - } 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; Why are you removing the kvm_apic_hw_enabled(apic)? We do not reset the APIC to the power-up values (the SDM says that "may" happen) when the enabled bit is turned off in the APIC_BASE MSR. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2014-11-10 18:35+0100, Paolo Bonzini: > On 06/11/2014 17:45, Radim Kr?má? wrote: > > - } 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; > > Why are you removing the kvm_apic_hw_enabled(apic)? We do not reset the > APIC to the power-up values (the SDM says that "may" happen) when the > enabled bit is turned off in the APIC_BASE MSR. We checked for that before, in kvm_apic_present(). -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. Paolo -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;