| Message ID | 20141127201641.GB383@potion.brq.redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Radim Kr?má? <rkrcmar@redhat.com> wrote: > 2014-11-27 21:53+0200, Nadav Amit: >> Radim Kr?má? <rkrcmar@redhat.com> wrote: >>> - new->cid_mask = (1 << KVM_X2APIC_CID_BITS) - 1; >>> - new->lid_mask = 0xffff; >>> + new->cid_mask = new->lid_mask = 0xffff; >> You set cid_mask to 0xffff, while there are only 16 clusters. I think it is >> risky (if you twist my hand would come with a scenario). > > Let's see :) APIC id is 8 bit, and we compute cluster part of LDR by > taking four upper bits, so 16 is enough. To clarify my concern - I am worried that some of the CPUs are still in xAPIC mode with LDR that does not follow x2APIC LDR scheme. > It isn't the safest programming practice, but we already fail to check > physical_map bounds and any boost to maximal APIC ID is going to require > a rewrite, thus I didn't bother to do it ... > > All uses should be covered with the following hunk, I will add it to v2 > after all reviews, > > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > index 6c2b8a5..30e4cc1 100644 > --- a/arch/x86/kvm/lapic.c > +++ b/arch/x86/kvm/lapic.c > @@ -198,7 +198,7 @@ static void recalculate_apic_map(struct kvm *kvm) > cid = apic_cluster_id(new, ldr); > lid = apic_logical_id(new, ldr); > > - if (lid) > + if (lid && cid < ARRAY_SIZE(map->logical_map)) > new->logical_map[cid][ffs(lid) - 1] = apic; > } > out: > > >> Yet, why not to set >> cid_mask to (ARRAY_SIZE(map->logical_map) - 1) ? > > We would incorrectly deliver messages intended for high clusters, > it has to be 0xffff. From the SDM, I am not sure you are correct, but your solution is fine. Nadav -- 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-27 22:39+0200, Nadav Amit: > Radim Kr?má? <rkrcmar@redhat.com> wrote: > > > 2014-11-27 21:53+0200, Nadav Amit: > >> Radim Kr?má? <rkrcmar@redhat.com> wrote: > >>> - new->cid_mask = (1 << KVM_X2APIC_CID_BITS) - 1; > >>> - new->lid_mask = 0xffff; > >>> + new->cid_mask = new->lid_mask = 0xffff; > >> You set cid_mask to 0xffff, while there are only 16 clusters. I think it is > >> risky (if you twist my hand would come with a scenario). > > > > Let's see :) APIC id is 8 bit, and we compute cluster part of LDR by > > taking four upper bits, so 16 is enough. > To clarify my concern - I am worried that some of the CPUs are still in > xAPIC mode with LDR that does not follow x2APIC LDR scheme. xAPIC has only the highest LDR byte nonzero, so it won't pass the lid check. (Logical xAPIC doesn't work with x2APIC in our implementation.) I agree, it is really obsucure and I should have put it in v1. > > We would incorrectly deliver messages intended for high clusters, > > it has to be 0xffff. > From the SDM, I am not sure you are correct, but your solution is fine. We'd have to change the code in irq_deliver...fast to account for that change, which I think is uglier than checking here. (If the message is for 0xf0000001 and 0x00000001 accepts it, SDM doesn't approve.) -- 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 6c2b8a5..30e4cc1 100644 --- a/arch/x86/kvm/lapic.c +++ b/arch/x86/kvm/lapic.c @@ -198,7 +198,7 @@ static void recalculate_apic_map(struct kvm *kvm) cid = apic_cluster_id(new, ldr); lid = apic_logical_id(new, ldr); - if (lid) + if (lid && cid < ARRAY_SIZE(map->logical_map)) new->logical_map[cid][ffs(lid) - 1] = apic; } out: