Message ID | 1462568045-31085-3-git-send-email-rkrcmar@redhat.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Hi, Radim, Got several questions inline. On Fri, May 06, 2016 at 10:53:58PM +0200, Radim Krčmář wrote: > x2APIC supports up to 2^32-1 LAPICs, but most guest in coming years will > have slighly less VCPUs. Dynamic size saves memory at the cost of > turning one constant into a variable. From the manual, I see x2apic only support 2^20-16 processors, not 2^32-1. Which one is correct? From below codes [1], I still see 2^20-16, since we are using high 16 bits of dest ID as cluster ID (0-0xfffe, while I guess 0xffff should be reserved), and lower 16 bits as processor/lapic mask. [...] > +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; > + // XXX: phys_map must be allocated as multiples of 16 > + if (offset < map->size) { > + *cluster = &map->phys_map[offset]; > + *mask = dest_id & 0xffff; [1] [...] > static void recalculate_apic_map(struct kvm *kvm) > @@ -156,17 +162,28 @@ static void recalculate_apic_map(struct kvm *kvm) > struct kvm_apic_map *new, *old = NULL; > struct kvm_vcpu *vcpu; > int i; > - > - new = kzalloc(sizeof(struct kvm_apic_map), GFP_KERNEL); > + u32 size = 255; > > mutex_lock(&kvm->arch.apic_map_lock); > > + kvm_for_each_vcpu(i, vcpu, kvm) > + if (kvm_apic_present(vcpu)) > + size = max(size, kvm_apic_id(vcpu->arch.apic)); > + > + size++; > + size += (16 - size) & 16; Is this same as: size = round_up(size, 16); ? Thanks, -- peterx -- 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-05-23 16:04+0800, Peter Xu: > Hi, Radim, > > Got several questions inline. > > On Fri, May 06, 2016 at 10:53:58PM +0200, Radim Krčmář wrote: >> x2APIC supports up to 2^32-1 LAPICs, but most guest in coming years will >> have slighly less VCPUs. Dynamic size saves memory at the cost of >> turning one constant into a variable. > > From the manual, I see x2apic only support 2^20-16 processors, not > 2^32-1. Which one is correct? "up to" is a crucial part. Physical x2APIC can address 2^32-1, logical x2APIC can address (2^16-1)*16 LAPICs and the OS can choose which mode to use. I think that machines with APIC IDs >2^20 will still be able to use logical x2APIC, but higher APIC IDs are only be addressable with physical x2APIC. (Well, broadcasts would make even xAPIC "support" an unbounded amount of LAPICs. :]) > From below codes [1], I still see 2^20-16, since we are using high > 16 bits of dest ID as cluster ID (0-0xfffe, while I guess 0xffff > should be reserved), and lower 16 bits as processor/lapic mask. > > [...] > >> +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; >> + // XXX: phys_map must be allocated as multiples of 16 >> + if (offset < map->size) { >> + *cluster = &map->phys_map[offset]; >> + *mask = dest_id & 0xffff; > > [1] This function is called ...get_LOGICAL_dest, so only logical addresses are going to be passed to it. Yes, it cannot handle APIC ID > 2^20-16. > [...] > >> static void recalculate_apic_map(struct kvm *kvm) >> @@ -156,17 +162,28 @@ static void recalculate_apic_map(struct kvm *kvm) >> struct kvm_apic_map *new, *old = NULL; >> struct kvm_vcpu *vcpu; >> int i; >> - >> - new = kzalloc(sizeof(struct kvm_apic_map), GFP_KERNEL); >> + u32 size = 255; >> >> mutex_lock(&kvm->arch.apic_map_lock); >> >> + kvm_for_each_vcpu(i, vcpu, kvm) >> + if (kvm_apic_present(vcpu)) >> + size = max(size, kvm_apic_id(vcpu->arch.apic)); >> + >> + size++; >> + size += (16 - size) & 16; > > Is this same as: > > size = round_up(size, 16); > > ? It is, thank you. -- 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 Wed, May 25, 2016 at 06:15:00PM +0200, Radim Krčmář wrote: > 2016-05-23 16:04+0800, Peter Xu: > > Hi, Radim, > > > > Got several questions inline. > > > > On Fri, May 06, 2016 at 10:53:58PM +0200, Radim Krčmář wrote: > >> x2APIC supports up to 2^32-1 LAPICs, but most guest in coming years will > >> have slighly less VCPUs. Dynamic size saves memory at the cost of > >> turning one constant into a variable. > > > > From the manual, I see x2apic only support 2^20-16 processors, not > > 2^32-1. Which one is correct? > > "up to" is a crucial part. Physical x2APIC can address 2^32-1, logical > x2APIC can address (2^16-1)*16 LAPICs and the OS can choose which mode > to use. > > I think that machines with APIC IDs >2^20 will still be able to use > logical x2APIC, but higher APIC IDs are only be addressable with > physical x2APIC. > > (Well, broadcasts would make even xAPIC "support" an unbounded amount of > LAPICs. :]) > > > From below codes [1], I still see 2^20-16, since we are using high > > 16 bits of dest ID as cluster ID (0-0xfffe, while I guess 0xffff > > should be reserved), and lower 16 bits as processor/lapic mask. > > > > [...] > > > >> +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; > >> + // XXX: phys_map must be allocated as multiples of 16 > >> + if (offset < map->size) { > >> + *cluster = &map->phys_map[offset]; > >> + *mask = dest_id & 0xffff; > > > > [1] > > This function is called ...get_LOGICAL_dest, so only logical addresses > are going to be passed to it. Yes, it cannot handle APIC ID > 2^20-16. Thanks for the explanation. I thought x2apic only support logical mode, seems I was wrong. :) -- peterx -- 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 b7e394485a5f..c93c4a7877ed 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -682,9 +682,12 @@ struct kvm_arch_memory_slot { 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]; + u32 size; + union { + struct kvm_lapic *xapic_flat_map[8]; + struct kvm_lapic *xapic_cluster_map[16][4]; + }; + struct kvm_lapic *phys_map[]; }; /* Hyper-V emulation context */ diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c index 6812e61c12d4..90e07c109dfe 100644 --- a/arch/x86/kvm/lapic.c +++ b/arch/x86/kvm/lapic.c @@ -129,26 +129,32 @@ 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; + // XXX: phys_map must be allocated as multiples of 16 + if (offset < map->size) { + *cluster = &map->phys_map[offset]; + *mask = dest_id & 0xffff; + } else + *mask = 0; -static inline void -apic_logical_id(struct kvm_apic_map *map, u32 dest_id, u16 *cid, u16 *lid) -{ - unsigned lid_bits; - - 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; - - *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) @@ -156,17 +162,28 @@ static void recalculate_apic_map(struct kvm *kvm) struct kvm_apic_map *new, *old = NULL; struct kvm_vcpu *vcpu; int i; - - new = kzalloc(sizeof(struct kvm_apic_map), GFP_KERNEL); + u32 size = 255; mutex_lock(&kvm->arch.apic_map_lock); + kvm_for_each_vcpu(i, vcpu, kvm) + if (kvm_apic_present(vcpu)) + size = max(size, kvm_apic_id(vcpu->arch.apic)); + + size++; + size += (16 - size) & 16; + new = kzalloc(sizeof(struct kvm_apic_map) + + sizeof(struct kvm_lapic) * size, GFP_KERNEL); + if (!new) goto out; + new->size = size; + 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)) @@ -175,7 +192,7 @@ static void recalculate_apic_map(struct kvm *kvm) aid = kvm_apic_id(apic); ldr = kvm_apic_get_reg(apic, APIC_LDR); - if (aid < ARRAY_SIZE(new->phys_map)) + if (aid < size) new->phys_map[aid] = apic; if (apic_x2apic_mode(apic)) { @@ -188,13 +205,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, @@ -703,7 +718,6 @@ static inline bool kvm_apic_map_get_dest_lapic(struct kvm *kvm, struct kvm_lapic { int i, lowest; bool x2apic_ipi; - u16 cid; if (irq->shorthand == APIC_DEST_SELF) { *dst = &src; @@ -720,7 +734,7 @@ static inline bool kvm_apic_map_get_dest_lapic(struct kvm *kvm, struct kvm_lapic return false; if (irq->dest_mode == APIC_DEST_PHYSICAL) { - if (irq->dest_id >= ARRAY_SIZE(map->phys_map)) { + if (irq->dest_id >= map->size) { *bitmap = 0; } else { *dst = &map->phys_map[irq->dest_id]; @@ -729,18 +743,11 @@ static inline bool kvm_apic_map_get_dest_lapic(struct kvm *kvm, struct kvm_lapic 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; diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h index f71183e502ee..d818a36ce24e 100644 --- a/arch/x86/kvm/lapic.h +++ b/arch/x86/kvm/lapic.h @@ -167,7 +167,7 @@ static inline int kvm_lapic_latched_init(struct kvm_vcpu *vcpu) return lapic_in_kernel(vcpu) && test_bit(KVM_APIC_INIT, &vcpu->arch.apic->pending_events); } -static inline int kvm_apic_id(struct kvm_lapic *apic) +static inline u32 kvm_apic_id(struct kvm_lapic *apic) { return (kvm_apic_get_reg(apic, APIC_ID) >> 24) & 0xff; }
x2APIC supports up to 2^32-1 LAPICs, but most guest in coming years will have slighly less VCPUs. Dynamic size saves memory at the cost of turning one constant into a variable. apic_map mutex had to be moved before allocation to avoid races with cpu hotplug. Signed-off-by: Radim Kr?má? <rkrcmar@redhat.com> --- arch/x86/include/asm/kvm_host.h | 9 +++-- arch/x86/kvm/lapic.c | 87 ++++++++++++++++++++++------------------- arch/x86/kvm/lapic.h | 2 +- 3 files changed, 54 insertions(+), 44 deletions(-)