| Message ID | 1422568135-28402-8-git-send-email-rkrcmar@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 29/01/2015 22:48, Radim Kr?má? wrote: > We want to support mixed modes and the easiest solution is to avoid > optimizing those weird and unlikely scenarios. Hmm, what happens if use maxcpus to disable some CPUs at startup? Could the disabled CPUs remain in xapic mode while the others move to x2apic? 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
On 29/01/2015 22:48, Radim Kr?má? wrote: > We want to support mixed modes and the easiest solution is to avoid > optimizing those weird and unlikely scenarios. > > Signed-off-by: Radim Kr?má? <rkrcmar@redhat.com> > --- > arch/x86/include/asm/kvm_host.h | 1 + > arch/x86/kvm/lapic.c | 16 ++++++++++++++++ > arch/x86/kvm/lapic.h | 4 ++++ > 3 files changed, 21 insertions(+) > > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h > index 26d0f0f646d3..fec3188cabbb 100644 > --- a/arch/x86/include/asm/kvm_host.h > +++ b/arch/x86/include/asm/kvm_host.h > @@ -554,6 +554,7 @@ struct kvm_arch_memory_slot { > > struct kvm_apic_map { > struct rcu_head rcu; > + u8 mode; > u8 ldr_bits; > /* fields bellow are used to decode ldr values in different modes */ > u32 cid_shift, cid_mask, lid_mask; > diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c > index fab007509047..621d9df6ac63 100644 > --- a/arch/x86/kvm/lapic.c > +++ b/arch/x86/kvm/lapic.c > @@ -162,16 +162,19 @@ static void recalculate_apic_map(struct kvm *kvm) > new->ldr_bits = 32; > new->cid_shift = 16; > new->cid_mask = new->lid_mask = 0xffff; > + new->mode |= KVM_APIC_MODE_X2APIC; > } else if (kvm_apic_get_reg(apic, APIC_LDR)) { > if (kvm_apic_get_reg(apic, APIC_DFR) == > APIC_DFR_CLUSTER) { > new->cid_shift = 4; > new->cid_mask = 0xf; > new->lid_mask = 0xf; > + new->mode |= KVM_APIC_MODE_XAPIC_CLUSTER; > } else { > new->cid_shift = 8; > new->cid_mask = 0; > new->lid_mask = 0xff; > + new->mode |= KVM_APIC_MODE_XAPIC_FLAT; > } > } > > @@ -201,6 +204,13 @@ static void recalculate_apic_map(struct kvm *kvm) > > if (aid < ARRAY_SIZE(new->phys_map)) > new->phys_map[aid] = apic; > + > + /* The logical map is definitely wrong if we have multiple > + * modes at the same time. Physical is still right though. > + */ > + if (hweight8(new->mode) != 1) Better (more optimized): if (new->mode & (new->mode - 1)) Please add a comment to kvm_irq_delivery_to_apic_fast to explain what you are doing. > + continue; > + > if (lid && cid < ARRAY_SIZE(new->logical_map)) > new->logical_map[cid][ffs(lid) - 1] = apic; > } > @@ -720,6 +730,12 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src, > if (cid >= ARRAY_SIZE(map->logical_map)) > goto out; > > + if (hweight8(map->mode) != 1) { > + /* Not deliverable with optimized map. */ > + ret = false; > + goto out; > + } Put this before the computation of cid and mda. The cid and mda are all wrong with a mixed map, and the result of the "if" before is influenced by the wrong cid. Fixed by patch 8, but better get it right here. Paolo > dst = map->logical_map[cid]; > > bitmap = apic_logical_id(map, mda); > diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h > index c1ef25c89508..fd0197a93862 100644 > --- a/arch/x86/kvm/lapic.h > +++ b/arch/x86/kvm/lapic.h > @@ -8,6 +8,10 @@ > #define KVM_APIC_INIT 0 > #define KVM_APIC_SIPI 1 > > +#define KVM_APIC_MODE_XAPIC_FLAT (1 << 0) > +#define KVM_APIC_MODE_XAPIC_CLUSTER (1 << 1) > +#define KVM_APIC_MODE_X2APIC (1 << 2) > + > struct kvm_timer { > struct hrtimer timer; > s64 period; /* unit: ns */ > -- 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
2015-01-30 10:19+0100, Paolo Bonzini: > On 29/01/2015 22:48, Radim Kr?má? wrote: > > We want to support mixed modes and the easiest solution is to avoid > > optimizing those weird and unlikely scenarios. > > Hmm, what happens if use maxcpus to disable some CPUs at startup? Their APICs are kept sw-disabled in xAPIC mode. > Could > the disabled CPUs remain in xapic mode while the others move to x2apic? Yes, but it would only matter if the OS set LDR on them. I mark x2APIC with perfectly configured xAPIC on unused processors as "weird" ... (Linux 3.18 and 2.6.32 doesn't do it.) -- 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 30/01/2015 15:21, Radim Kr?má? wrote: > Yes, but it would only matter if the OS set LDR on them. > I mark x2APIC with perfectly configured xAPIC on unused processors as > "weird" ... (Linux 3.18 and 2.6.32 doesn't do it.) Ok. 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
2015-01-30 10:38+0100, Paolo Bonzini: > On 29/01/2015 22:48, Radim Kr?má? wrote: > > + if (hweight8(new->mode) != 1) > > Better (more optimized): > > if (new->mode & (new->mode - 1)) True, hweight needs to have X86_FEATURE_POPCNT to be efficient ... Do you know of a difference with it? new->mode & (new->mode - 1) | hweight8(new->mode) != 1 lea -0x1(%rax),%edi | popcnt %edi,%eax test %eax,%edi | cmp $1,%eax > Please add a comment to kvm_irq_delivery_to_apic_fast to explain what > you are doing. Would naming it kvm_apic_need_slow_delivery(), or something, be enough? > > + if (hweight8(map->mode) != 1) { > > + /* Not deliverable with optimized map. */ > > + ret = false; > > + goto out; > > + } > > Put this before the computation of cid and mda. The cid and mda are all > wrong with a mixed map, and the result of the "if" before is influenced > by the wrong cid. Fixed by patch 8, but better get it right here. Will do, thanks. -- 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 30/01/2015 15:56, Radim Kr?má? wrote: > 2015-01-30 10:38+0100, Paolo Bonzini: >> On 29/01/2015 22:48, Radim Kr?má? wrote: >>> + if (hweight8(new->mode) != 1) >> >> Better (more optimized): >> >> if (new->mode & (new->mode - 1)) > > True, hweight needs to have X86_FEATURE_POPCNT to be efficient ... > > Do you know of a difference with it? > new->mode & (new->mode - 1) | hweight8(new->mode) != 1 > lea -0x1(%rax),%edi | popcnt %edi,%eax > test %eax,%edi | cmp $1,%eax x & (x - 1) is really hweight8(new->mode) > 1. So if new->mode == 0 it would have a different result. >> Please add a comment to kvm_irq_delivery_to_apic_fast to explain what >> you are doing. > > Would naming it kvm_apic_need_slow_delivery(), or something, be enough? Or kvm_apic_map_valid() perhaps? Paolo >>> + if (hweight8(map->mode) != 1) { >>> + /* Not deliverable with optimized map. */ >>> + ret = false; >>> + goto out; >>> + } >> >> Put this before the computation of cid and mda. The cid and mda are all >> wrong with a mixed map, and the result of the "if" before is influenced >> by the wrong cid. Fixed by patch 8, but better get it right here. > > Will do, > > thanks. > -- 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
2015-01-30 16:10+0100, Paolo Bonzini: > On 30/01/2015 15:56, Radim Kr?má? wrote: > > Do you know of a difference with it? > > new->mode & (new->mode - 1) | hweight8(new->mode) != 1 > > lea -0x1(%rax),%edi | popcnt %edi,%eax > > test %eax,%edi | cmp $1,%eax > > x & (x - 1) is really hweight8(new->mode) > 1. So if new->mode == 0 it > would have a different result. (I was thinking if execution profile of those two instructions isn't vastly different. ">" in this check works too ... later it seemed like it was pushing lucky coincidence too far, so it ended as "!=" :) > >> Please add a comment to kvm_irq_delivery_to_apic_fast to explain what > >> you are doing. > > > > Would naming it kvm_apic_need_slow_delivery(), or something, be enough? > > Or kvm_apic_map_valid() perhaps? Sure, thanks. -- 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 26d0f0f646d3..fec3188cabbb 100644 --- a/arch/x86/include/asm/kvm_host.h +++ b/arch/x86/include/asm/kvm_host.h @@ -554,6 +554,7 @@ struct kvm_arch_memory_slot { struct kvm_apic_map { struct rcu_head rcu; + u8 mode; u8 ldr_bits; /* fields bellow are used to decode ldr values in different modes */ u32 cid_shift, cid_mask, lid_mask; diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c index fab007509047..621d9df6ac63 100644 --- a/arch/x86/kvm/lapic.c +++ b/arch/x86/kvm/lapic.c @@ -162,16 +162,19 @@ static void recalculate_apic_map(struct kvm *kvm) new->ldr_bits = 32; new->cid_shift = 16; new->cid_mask = new->lid_mask = 0xffff; + new->mode |= KVM_APIC_MODE_X2APIC; } else if (kvm_apic_get_reg(apic, APIC_LDR)) { if (kvm_apic_get_reg(apic, APIC_DFR) == APIC_DFR_CLUSTER) { new->cid_shift = 4; new->cid_mask = 0xf; new->lid_mask = 0xf; + new->mode |= KVM_APIC_MODE_XAPIC_CLUSTER; } else { new->cid_shift = 8; new->cid_mask = 0; new->lid_mask = 0xff; + new->mode |= KVM_APIC_MODE_XAPIC_FLAT; } } @@ -201,6 +204,13 @@ static void recalculate_apic_map(struct kvm *kvm) if (aid < ARRAY_SIZE(new->phys_map)) new->phys_map[aid] = apic; + + /* The logical map is definitely wrong if we have multiple + * modes at the same time. Physical is still right though. + */ + if (hweight8(new->mode) != 1) + continue; + if (lid && cid < ARRAY_SIZE(new->logical_map)) new->logical_map[cid][ffs(lid) - 1] = apic; } @@ -720,6 +730,12 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src, if (cid >= ARRAY_SIZE(map->logical_map)) goto out; + if (hweight8(map->mode) != 1) { + /* Not deliverable with optimized map. */ + ret = false; + goto out; + } + dst = map->logical_map[cid]; bitmap = apic_logical_id(map, mda); diff --git a/arch/x86/kvm/lapic.h b/arch/x86/kvm/lapic.h index c1ef25c89508..fd0197a93862 100644 --- a/arch/x86/kvm/lapic.h +++ b/arch/x86/kvm/lapic.h @@ -8,6 +8,10 @@ #define KVM_APIC_INIT 0 #define KVM_APIC_SIPI 1 +#define KVM_APIC_MODE_XAPIC_FLAT (1 << 0) +#define KVM_APIC_MODE_XAPIC_CLUSTER (1 << 1) +#define KVM_APIC_MODE_X2APIC (1 << 2) + struct kvm_timer { struct hrtimer timer; s64 period; /* unit: ns */
We want to support mixed modes and the easiest solution is to avoid optimizing those weird and unlikely scenarios. Signed-off-by: Radim Kr?má? <rkrcmar@redhat.com> --- arch/x86/include/asm/kvm_host.h | 1 + arch/x86/kvm/lapic.c | 16 ++++++++++++++++ arch/x86/kvm/lapic.h | 4 ++++ 3 files changed, 21 insertions(+)