| Message ID | 1476444115-205593-1-git-send-email-imammedo@redhat.com |
|---|---|
| State | New |
| Headers | show |
2016-10-14 13:21+0200, Igor Mammedov: > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- Reviewed-by: Radim Krčmář <rkrcmar@redhat.com> > v4: > - restore kvm_has_x2apic_api() and use it to avoid side-effects > of kvm_enable_x2apic(). x2APIC API will be enabled by iommu > if it's present or not enabled at all. > v3: > - drop kvm_has_x2apic_api() and reuse kvm_enable_x2apic() instead > ---
On Fri, Oct 14, 2016 at 01:21:55PM +0200, Igor Mammedov wrote: > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > --- > v4: > - restore kvm_has_x2apic_api() and use it to avoid side-effects > of kvm_enable_x2apic(). x2APIC API will be enabled by iommu > if it's present or not enabled at all. > v3: > - drop kvm_has_x2apic_api() and reuse kvm_enable_x2apic() instead > --- > target-i386/kvm_i386.h | 1 + > hw/i386/kvm/apic.c | 12 ++++++++++-- > target-i386/kvm.c | 13 ++++++++++--- > 3 files changed, 21 insertions(+), 5 deletions(-) > > diff --git a/target-i386/kvm_i386.h b/target-i386/kvm_i386.h > index 5c369b1..7607929 100644 > --- a/target-i386/kvm_i386.h > +++ b/target-i386/kvm_i386.h > @@ -44,4 +44,5 @@ int kvm_device_msix_deassign(KVMState *s, uint32_t dev_id); > void kvm_put_apicbase(X86CPU *cpu, uint64_t value); > > bool kvm_enable_x2apic(void); > +bool kvm_has_x2apic_api(void); > #endif > diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c > index be55102..39b73e7 100644 > --- a/hw/i386/kvm/apic.c > +++ b/hw/i386/kvm/apic.c > @@ -34,7 +34,11 @@ static void kvm_put_apic_state(APICCommonState *s, struct kvm_lapic_state *kapic > int i; > > memset(kapic, 0, sizeof(*kapic)); > - kvm_apic_set_reg(kapic, 0x2, s->id << 24); > + if (kvm_has_x2apic_api() && s->apicbase & MSR_IA32_APICBASE_EXTD) { > + kvm_apic_set_reg(kapic, 0x2, s->initial_apic_id); What happens if: * x2apic is enabled on CPUID; * guest sets MSR_IA32_APICBASE_EXTD; an * the x2apic API is not enabled? Does that mean kvm_{put,get}_apic_state() was already broken, or is the x2apic ID translated to the old format by the kernel when the x2apic API is disabled? > + } else { > + kvm_apic_set_reg(kapic, 0x2, s->id << 24); > + } > kvm_apic_set_reg(kapic, 0x8, s->tpr); > kvm_apic_set_reg(kapic, 0xd, s->log_dest << 24); > kvm_apic_set_reg(kapic, 0xe, s->dest_mode << 28 | 0x0fffffff); > @@ -59,7 +63,11 @@ void kvm_get_apic_state(DeviceState *dev, struct kvm_lapic_state *kapic) > APICCommonState *s = APIC_COMMON(dev); > int i, v; > > - s->id = kvm_apic_get_reg(kapic, 0x2) >> 24; > + if (kvm_has_x2apic_api() && s->apicbase & MSR_IA32_APICBASE_EXTD) { > + assert(kvm_apic_get_reg(kapic, 0x2) == s->initial_apic_id); > + } else { > + s->id = kvm_apic_get_reg(kapic, 0x2) >> 24; > + } > s->tpr = kvm_apic_get_reg(kapic, 0x8); > s->arb_id = kvm_apic_get_reg(kapic, 0x9); > s->log_dest = kvm_apic_get_reg(kapic, 0xd) >> 24; [...]
2016-10-18 12:56-0200, Eduardo Habkost: > On Fri, Oct 14, 2016 at 01:21:55PM +0200, Igor Mammedov wrote: >> Signed-off-by: Igor Mammedov <imammedo@redhat.com> >> --- >> v4: >> - restore kvm_has_x2apic_api() and use it to avoid side-effects >> of kvm_enable_x2apic(). x2APIC API will be enabled by iommu >> if it's present or not enabled at all. >> v3: >> - drop kvm_has_x2apic_api() and reuse kvm_enable_x2apic() instead >> --- >> diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c >> @@ -34,7 +34,11 @@ static void kvm_put_apic_state(APICCommonState *s, struct kvm_lapic_state *kapic >> int i; >> >> memset(kapic, 0, sizeof(*kapic)); >> - kvm_apic_set_reg(kapic, 0x2, s->id << 24); >> + if (kvm_has_x2apic_api() && s->apicbase & MSR_IA32_APICBASE_EXTD) { >> + kvm_apic_set_reg(kapic, 0x2, s->initial_apic_id); > > What happens if: > > * x2apic is enabled on CPUID; > * guest sets MSR_IA32_APICBASE_EXTD; an > * the x2apic API is not enabled? KVM expects APIC ID to be in upper 8 bits of the register then. Guest APIC mode does not come into play if the x2APIC API is not enabled. This is to keep compatibility with old KVMs that used xAPIC format regardless of APIC mode. > Does that mean kvm_{put,get}_apic_state() was already broken, or > is the x2apic ID translated to the old format by the kernel when > the x2apic API is disabled? The latter. KVM stores the 8 bits in an appropriate format, but it doesn't really matter to QEMU: the exchange format without enabled x2APIC API is defined to be the xAPIC one. (KVM used to keep always keep ID in xAPIC format and trapped x2APIC ID reads to shift the value.)
On Tue, Oct 18, 2016 at 06:26:54PM +0200, Radim Krčmář wrote: > 2016-10-18 12:56-0200, Eduardo Habkost: > > On Fri, Oct 14, 2016 at 01:21:55PM +0200, Igor Mammedov wrote: > >> Signed-off-by: Igor Mammedov <imammedo@redhat.com> > >> --- > >> v4: > >> - restore kvm_has_x2apic_api() and use it to avoid side-effects > >> of kvm_enable_x2apic(). x2APIC API will be enabled by iommu > >> if it's present or not enabled at all. > >> v3: > >> - drop kvm_has_x2apic_api() and reuse kvm_enable_x2apic() instead > >> --- > >> diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c > >> @@ -34,7 +34,11 @@ static void kvm_put_apic_state(APICCommonState *s, struct kvm_lapic_state *kapic > >> int i; > >> > >> memset(kapic, 0, sizeof(*kapic)); > >> - kvm_apic_set_reg(kapic, 0x2, s->id << 24); > >> + if (kvm_has_x2apic_api() && s->apicbase & MSR_IA32_APICBASE_EXTD) { > >> + kvm_apic_set_reg(kapic, 0x2, s->initial_apic_id); > > > > What happens if: > > > > * x2apic is enabled on CPUID; > > * guest sets MSR_IA32_APICBASE_EXTD; an > > * the x2apic API is not enabled? > > KVM expects APIC ID to be in upper 8 bits of the register then. > Guest APIC mode does not come into play if the x2APIC API is not > enabled. This is to keep compatibility with old KVMs that used xAPIC > format regardless of APIC mode. > > > Does that mean kvm_{put,get}_apic_state() was already broken, or > > is the x2apic ID translated to the old format by the kernel when > > the x2apic API is disabled? > > The latter. KVM stores the 8 bits in an appropriate format, but it > doesn't really matter to QEMU: the exchange format without enabled > x2APIC API is defined to be the xAPIC one. (KVM used to keep always > keep ID in xAPIC format and trapped x2APIC ID reads to shift the value.) Thanks for the clarification! Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>
diff --git a/target-i386/kvm_i386.h b/target-i386/kvm_i386.h index 5c369b1..7607929 100644 --- a/target-i386/kvm_i386.h +++ b/target-i386/kvm_i386.h @@ -44,4 +44,5 @@ int kvm_device_msix_deassign(KVMState *s, uint32_t dev_id); void kvm_put_apicbase(X86CPU *cpu, uint64_t value); bool kvm_enable_x2apic(void); +bool kvm_has_x2apic_api(void); #endif diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c index be55102..39b73e7 100644 --- a/hw/i386/kvm/apic.c +++ b/hw/i386/kvm/apic.c @@ -34,7 +34,11 @@ static void kvm_put_apic_state(APICCommonState *s, struct kvm_lapic_state *kapic int i; memset(kapic, 0, sizeof(*kapic)); - kvm_apic_set_reg(kapic, 0x2, s->id << 24); + if (kvm_has_x2apic_api() && s->apicbase & MSR_IA32_APICBASE_EXTD) { + kvm_apic_set_reg(kapic, 0x2, s->initial_apic_id); + } else { + kvm_apic_set_reg(kapic, 0x2, s->id << 24); + } kvm_apic_set_reg(kapic, 0x8, s->tpr); kvm_apic_set_reg(kapic, 0xd, s->log_dest << 24); kvm_apic_set_reg(kapic, 0xe, s->dest_mode << 28 | 0x0fffffff); @@ -59,7 +63,11 @@ void kvm_get_apic_state(DeviceState *dev, struct kvm_lapic_state *kapic) APICCommonState *s = APIC_COMMON(dev); int i, v; - s->id = kvm_apic_get_reg(kapic, 0x2) >> 24; + if (kvm_has_x2apic_api() && s->apicbase & MSR_IA32_APICBASE_EXTD) { + assert(kvm_apic_get_reg(kapic, 0x2) == s->initial_apic_id); + } else { + s->id = kvm_apic_get_reg(kapic, 0x2) >> 24; + } s->tpr = kvm_apic_get_reg(kapic, 0x8); s->arb_id = kvm_apic_get_reg(kapic, 0x9); s->log_dest = kvm_apic_get_reg(kapic, 0xd) >> 24; diff --git a/target-i386/kvm.c b/target-i386/kvm.c index 0472f45..86b41a9 100644 --- a/target-i386/kvm.c +++ b/target-i386/kvm.c @@ -129,9 +129,8 @@ static bool kvm_x2apic_api_set_flags(uint64_t flags) return !kvm_vm_enable_cap(s, KVM_CAP_X2APIC_API, 0, flags); } -#define MEMORIZE(fn) \ +#define MEMORIZE(fn, _result) \ ({ \ - static typeof(fn) _result; \ static bool _memorized; \ \ if (_memorized) { \ @@ -141,11 +140,19 @@ static bool kvm_x2apic_api_set_flags(uint64_t flags) _result = fn; \ }) +static bool has_x2apic_api; + +bool kvm_has_x2apic_api(void) +{ + return has_x2apic_api; +} + bool kvm_enable_x2apic(void) { return MEMORIZE( kvm_x2apic_api_set_flags(KVM_X2APIC_API_USE_32BIT_IDS | - KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK)); + KVM_X2APIC_API_DISABLE_BROADCAST_QUIRK), + has_x2apic_api); } static int kvm_get_tsc(CPUState *cs)
Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- v4: - restore kvm_has_x2apic_api() and use it to avoid side-effects of kvm_enable_x2apic(). x2APIC API will be enabled by iommu if it's present or not enabled at all. v3: - drop kvm_has_x2apic_api() and reuse kvm_enable_x2apic() instead --- target-i386/kvm_i386.h | 1 + hw/i386/kvm/apic.c | 12 ++++++++++-- target-i386/kvm.c | 13 ++++++++++--- 3 files changed, 21 insertions(+), 5 deletions(-)