| Message ID | 20171122180908.31389-1-rkrcmar@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 22/11/2017 19:09, Radim Krčmář wrote: > QEMU saves only 8 bits of APIC LDR, which means that it does not support > x2APIC. The correct way of fixing this would be to save and restore the > full 32 bit register, but because x2APIC LDR is a function of x2APIC ID, > we can also compute it and keep the migration format untouched. > > KVM always expected the LDR format to follow the xAPIC/x2APIC standard, > but pre 4.1 KVMs used non-standard x2APIC ID in case the OS changed > xAPIC ID before switching to x2APIC, which means that QEMU has to use > the kvm_x2apic_api feature to derive the x2APIC ID. > > This bug has also been addressed on the KVM side with patch 5849d75a5c9b > ("KVM: lapic: Fixup LDR on load in x2apic"). > + if (s->apicbase & MSR_IA32_APICBASE_EXTD) { > + kvm_apic_set_reg(kapic, 0xd, kvm_apic_calc_x2apic_ldr(s)); Is this correct if the kernel doesn't support the new-style x2APIC API? In the end, it seems simpler to just fix it in the kernel. Paolo > + } else { > + kvm_apic_set_reg(kapic, 0xd, s->log_dest << 24); > + }
On Wed, Nov 22, 2017 at 07:09:08PM +0100, Radim Krčmář wrote: > QEMU saves only 8 bits of APIC LDR, which means that it does not support > x2APIC. The correct way of fixing this would be to save and restore the > full 32 bit register, but because x2APIC LDR is a function of x2APIC ID, > we can also compute it and keep the migration format untouched. > > KVM always expected the LDR format to follow the xAPIC/x2APIC standard, > but pre 4.1 KVMs used non-standard x2APIC ID in case the OS changed > xAPIC ID before switching to x2APIC, which means that QEMU has to use > the kvm_x2apic_api feature to derive the x2APIC ID. > > This bug has also been addressed on the KVM side with patch 5849d75a5c9b > ("KVM: lapic: Fixup LDR on load in x2apic"). Is this sufficient to fix the bug on hosts that lack KVM commit 5849d75a5c9b, or we need both the KVM and QEMU patches? > > Reported-by: Dr. David Alan Gilbert <dgilbert@redhat.com> > Reported-by: Yiqian Wei <yiwei@redhat.com> > Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> > --- > I haven't tested that it actually fixes the bug, > https://bugzilla.redhat.com/show_bug.cgi?id=1502591. > > hw/i386/kvm/apic.c | 19 +++++++++++++++++-- > 1 file changed, 17 insertions(+), 2 deletions(-) > > diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c > index 1df6d26816f9..89df165a04bf 100644 > --- a/hw/i386/kvm/apic.c > +++ b/hw/i386/kvm/apic.c > @@ -30,6 +30,13 @@ static inline uint32_t kvm_apic_get_reg(struct kvm_lapic_state *kapic, > return *((uint32_t *)(kapic->regs + (reg_id << 4))); > } > > +static inline uint32_t kvm_apic_calc_x2apic_ldr(APICCommonState *s) > +{ > + uint32_t id = kvm_has_x2apic_api() ? s->initial_apic_id : s->id; > + > + return ((id >> 4) << 16) | (1 << (id & 0xf)); > +} > + > static void kvm_put_apic_state(APICCommonState *s, struct kvm_lapic_state *kapic) > { > int i; > @@ -41,7 +48,11 @@ static void kvm_put_apic_state(APICCommonState *s, struct kvm_lapic_state *kapic > 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); > + if (s->apicbase & MSR_IA32_APICBASE_EXTD) { > + kvm_apic_set_reg(kapic, 0xd, kvm_apic_calc_x2apic_ldr(s)); > + } else { > + kvm_apic_set_reg(kapic, 0xd, s->log_dest << 24); > + } > kvm_apic_set_reg(kapic, 0xe, s->dest_mode << 28 | 0x0fffffff); > kvm_apic_set_reg(kapic, 0xf, s->spurious_vec); > for (i = 0; i < 8; i++) { > @@ -71,7 +82,11 @@ void kvm_get_apic_state(DeviceState *dev, struct kvm_lapic_state *kapic) > } > 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; > + if (s->apicbase & MSR_IA32_APICBASE_EXTD) { > + assert(kvm_apic_get_reg(kapic, 0xd) == kvm_apic_calc_x2apic_ldr(s)); I assume this assert() won't trigger if the host just lacks the kernel patch, will it? What if we're going to migrate to a QEMU version that doesn't have this patch applied? Do we want to send the same log_dest value as old QEMU versions, just in case? (Those 8 bits QEMU currently sets at LDR[31:24] seem completely useless, but maybe it won't hurt to keep them?) > + } else { > + s->log_dest = kvm_apic_get_reg(kapic, 0xd) >> 24; > + } > s->dest_mode = kvm_apic_get_reg(kapic, 0xe) >> 28; > s->spurious_vec = kvm_apic_get_reg(kapic, 0xf); > for (i = 0; i < 8; i++) { > -- > 2.14.2 >
2017-11-22 20:26+0100, Paolo Bonzini: > On 22/11/2017 19:09, Radim Krčmář wrote: > > QEMU saves only 8 bits of APIC LDR, which means that it does not support > > x2APIC. The correct way of fixing this would be to save and restore the > > full 32 bit register, but because x2APIC LDR is a function of x2APIC ID, > > we can also compute it and keep the migration format untouched. > > > > KVM always expected the LDR format to follow the xAPIC/x2APIC standard, > > but pre 4.1 KVMs used non-standard x2APIC ID in case the OS changed > > xAPIC ID before switching to x2APIC, which means that QEMU has to use > > the kvm_x2apic_api feature to derive the x2APIC ID. > > > > This bug has also been addressed on the KVM side with patch 5849d75a5c9b > > ("KVM: lapic: Fixup LDR on load in x2apic"). > > > + if (s->apicbase & MSR_IA32_APICBASE_EXTD) { > > + kvm_apic_set_reg(kapic, 0xd, kvm_apic_calc_x2apic_ldr(s)); > > Is this correct if the kernel doesn't support the new-style x2APIC API? Should be: QEMU will use the APIC_ID register in that case, which contains the x2APIC ID that KVM used to compute the LDR from. (old-style APIC_ID just cannot store more than 8 bits and isn't tied to vcpu_id.) > In the end, it seems simpler to just fix it in the kernel. We already have the workaround in KVM, so dropping this one doesn't make that much of a difference. I perceive it as solely QEMU bug, though. :)
2017-11-22 18:28-0200, Eduardo Habkost: > On Wed, Nov 22, 2017 at 07:09:08PM +0100, Radim Krčmář wrote: > > QEMU saves only 8 bits of APIC LDR, which means that it does not support > > x2APIC. The correct way of fixing this would be to save and restore the > > full 32 bit register, but because x2APIC LDR is a function of x2APIC ID, > > we can also compute it and keep the migration format untouched. > > > > KVM always expected the LDR format to follow the xAPIC/x2APIC standard, > > but pre 4.1 KVMs used non-standard x2APIC ID in case the OS changed > > xAPIC ID before switching to x2APIC, which means that QEMU has to use > > the kvm_x2apic_api feature to derive the x2APIC ID. > > > > This bug has also been addressed on the KVM side with patch 5849d75a5c9b > > ("KVM: lapic: Fixup LDR on load in x2apic"). > > Is this sufficient to fix the bug on hosts that lack KVM commit > 5849d75a5c9b, or we need both the KVM and QEMU patches? Good point, either one is enough to fix the bug. This patch makes x2APIC LDR work on old KVMs.
diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c index 1df6d26816f9..89df165a04bf 100644 --- a/hw/i386/kvm/apic.c +++ b/hw/i386/kvm/apic.c @@ -30,6 +30,13 @@ static inline uint32_t kvm_apic_get_reg(struct kvm_lapic_state *kapic, return *((uint32_t *)(kapic->regs + (reg_id << 4))); } +static inline uint32_t kvm_apic_calc_x2apic_ldr(APICCommonState *s) +{ + uint32_t id = kvm_has_x2apic_api() ? s->initial_apic_id : s->id; + + return ((id >> 4) << 16) | (1 << (id & 0xf)); +} + static void kvm_put_apic_state(APICCommonState *s, struct kvm_lapic_state *kapic) { int i; @@ -41,7 +48,11 @@ static void kvm_put_apic_state(APICCommonState *s, struct kvm_lapic_state *kapic 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); + if (s->apicbase & MSR_IA32_APICBASE_EXTD) { + kvm_apic_set_reg(kapic, 0xd, kvm_apic_calc_x2apic_ldr(s)); + } else { + kvm_apic_set_reg(kapic, 0xd, s->log_dest << 24); + } kvm_apic_set_reg(kapic, 0xe, s->dest_mode << 28 | 0x0fffffff); kvm_apic_set_reg(kapic, 0xf, s->spurious_vec); for (i = 0; i < 8; i++) { @@ -71,7 +82,11 @@ void kvm_get_apic_state(DeviceState *dev, struct kvm_lapic_state *kapic) } 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; + if (s->apicbase & MSR_IA32_APICBASE_EXTD) { + assert(kvm_apic_get_reg(kapic, 0xd) == kvm_apic_calc_x2apic_ldr(s)); + } else { + s->log_dest = kvm_apic_get_reg(kapic, 0xd) >> 24; + } s->dest_mode = kvm_apic_get_reg(kapic, 0xe) >> 28; s->spurious_vec = kvm_apic_get_reg(kapic, 0xf); for (i = 0; i < 8; i++) {
QEMU saves only 8 bits of APIC LDR, which means that it does not support x2APIC. The correct way of fixing this would be to save and restore the full 32 bit register, but because x2APIC LDR is a function of x2APIC ID, we can also compute it and keep the migration format untouched. KVM always expected the LDR format to follow the xAPIC/x2APIC standard, but pre 4.1 KVMs used non-standard x2APIC ID in case the OS changed xAPIC ID before switching to x2APIC, which means that QEMU has to use the kvm_x2apic_api feature to derive the x2APIC ID. This bug has also been addressed on the KVM side with patch 5849d75a5c9b ("KVM: lapic: Fixup LDR on load in x2apic"). Reported-by: Dr. David Alan Gilbert <dgilbert@redhat.com> Reported-by: Yiqian Wei <yiwei@redhat.com> Signed-off-by: Radim Krčmář <rkrcmar@redhat.com> --- I haven't tested that it actually fixes the bug, https://bugzilla.redhat.com/show_bug.cgi?id=1502591. hw/i386/kvm/apic.c | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-)