KVM: init bsp_vcpu before kvm_arch_vcpu_init

KVM: init bsp_vcpu before kvm_arch_vcpu_init

[Marcelo Tosatti]

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?


Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>


diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 8939ffa..7225064 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -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;

Re: KVM: init bsp_vcpu before kvm_arch_vcpu_init

[Gleb Natapov]

On Tue, Jun 16, 2009 at 11:33:16AM -0300, Marcelo Tosatti wrote:
> 
> 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?
> 
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?

> 
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> 
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 8939ffa..7225064 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -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;

--
			Gleb.

Re: KVM: init bsp_vcpu before kvm_arch_vcpu_init

[Marcelo Tosatti]

On Wed, Jun 17, 2009 at 03:29:05PM +0300, Gleb Natapov wrote:
> On Tue, Jun 16, 2009 at 11:33:16AM -0300, Marcelo Tosatti wrote:
> > 
> > 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?
> > 
> 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. 

Thats much better.

> 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?

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.

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>


diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 1b48092..026ed0a 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -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

Re: KVM: init bsp_vcpu before kvm_arch_vcpu_init

[Gleb Natapov]

On Wed, Jun 17, 2009 at 10:07:59AM -0300, Marcelo Tosatti wrote:
> On Wed, Jun 17, 2009 at 03:29:05PM +0300, Gleb Natapov wrote:
> > On Tue, Jun 16, 2009 at 11:33:16AM -0300, Marcelo Tosatti wrote:
> > > 
> > > 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?
> > > 
> > 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. 
> 
> Thats much better.
> 
> > 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?
> 
> 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..).
> 
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.

> 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.
> 
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> 
> 
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 1b48092..026ed0a 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -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.

Re: KVM: init bsp_vcpu before kvm_arch_vcpu_init

[Avi Kivity]

On 06/17/2009 04:07 PM, Marcelo Tosatti wrote:
> 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.
>    

Applied, thanks.

-- 
error compiling committee.c: too many arguments to function