KVM: init bsp_vcpu before kvm_arch_vcpu_init
On x86 mp_state is initialized by kvm_arch_vcpu_init. Right now kvm_vcpu_is_bsp returns false because kvm->bsp_vcpu has not been initialized, so vcpu_id == 0 ends up with mp_state == KVM_MP_STATE_UNINITIALIZED.
Gleb do you see a better way to fix this?
virt/kvm/kvm_main.c +9 -4
@@ -773,6 +773,13 @@ int kvm_vcpu_init(struct kvm_vcpu *vcpu, struct kvm *kvm, unsigned id)struct page *page;int r; +mutex_lock (&kvm->lock); +#ifdef CONFIG_KVM_APIC_ARCHITECTURE +if (kvm->bsp_vcpu_id == id) + kvm->bsp_vcpu = vcpu; +#endif +mutex_unlock (&kvm->lock); +mutex_init (&vcpu->mutex); vcpu->cpu =-1 ; vcpu->kvm = kvm;
@@ -1760,14 +1767,12 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, u32 id)smp_wmb ();atomic_inc (&kvm->online_vcpus); -#ifdef CONFIG_KVM_APIC_ARCHITECTURE -if (kvm->bsp_vcpu_id == id) - kvm->bsp_vcpu = vcpu; -#endif mutex_unlock (&kvm->lock);return r; vcpu_destroy: +if (kvm->bsp_vcpu_id == id) + kvm->bsp_vcpu =NULL ;mutex_unlock (&kvm->lock);kvm_arch_vcpu_destroy (vcpu);return r;
Quoted 8 lines · Marcelo Tosatti
On x86
mp_stateis initialized bykvm_arch_vcpu_init. Right nowkvm_vcpu_is_bspreturns false becausekvm->bsp_vcpuhas not been initialized, sovcpu_id== 0 ends up withmp_state==KVM_MP_STATE_UNINITIALIZED.Gleb do you see a better way to fix this?
I have two, not necessary better ways. The first one is to change kvm_vcpu_is_bsp() to do kvm->bsp_vcpu_id == vcpu->vcpu_id. Another one is to understand why mp_state is set to runnable for bsp here at all. May be we can drop this use of kvm_vcpu_is_bsp() since mp_state will be set to RUNNABLE in kvm_arch_vcpu_ioctl_set_sregs() anyway?
Quoted 29 lines
Signed-off-by Marcelo Tosatti <[email protected]>virt/kvm/kvm_main.c +7
-3@@ -773,6 +773,13 @@ int kvm_vcpu_init(struct kvm_vcpu *vcpu, struct kvm *kvm, unsigned id)struct page *page;int r; +mutex_lock (&kvm->lock); +#ifdef CONFIG_KVM_APIC_ARCHITECTURE +if (kvm->bsp_vcpu_id == id) + kvm->bsp_vcpu = vcpu; +#endif +mutex_unlock (&kvm->lock); +mutex_init (&vcpu->mutex); vcpu->cpu =-1 ; vcpu->kvm = kvm;@@ -1760,14 +1767,12 @@ static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, u32 id)smp_wmb ();atomic_inc (&kvm->online_vcpus); -#ifdef CONFIG_KVM_APIC_ARCHITECTURE -if (kvm->bsp_vcpu_id == id) - kvm->bsp_vcpu = vcpu;
virt/kvm/kvm_main.c static int kvm_vm_ioctl_create_vcpu(struct kvm *kvm, u32 id) · line 1778
Quoted 10 lines
virt/kvm/kvm_main.c +2
-1-#endif mutex_unlock (&kvm->lock);return r; vcpu_destroy: +if (kvm->bsp_vcpu_id == id) + kvm->bsp_vcpu =NULL ;mutex_unlock (&kvm->lock);kvm_arch_vcpu_destroy (vcpu);return r;
--
Gleb.
Quoted 11 lines · Gleb Natapov
Quoted 8 lines · Marcelo Tosatti
On x86
mp_stateis initialized bykvm_arch_vcpu_init. Right nowkvm_vcpu_is_bspreturns false becausekvm->bsp_vcpuhas not been initialized, sovcpu_id== 0 ends up withmp_state==KVM_MP_STATE_UNINITIALIZED.Gleb do you see a better way to fix this?
I have two, not necessary better ways. The first one is to change
kvm_vcpu_is_bsp()to dokvm->bsp_vcpu_id==vcpu->vcpu_id.
Thats much better.
Quoted 4 lines
Another one is to understand why
mp_stateis set to runnable for bsp here at all. May be we can drop this use ofkvm_vcpu_is_bsp()sincemp_statewill be set to RUNNABLE inkvm_arch_vcpu_ioctl_set_sregs()anyway?
Testcase is kvmctl without -p, which does not do kvm_arch_vcpu_ioctl_set_sregs. I suppose that is valid? (in practice its no big deal since kvmctl can be updated, and qemu-kvm does set_sregs, but..).
KVM: use vcpu_id instead of bsp_vcpu pointer in kvm_vcpu_is_bsp
Change kvm_vcpu_is_bsp to use vcpu_id instead of bsp_vcpu pointer, which is only initialized at the end of kvm_vm_ioctl_create_vcpu.
include/linux/kvm_host.h +1 -1
@@ -569,7 +569,7 @@ static inline void kvm_irqfd_release(struct kvm *kvm) {}#ifdef CONFIG_KVM_APIC_ARCHITECTURE static inline bool kvm_vcpu_is_bsp (struct kvm_vcpu *vcpu) { -return vcpu->kvm->bsp_vcpu == vcpu; +return vcpu->kvm->bsp_vcpu_id == vcpu->vcpu_id; }#endif #endif
Quoted 20 lines · Marcelo Tosatti
Quoted 11 lines · Gleb Natapov
Quoted 8 lines · Marcelo Tosatti
On x86
mp_stateis initialized bykvm_arch_vcpu_init. Right nowkvm_vcpu_is_bspreturns false becausekvm->bsp_vcpuhas not been initialized, sovcpu_id== 0 ends up withmp_state==KVM_MP_STATE_UNINITIALIZED.Gleb do you see a better way to fix this?
I have two, not necessary better ways. The first one is to change
kvm_vcpu_is_bsp()to dokvm->bsp_vcpu_id==vcpu->vcpu_id.Thats much better.
Quoted 4 lines
Another one is to understand why
mp_stateis set to runnable for bsp here at all. May be we can drop this use ofkvm_vcpu_is_bsp()sincemp_statewill be set to RUNNABLE inkvm_arch_vcpu_ioctl_set_sregs()anyway?
Quoted 5 lines
Testcase is kvmctl without -p, which does not do
kvm_arch_vcpu_ioctl_set_sregs. I suppose that is valid? (in practice its no big deal since kvmctl can be updated, and qemu-kvm doesset_sregs, but..).
I am not sure why all those special cases are present, may be for backwards compatibility. How can we make vcpu runnable without initializing it first (setting cpuid/regs/sregd)? But the patch is good regardless.
Quoted 13 lines
KVM: use
vcpu_idinstead ofbsp_vcpupointer inkvm_vcpu_is_bspChange
kvm_vcpu_is_bspto usevcpu_idinstead ofbsp_vcpupointer, which is only initialized at the end ofkvm_vm_ioctl_create_vcpu.Signed-off-by Marcelo Tosatti <[email protected]>include/linux/kvm_host.h +0
-0
include/linux/kvm_host.h static inline void kvm_irqfd_release(struct kvm *kvm) {} · line 575
Quoted 8 lines
include/linux/kvm_host.h +1
-1@@ -569,7 +569,7 @@ static inline void kvm_irqfd_release(struct kvm *kvm) {}#ifdef CONFIG_KVM_APIC_ARCHITECTURE static inline bool kvm_vcpu_is_bsp (struct kvm_vcpu *vcpu) { -return vcpu->kvm->bsp_vcpu == vcpu; +return vcpu->kvm->bsp_vcpu_id == vcpu->vcpu_id; }#endif #endif
--
Gleb.
Avi Kivity
AppliedQuoted 4 lines · Marcelo Tosatti
KVM: use
vcpu_idinstead ofbsp_vcpupointer inkvm_vcpu_is_bspChangekvm_vcpu_is_bspto usevcpu_idinstead ofbsp_vcpupointer, which is only initialized at the end ofkvm_vm_ioctl_create_vcpu.
Applied, thanks.
Signature · 1 line
error compiling committee.c: too many arguments to function