Re: [Qemu-devel] [PATCH 2/2] kvm: apic: set APIC base as part of kvm_apic_put

[Dr. David Alan Gilbert]

* Paolo Bonzini (address@hidden) wrote:
> From: "Dr. David Alan Gilbert" <address@hidden>
> 
> The parsing of KVM_SET_LAPIC's input depends on the current value of the
> APIC base MSR---which indeed is stored in APICCommonState---but for historical
> reasons APIC base is set through KVM_SET_SREGS together with cr8 (which is
> really just the APIC TPR) and the actual "special CPU registers".
> 
> APIC base must now be set before the actual LAPIC registers, so do that
> in kvm_apic_put.  It will be set again to the same value with KVM_SET_SREGS,
> but that's not a big issue.
> 
> This only happens since Linux 4.8, which checks for x2apic mode in
> KVM_SET_LAPIC.  However it's really a QEMU bug; until the recent
> commit 78d6a05 ("x86/lapic: Load LAPIC state at post_load", 2016-09-13)
> QEMU was indeed setting APIC base (via KVM_SET_SREGS) before the other
> LAPIC registers.

I think you've posted the wrong version here; it's not the version I tested
and it's got two problems:

> 
> Signed-off-by: Dr. David Alan Gilbert <address@hidden>
> Signed-off-by: Paolo Bonzini <address@hidden>
> ---
>  hw/i386/kvm/apic.c     | 2 ++
>  target-i386/kvm.c      | 9 +++++++++
>  target-i386/kvm_i386.h | 2 ++
>  3 files changed, 13 insertions(+)
> 
> diff --git a/hw/i386/kvm/apic.c b/hw/i386/kvm/apic.c
> index feb0002..f57fed1 100644
> --- a/hw/i386/kvm/apic.c
> +++ b/hw/i386/kvm/apic.c
> @@ -15,6 +15,7 @@
>  #include "hw/i386/apic_internal.h"
>  #include "hw/pci/msi.h"
>  #include "sysemu/kvm.h"
> +#include "target-i386/kvm_i386.h"
>  
>  static inline void kvm_apic_set_reg(struct kvm_lapic_state *kapic,
>                                      int reg_id, uint32_t val)
> @@ -130,6 +131,7 @@ static void kvm_apic_put(void *data)
>      struct kvm_lapic_state kapic;
>      int ret;
>  
> +    kvm_put_apicbase(s->cpu, s->apicbase);
>      kvm_put_apic_state(s, &kapic);
>  
>      ret = kvm_vcpu_ioctl(CPU(s->cpu), KVM_SET_LAPIC, &kapic);
> diff --git a/target-i386/kvm.c b/target-i386/kvm.c
> index 38609fd..59242e4 100644
> --- a/target-i386/kvm.c
> +++ b/target-i386/kvm.c
> @@ -1542,6 +1542,15 @@ static int kvm_put_one_msr(X86CPU *cpu, int index, 
> uint64_t value)
>      return kvm_vcpu_ioctl(CPU(cpu), KVM_SET_MSRS, cpu->kvm_msr_buf);
>  }
>  
> +void kvm_put_apicbase(X86CPU *cpu, uint64_t value)
> +{
> +    CPUX86State *env = &cpu->env;
> +    int ret;
> +
> +    ret = kvm_put_one_msr(cpu, MSR_IA32_TSCDEADLINE, value);

That's not the MSR you wanted.

Dave

> +    assert(ret == 1);
> +}
> +
>  static int kvm_put_tscdeadline_msr(X86CPU *cpu)
>  {
>      CPUX86State *env = &cpu->env;
> diff --git a/target-i386/kvm_i386.h b/target-i386/kvm_i386.h
> index 42b00af..36407e0 100644
> --- a/target-i386/kvm_i386.h
> +++ b/target-i386/kvm_i386.h
> @@ -41,4 +41,6 @@ int kvm_device_msix_set_vector(KVMState *s, uint32_t 
> dev_id, uint32_t vector,
>  int kvm_device_msix_assign(KVMState *s, uint32_t dev_id);
>  int kvm_device_msix_deassign(KVMState *s, uint32_t dev_id);
>  
> +void kvm_put_apicbase(X86CPU *cpu, uint64_t value);
> +
>  #endif
> -- 
> 2.7.4
> 
--
Dr. David Alan Gilbert / address@hidden / Manchester, UK