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 <dgilb...@redhat.com>
> Reported-by: Yiqian Wei <yi...@redhat.com>
> Signed-off-by: Radim Krčmář <rkrc...@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
>
--
Eduardo
