Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1360355 > unrolled thread

[PART1 RFC v3 02/12] KVM: x86: Introducing kvm_x86_ops VM init/uninit hooks

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PART1 RFC v3 02/12] KVM: x86: Introducing kvm_x86_ops VM init/uninit hooks Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> - 2016-03-18 07:20 +0100
    Re: [PART1 RFC v3 02/12] KVM: x86: Introducing kvm_x86_ops VM  init/uninit hooks Paolo Bonzini <pbonzini@redhat.com> - 2016-03-18 11:20 +0100
      Re: [PART1 RFC v3 02/12] KVM: x86: Introducing kvm_x86_ops VM  init/uninit hooks Suravee Suthikulpanit <Suravee.Suthikulpanit@amd.com> - 2016-03-29 07:30 +0200
        Re: [PART1 RFC v3 02/12] KVM: x86: Introducing kvm_x86_ops VM  init/uninit hooks Paolo Bonzini <pbonzini@redhat.com> - 2016-03-29 12:30 +0200
          Re: [PART1 RFC v3 02/12] KVM: x86: Introducing kvm_x86_ops VM  init/uninit hooks Paolo Bonzini <pbonzini@redhat.com> - 2016-03-30 14:10 +0200

#1360355 — [PART1 RFC v3 02/12] KVM: x86: Introducing kvm_x86_ops VM init/uninit hooks

