Re: [Qemu-devel] [PATCH] kvm: apic: save and restore x2APIC LDR

From: Eduardo Habkost <ehabkost@redhat.com>
To: "Radim Krčmář" <rkrcmar@redhat.com>
Cc: qemu-devel@nongnu.org, Paolo Bonzini <pbonzini@redhat.com>,
	Richard Henderson <rth@twiddle.net>,
	"Dr . David Alan Gilbert" <dgilbert@redhat.com>
Subject: Re: [Qemu-devel] [PATCH] kvm: apic: save and restore x2APIC LDR
Date: Wed, 22 Nov 2017 18:28:00 -0200	[thread overview]
Message-ID: <20171122202800.GU3037@localhost.localdomain> (raw)
In-Reply-To: <20171122180908.31389-1-rkrcmar@redhat.com>

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
> 

-- 
Eduardo