| Message ID | 20160712200930.32143-4-rkrcmar@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2016-07-13 4:09 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>: > Logical x2APIC IDs map injectively to physical x2APIC IDs, so we can > reuse the physical array for them. This allows us to save space by > separating logical xAPIC maps. > > Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> > --- > arch/x86/include/asm/kvm_host.h | 6 ++-- > arch/x86/kvm/lapic.c | 69 +++++++++++++++++++++-------------------- > 2 files changed, 39 insertions(+), 36 deletions(-) > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index 53d39771842b..3194b19b9c7b 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -683,8 +683,10 @@ struct kvm_apic_map { > struct rcu_head rcu; > u8 mode; > struct kvm_lapic *phys_map[256]; > - /* first index is cluster id second is cpu id in a cluster */ > - struct kvm_lapic *logical_map[16][16]; > + union { > + struct kvm_lapic *xapic_flat_map[8]; > + struct kvm_lapic *xapic_cluster_map[16][4]; > + }; > }; > > /* Hyper-V emulation context */ > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > index 2987843657db..9880d03f533d 100644 > --- a/arch/x86/kvm/lapic.c > +++ b/arch/x86/kvm/lapic.c > @@ -115,26 +115,36 @@ static inline int apic_enabled(struct kvm_lapic *apic) > (LVT_MASK | APIC_MODE_MASK | APIC_INPUT_POLARITY | \ > APIC_LVT_REMOTE_IRR | APIC_LVT_LEVEL_TRIGGER) > > -/* The logical map is definitely wrong if we have multiple > - * modes at the same time. (Physical map is always right.) > - */ > -static inline bool kvm_apic_logical_map_valid(struct kvm_apic_map *map) > -{ > - return !(map->mode & (map->mode - 1)); > -} > +static inline bool kvm_apic_map_get_logical_dest(struct kvm_apic_map *map, > + u32 dest_id, struct kvm_lapic ***cluster, u16 *mask) { > + switch (map->mode) { > + case KVM_APIC_MODE_X2APIC: { > + u32 offset = (dest_id >> 16) * 16; > + u32 max_apic_id = ARRAY_SIZE(map->phys_map) - 1; > > -static inline void > -apic_logical_id(struct kvm_apic_map *map, u32 dest_id, u16 *cid, u16 *lid) > -{ > - unsigned lid_bits; > + if (offset <= max_apic_id) { > + u8 cluster_size = min(max_apic_id - offset + 1, 16U); > > - BUILD_BUG_ON(KVM_APIC_MODE_XAPIC_CLUSTER != 4); > - BUILD_BUG_ON(KVM_APIC_MODE_XAPIC_FLAT != 8); > - BUILD_BUG_ON(KVM_APIC_MODE_X2APIC != 16); > - lid_bits = map->mode; > + *cluster = &map->phys_map[offset]; > + *mask = dest_id & (0xffff >> (16 - cluster_size)); > + } else { > + *mask = 0; > + } > > - *cid = dest_id >> lid_bits; > - *lid = dest_id & ((1 << lid_bits) - 1); > + return true; > + } > + case KVM_APIC_MODE_XAPIC_FLAT: > + *cluster = map->xapic_flat_map; > + *mask = dest_id & 0xff; > + return true; > + case KVM_APIC_MODE_XAPIC_CLUSTER: > + *cluster = map->xapic_cluster_map[dest_id >> 4]; > + *mask = dest_id & 0xf; > + return true; > + default: > + /* Not optimized. */ > + return false; > + } > } > > static void recalculate_apic_map(struct kvm *kvm) > @@ -152,7 +162,8 @@ static void recalculate_apic_map(struct kvm *kvm) > > kvm_for_each_vcpu(i, vcpu, kvm) { > struct kvm_lapic *apic = vcpu->arch.apic; > - u16 cid, lid; > + struct kvm_lapic **cluster; > + u16 mask; > u32 ldr, aid; > > if (!kvm_apic_present(vcpu)) > @@ -174,13 +185,11 @@ static void recalculate_apic_map(struct kvm *kvm) > new->mode |= KVM_APIC_MODE_XAPIC_CLUSTER; > } > > - if (!kvm_apic_logical_map_valid(new)) > + if (!kvm_apic_map_get_logical_dest(new, ldr, &cluster, &mask)) > continue; As the comments of the removed function kvm_apic_logical_map_valid() mentioned: "The logical map is definitely wrong if we have multiple modes at the same time". So once one lapic offend this, we will just skip it. However, the offend mode is still not removed from the new->mode, which results in all the lapics after the offend one will be skipped and they can't get the benefit of the apic map table. I can make a patch once it make sense to you. Regards, Wanpeng Li -- 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
2016-08-02 21:46+0800, Wanpeng Li: > 2016-07-13 4:09 GMT+08:00 Radim Krčmář <rkrcmar@redhat.com>: >> Logical x2APIC IDs map injectively to physical x2APIC IDs, so we can >> reuse the physical array for them. This allows us to save space by >> separating logical xAPIC maps. >> >> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> >> --- >> arch/x86/include/asm/kvm_host.h | 6 ++-- >> arch/x86/kvm/lapic.c | 69 +++++++++++++++++++++-------------------- >> 2 files changed, 39 insertions(+), 36 deletions(-) >> >> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h >> index 53d39771842b..3194b19b9c7b 100644 >> --- a/arch/x86/include/asm/kvm_host.h >> +++ b/arch/x86/include/asm/kvm_host.h >> @@ -683,8 +683,10 @@ struct kvm_apic_map { >> struct rcu_head rcu; >> u8 mode; >> struct kvm_lapic *phys_map[256]; >> - /* first index is cluster id second is cpu id in a cluster */ >> - struct kvm_lapic *logical_map[16][16]; >> + union { >> + struct kvm_lapic *xapic_flat_map[8]; >> + struct kvm_lapic *xapic_cluster_map[16][4]; >> + }; >> }; >> >> /* Hyper-V emulation context */ >> diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c >> index 2987843657db..9880d03f533d 100644 >> --- a/arch/x86/kvm/lapic.c >> +++ b/arch/x86/kvm/lapic.c >> @@ -115,26 +115,36 @@ static inline int apic_enabled(struct kvm_lapic *apic) >> (LVT_MASK | APIC_MODE_MASK | APIC_INPUT_POLARITY | \ >> APIC_LVT_REMOTE_IRR | APIC_LVT_LEVEL_TRIGGER) >> >> -/* The logical map is definitely wrong if we have multiple >> - * modes at the same time. (Physical map is always right.) >> - */ >> -static inline bool kvm_apic_logical_map_valid(struct kvm_apic_map *map) >> -{ >> - return !(map->mode & (map->mode - 1)); >> -} >> +static inline bool kvm_apic_map_get_logical_dest(struct kvm_apic_map *map, >> + u32 dest_id, struct kvm_lapic ***cluster, u16 *mask) { >> + switch (map->mode) { >> + case KVM_APIC_MODE_X2APIC: { >> + u32 offset = (dest_id >> 16) * 16; >> + u32 max_apic_id = ARRAY_SIZE(map->phys_map) - 1; >> + if (offset <= max_apic_id) { >> + u8 cluster_size = min(max_apic_id - offset + 1, 16U); >> + *cluster = &map->phys_map[offset]; >> + *mask = dest_id & (0xffff >> (16 - cluster_size)); >> + } else { >> + *mask = 0; >> + } >> + return true; >> + } >> + case KVM_APIC_MODE_XAPIC_FLAT: >> + *cluster = map->xapic_flat_map; >> + *mask = dest_id & 0xff; >> + return true; >> + case KVM_APIC_MODE_XAPIC_CLUSTER: >> + *cluster = map->xapic_cluster_map[dest_id >> 4]; >> + *mask = dest_id & 0xf; >> + return true; >> + default: >> + /* Not optimized. */ >> + return false; >> + } > > As the comments of the removed function kvm_apic_logical_map_valid() > mentioned: "The logical map is definitely wrong if we have multiple > modes at the same time". So once one lapic offend this, we will just > skip it. However, the offend mode is still not removed from the > new->mode, which results in all the lapics after the offend one will > be skipped and they can't get the benefit of the apic map table. I can > make a patch once it make sense to you. Offending LAPIC marks the logical mapping as unusable, which was intended. kvm_apic_map_get_logical_dest() returns false. The main problem is that SDM does not clearly forbid LAPICs in x2APIC mode while some xAPIC LAPICs have nonzero LDR, so we should handle it somewhat gracefully. In combination with our IR-less IOAPIC hack, this would mean that we would have to check both interpretations (x2 and xAPIC) of a logical address before delivering. Not to mention that we would also have to decide how to handle a case when both interpretations are valid. Mixed logical mode is rare and the slow path works fine with it, so I think it doesn't make sense to do more than what we do now, i.e. detect mixed in the fast path and fall back to the slow path. (The map needs to keep a mode in any case, so we're not losing much by remembering that the mode is weird.) -- 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
Radim Krčmář <rkrcmar@redhat.com> wrote: > > Offending LAPIC marks the logical mapping as unusable, which was > intended. kvm_apic_map_get_logical_dest() returns false. > > The main problem is that SDM does not clearly forbid LAPICs in x2APIC > mode while some xAPIC LAPICs have nonzero LDR, so we should handle it > somewhat gracefully. > In combination with our IR-less IOAPIC hack, this would mean that we > would have to check both interpretations (x2 and xAPIC) of a logical > address before delivering. Not to mention that we would also have to > decide how to handle a case when both interpretations are valid. My recollection of running Intel tests is that they do leave sometimes some of the cores in the xAPIC mode while others are in x2APIC mode. In this case, IIRC they do not generate IPIs from the xAPIC core. IIRC they do expect broadcast interrupts to be received by all cores, including those with xAPIC mode. However, I am unsure about this last point. Regards, 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
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h index 53d39771842b..3194b19b9c7b 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -683,8 +683,10 @@ struct kvm_apic_map { struct rcu_head rcu; u8 mode; struct kvm_lapic *phys_map[256]; - /* first index is cluster id second is cpu id in a cluster */ - struct kvm_lapic *logical_map[16][16]; + union { + struct kvm_lapic *xapic_flat_map[8]; + struct kvm_lapic *xapic_cluster_map[16][4]; + }; }; /* Hyper-V emulation context */ diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c index 2987843657db..9880d03f533d 100644 --- a/arch/x86/kvm/lapic.c +++ b/arch/x86/kvm/lapic.c @@ -115,26 +115,36 @@ static inline int apic_enabled(struct kvm_lapic *apic) (LVT_MASK | APIC_MODE_MASK | APIC_INPUT_POLARITY | \ APIC_LVT_REMOTE_IRR | APIC_LVT_LEVEL_TRIGGER) -/* The logical map is definitely wrong if we have multiple - * modes at the same time. (Physical map is always right.) - */ -static inline bool kvm_apic_logical_map_valid(struct kvm_apic_map *map) -{ - return !(map->mode & (map->mode - 1)); -} +static inline bool kvm_apic_map_get_logical_dest(struct kvm_apic_map *map, + u32 dest_id, struct kvm_lapic ***cluster, u16 *mask) { + switch (map->mode) { + case KVM_APIC_MODE_X2APIC: { + u32 offset = (dest_id >> 16) * 16; + u32 max_apic_id = ARRAY_SIZE(map->phys_map) - 1; -static inline void -apic_logical_id(struct kvm_apic_map *map, u32 dest_id, u16 *cid, u16 *lid) -{ - unsigned lid_bits; + if (offset <= max_apic_id) { + u8 cluster_size = min(max_apic_id - offset + 1, 16U); - BUILD_BUG_ON(KVM_APIC_MODE_XAPIC_CLUSTER != 4); - BUILD_BUG_ON(KVM_APIC_MODE_XAPIC_FLAT != 8); - BUILD_BUG_ON(KVM_APIC_MODE_X2APIC != 16); - lid_bits = map->mode; + *cluster = &map->phys_map[offset]; + *mask = dest_id & (0xffff >> (16 - cluster_size)); + } else { + *mask = 0; + } - *cid = dest_id >> lid_bits; - *lid = dest_id & ((1 << lid_bits) - 1); + return true; + } + case KVM_APIC_MODE_XAPIC_FLAT: + *cluster = map->xapic_flat_map; + *mask = dest_id & 0xff; + return true; + case KVM_APIC_MODE_XAPIC_CLUSTER: + *cluster = map->xapic_cluster_map[dest_id >> 4]; + *mask = dest_id & 0xf; + return true; + default: + /* Not optimized. */ + return false; + } } static void recalculate_apic_map(struct kvm *kvm) @@ -152,7 +162,8 @@ static void recalculate_apic_map(struct kvm *kvm) kvm_for_each_vcpu(i, vcpu, kvm) { struct kvm_lapic *apic = vcpu->arch.apic; - u16 cid, lid; + struct kvm_lapic **cluster; + u16 mask; u32 ldr, aid; if (!kvm_apic_present(vcpu)) @@ -174,13 +185,11 @@ static void recalculate_apic_map(struct kvm *kvm) new->mode |= KVM_APIC_MODE_XAPIC_CLUSTER; } - if (!kvm_apic_logical_map_valid(new)) + if (!kvm_apic_map_get_logical_dest(new, ldr, &cluster, &mask)) continue; - apic_logical_id(new, ldr, &cid, &lid); - - if (lid && cid < ARRAY_SIZE(new->logical_map)) - new->logical_map[cid][ffs(lid) - 1] = apic; + if (mask) + cluster[ffs(mask) - 1] = apic; } out: old = rcu_dereference_protected(kvm->arch.apic_map, @@ -685,7 +694,6 @@ static inline bool kvm_apic_map_get_dest_lapic(struct kvm *kvm, { int i, lowest; bool x2apic_ipi; - u16 cid; if (irq->shorthand == APIC_DEST_SELF && src) { *dst = src; @@ -711,18 +719,11 @@ static inline bool kvm_apic_map_get_dest_lapic(struct kvm *kvm, return true; } - if (!kvm_apic_logical_map_valid(map)) + *bitmap = 0; + if (!kvm_apic_map_get_logical_dest(map, irq->dest_id, dst, + (u16 *)bitmap)) return false; - apic_logical_id(map, irq->dest_id, &cid, (u16 *)bitmap); - - if (cid >= ARRAY_SIZE(map->logical_map)) { - *bitmap = 0; - return true; - } - - *dst = map->logical_map[cid]; - if (!kvm_lowest_prio_delivery(irq)) return true;
Logical x2APIC IDs map injectively to physical x2APIC IDs, so we can reuse the physical array for them. This allows us to save space by separating logical xAPIC maps. Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> --- arch/x86/include/asm/kvm_host.h | 6 ++-- arch/x86/kvm/lapic.c | 69 +++++++++++++++++++++-------------------- 2 files changed, 39 insertions(+), 36 deletions(-)