Adding function pointers in struct kvm_x86_ops for processor-specific
layer to provide hooks for when KVM initialize and un-initialize VM.

Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
---
 arch/x86/include/asm/kvm_host.h |  3 +++
 arch/x86/kvm/x86.c              | 10 +++++++++-
 virt/kvm/kvm_main.c             |  8 ++++----
 3 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 44adbb8..4b0dd0f 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -828,6 +828,9 @@ struct kvm_x86_ops {
 	bool (*cpu_has_high_real_mode_segbase)(void);
 	void (*cpuid_update)(struct kvm_vcpu *vcpu);
 
+	int (*vm_init)(struct kvm *kvm);
+	void (*vm_uninit)(struct kvm *kvm);
+
 	/* Create, but do not attach this VCPU */
 	struct kvm_vcpu *(*vcpu_create)(struct kvm *kvm, unsigned id);
 	void (*vcpu_free)(struct kvm_vcpu *vcpu);
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 429c3f5..4d2961d 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -7700,6 +7700,8 @@ void kvm_arch_sched_in(struct kvm_vcpu *vcpu, int cpu)
 
 int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
 {
+	int ret = 0;
+
 	if (type)
 		return -EINVAL;
 
@@ -7724,7 +7726,10 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
 	INIT_DELAYED_WORK(&kvm->arch.kvmclock_update_work, kvmclock_update_fn);
 	INIT_DELAYED_WORK(&kvm->arch.kvmclock_sync_work, kvmclock_sync_fn);
 
-	return 0;
+	if (kvm_x86_ops->vm_init)
+		ret = kvm_x86_ops->vm_init(kvm);
+
+	return ret;
 }
 
 static void kvm_unload_vcpu_mmu(struct kvm_vcpu *vcpu)
@@ -7751,6 +7756,9 @@ static void kvm_free_vcpus(struct kvm *kvm)
 	kvm_for_each_vcpu(i, vcpu, kvm)
 		kvm_arch_vcpu_free(vcpu);
 
+	if (kvm_x86_ops->vm_uninit)
+		kvm_x86_ops->vm_uninit(kvm);
+
 	mutex_lock(&kvm->lock);
 	for (i = 0; i < atomic_read(&kvm->online_vcpus); i++)
 		kvm->vcpus[i] = NULL;
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 1ca0258..5460325 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -536,10 +536,6 @@ static struct kvm *kvm_create_vm(unsigned long type)
 	if (!kvm)
 		return ERR_PTR(-ENOMEM);
 
-	r = kvm_arch_init_vm(kvm, type);
-	if (r)
-		goto out_err_no_disable;
-
 	r = hardware_enable_all();
 	if (r)
 		goto out_err_no_disable;
@@ -578,6 +574,10 @@ static struct kvm *kvm_create_vm(unsigned long type)
 	atomic_set(&kvm->users_count, 1);
 	INIT_LIST_HEAD(&kvm->devices);
 
+	r = kvm_arch_init_vm(kvm, type);
+	if (r)
+		goto out_err;
+
 	r = kvm_init_mmu_notifier(kvm);
 	if (r)
 		goto out_err;
-- 
1.9.1

[toc] | [next] | [standalone]

#1360492 — Re: [PART1 RFC v3 02/12] KVM: x86: Introducing kvm_x86_ops VM init/uninit hooks

On 18/03/2016 07:09, Suravee Suthikulpanit wrote:
> Adding function pointers in struct kvm_x86_ops for processor-specific
> layer to provide hooks for when KVM initialize and un-initialize VM.

This is not the only thing your patch is doing, and the "other" change
definitely needs a lot more explanation about why you did it and how you
audited the code to ensure that it is safe.

Paolo

> Signed-off-by: Suravee Suthikulpanit <suravee.suthikulpanit@amd.com>
> ---
>  arch/x86/include/asm/kvm_host.h |  3 +++
>  arch/x86/kvm/x86.c              | 10 +++++++++-
>  virt/kvm/kvm_main.c             |  8 ++++----
>  3 files changed, 16 insertions(+), 5 deletions(-)
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 44adbb8..4b0dd0f 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -828,6 +828,9 @@ struct kvm_x86_ops {
>  	bool (*cpu_has_high_real_mode_segbase)(void);
>  	void (*cpuid_update)(struct kvm_vcpu *vcpu);
>  
> +	int (*vm_init)(struct kvm *kvm);
> +	void (*vm_uninit)(struct kvm *kvm);
> +
>  	/* Create, but do not attach this VCPU */
>  	struct kvm_vcpu *(*vcpu_create)(struct kvm *kvm, unsigned id);
>  	void (*vcpu_free)(struct kvm_vcpu *vcpu);
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 429c3f5..4d2961d 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -7700,6 +7700,8 @@ void kvm_arch_sched_in(struct kvm_vcpu *vcpu, int cpu)
>  
>  int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
>  {
> +	int ret = 0;
> +
>  	if (type)
>  		return -EINVAL;
>  
> @@ -7724,7 +7726,10 @@ int kvm_arch_init_vm(struct kvm *kvm, unsigned long type)
>  	INIT_DELAYED_WORK(&kvm->arch.kvmclock_update_work, kvmclock_update_fn);
>  	INIT_DELAYED_WORK(&kvm->arch.kvmclock_sync_work, kvmclock_sync_fn);
>  
> -	return 0;
> +	if (kvm_x86_ops->vm_init)
> +		ret = kvm_x86_ops->vm_init(kvm);
> +
> +	return ret;
>  }
>  
>  static void kvm_unload_vcpu_mmu(struct kvm_vcpu *vcpu)
> @@ -7751,6 +7756,9 @@ static void kvm_free_vcpus(struct kvm *kvm)
>  	kvm_for_each_vcpu(i, vcpu, kvm)
>  		kvm_arch_vcpu_free(vcpu);
>  
> +	if (kvm_x86_ops->vm_uninit)
> +		kvm_x86_ops->vm_uninit(kvm);
> +
>  	mutex_lock(&kvm->lock);
>  	for (i = 0; i < atomic_read(&kvm->online_vcpus); i++)
>  		kvm->vcpus[i] = NULL;
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 1ca0258..5460325 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -536,10 +536,6 @@ static struct kvm *kvm_create_vm(unsigned long type)
>  	if (!kvm)
>  		return ERR_PTR(-ENOMEM);
>  
> -	r = kvm_arch_init_vm(kvm, type);
> -	if (r)
> -		goto out_err_no_disable;
> -
>  	r = hardware_enable_all();
>  	if (r)
>  		goto out_err_no_disable;
> @@ -578,6 +574,10 @@ static struct kvm *kvm_create_vm(unsigned long type)
>  	atomic_set(&kvm->users_count, 1);
>  	INIT_LIST_HEAD(&kvm->devices);
>  
> +	r = kvm_arch_init_vm(kvm, type);
> +	if (r)
> +		goto out_err;
> +
>  	r = kvm_init_mmu_notifier(kvm);
>  	if (r)
>  		goto out_err;
> 

[toc] | [prev] | [next] | [standalone]

#1365859 — Re: [PART1 RFC v3 02/12] KVM: x86: Introducing kvm_x86_ops VM init/uninit hooks

Hi Paolo,

On 3/18/16 17:11, Paolo Bonzini wrote:
>
> On 18/03/2016 07:09, Suravee Suthikulpanit wrote:
>> >Adding function pointers in struct kvm_x86_ops for processor-specific
>> >layer to provide hooks for when KVM initialize and un-initialize VM.
> This is not the only thing your patch is doing, and the "other" change
> definitely needs a lot more explanation about why you did it and how you
> audited the code to ensure that it is safe.
>
> Paolo
>

Sorry, for not mentioning this earlier. I am moving the 
kvm_arch_init_vm() call mainly to go after mutex_init(&kvm->slots_lock) 
since I am calling the x86_set_memory_region() (which uses slots_lock) 
in the vm_init() hooks (for AVIC initialization).

Lemme re-check if this would be safe for other code.

Thanks,
Suravee

[toc] | [prev] | [next] | [standalone]

#1366112 — Re: [PART1 RFC v3 02/12] KVM: x86: Introducing kvm_x86_ops VM init/uninit hooks

On 29/03/2016 07:27, Suravee Suthikulpanit wrote:
>>
>>> >Adding function pointers in struct kvm_x86_ops for processor-specific
>>> >layer to provide hooks for when KVM initialize and un-initialize VM.
>> This is not the only thing your patch is doing, and the "other" change
>> definitely needs a lot more explanation about why you did it and how you
>> audited the code to ensure that it is safe.
>>
>> Paolo
>>
> 
> Sorry, for not mentioning this earlier. I am moving the
> kvm_arch_init_vm() call mainly to go after mutex_init(&kvm->slots_lock)
> since I am calling the x86_set_memory_region() (which uses slots_lock)
> in the vm_init() hooks (for AVIC initialization).
> 
> Lemme re-check if this would be safe for other code.

No problem.  In the meanwhile a patch got in ("KVM: fix spin_lock_init
order on x86") that should help you.

Thanks,

Paolo

[toc] | [prev] | [next] | [standalone]

#1367147 — Re: [PART1 RFC v3 02/12] KVM: x86: Introducing kvm_x86_ops VM init/uninit hooks

On 30/03/2016 12:00, Suravee Suthikulpanit wrote:
> Hi Paolo,
> 
> On 3/29/16 18:47, Suravee Suthikulpanit wrote:
>> Hi,
>>
>> On 03/29/2016 05:21 PM, Paolo Bonzini wrote:
>>>
>>>
>>> On 29/03/2016 07:27, Suravee Suthikulpanit wrote:
>>>>>
>>>>>>> Adding function pointers in struct kvm_x86_ops for
>>>>>>> processor-specific
>>>>>>> layer to provide hooks for when KVM initialize and un-initialize VM.
>>>>> This is not the only thing your patch is doing, and the "other" change
>>>>> definitely needs a lot more explanation about why you did it and how
>>>>> you
>>>>> audited the code to ensure that it is safe.
>>>>>
>>>>> Paolo
>>>>>
>>>>
>>>> Sorry, for not mentioning this earlier. I am moving the
>>>> kvm_arch_init_vm() call mainly to go after mutex_init(&kvm->slots_lock)
>>>> since I am calling the x86_set_memory_region() (which uses slots_lock)
>>>> in the vm_init() hooks (for AVIC initialization).
>>>>
>>>> Lemme re-check if this would be safe for other code.
>>>
>>> No problem.  In the meanwhile a patch got in ("KVM: fix spin_lock_init
>>> order on x86") that should help you.
>>>
>>> Thanks,
>>>
>>> Paolo
>>>
>>
>> Right.... that's just what I need :) I'll re-base to the latest tip then.
> 
> Actually, in the file virt/kvm/kvm_main.c, I still need to move the
> kvm_arch_init_vm() to some place after the call to kvm_alloc_memslots()
> since I am calling x86_set_memory_region() in the vm_init hook.
> 
>         r = -ENOMEM;
>         for (i = 0; i < KVM_ADDRESS_SPACE_NUM; i++) {
>                 kvm->memslots[i] = kvm_alloc_memslots();
>                 if (!kvm->memslots[i])
>                         goto out_err_no_srcu;
>         }
> 
>         if (init_srcu_struct(&kvm->srcu))
>                 goto out_err_no_srcu;
>         if (init_srcu_struct(&kvm->irq_srcu))
>                 goto out_err_no_irq_srcu;
>         for (i = 0; i < KVM_NR_BUSES; i++) {
>                 kvm->buses[i] = kzalloc(sizeof(struct kvm_io_bus),
>                                         GFP_KERNEL);
>                 if (!kvm->buses[i])
>                         goto out_err;
>         }
> //HERE
>         r = kvm_arch_init_vm(kvm, type);
>         if (r)
>                 goto out_err;
> 
> Do you think that would be a problem?

Can you delay that to after the creation of the first VCPU?

Allocating AVIC data structures is not required if userspace has not
called KVM_CREATE_IRQCHIP or enabled KVM_CAP_SPLIT_IRQCHIP.

Paolo

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web