[PATCH v2] KVM: Clean up vm creation and release

[PATCH v2] KVM: Clean up vm creation and release

[Jan Kiszka]

IA64 support forces us to abstract the allocation of the kvm structure.
But instead of mixing this up with arch-specific initialization and
doing the same on destruction, split both steps. This allows to move
generic destruction calls into generic code.

It also fixes error clean-up on failures of kvm_create_vm for IA64.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---

Changes in v2:
 - Fixed s390 conversion and added linux/slab.h
   as remarked by Christian (thanks!)

 arch/ia64/include/asm/kvm_host.h |    4 ++++
 arch/ia64/kvm/kvm-ia64.c         |   28 +++++++---------------------
 arch/powerpc/kvm/powerpc.c       |   20 +++-----------------
 arch/s390/kvm/kvm-s390.c         |   23 ++++++-----------------
 arch/x86/kvm/x86.c               |   12 ++----------
 include/linux/kvm_host.h         |   15 ++++++++++++++-
 virt/kvm/kvm_main.c              |   19 +++++++++++++------
 7 files changed, 49 insertions(+), 72 deletions(-)

diff --git a/arch/ia64/include/asm/kvm_host.h b/arch/ia64/include/asm/kvm_host.h
index 2f229e5..2689ee5 100644
--- a/arch/ia64/include/asm/kvm_host.h
+++ b/arch/ia64/include/asm/kvm_host.h
@@ -590,6 +590,10 @@ int kvm_emulate_halt(struct kvm_vcpu *vcpu);
 int kvm_pal_emul(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run);
 void kvm_sal_emul(struct kvm_vcpu *vcpu);
 
+#define __KVM_HAVE_ARCH_VM_ALLOC 1
+struct kvm *kvm_arch_alloc_vm(void);
+void kvm_arch_free_vm(struct kvm *kvm);
+
 #endif /* __ASSEMBLY__*/
 
 #endif
diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
index f56a631..48a48bd 100644
--- a/arch/ia64/kvm/kvm-ia64.c
+++ b/arch/ia64/kvm/kvm-ia64.c
@@ -749,7 +749,7 @@ out:
 	return r;
 }
 
-static struct kvm *kvm_alloc_kvm(void)
+struct kvm *kvm_arch_alloc_vm(void)
 {
 
 	struct kvm *kvm;
@@ -760,7 +760,7 @@ static struct kvm *kvm_alloc_kvm(void)
 	vm_base = __get_free_pages(GFP_KERNEL, get_order(KVM_VM_DATA_SIZE));
 
 	if (!vm_base)
-		return ERR_PTR(-ENOMEM);
+		return NULL;
 
 	memset((void *)vm_base, 0, KVM_VM_DATA_SIZE);
 	kvm = (struct kvm *)(vm_base +
@@ -806,10 +806,12 @@ static void kvm_build_io_pmt(struct kvm *kvm)
 #define GUEST_PHYSICAL_RR4	0x2739
 #define VMM_INIT_RR		0x1660
 
-static void kvm_init_vm(struct kvm *kvm)
+int kvm_arch_init_vm(struct kvm *kvm)
 {
 	BUG_ON(!kvm);
 
+	kvm->arch.is_sn2 = ia64_platform_is("sn2");
+
 	kvm->arch.metaphysical_rr0 = GUEST_PHYSICAL_RR0;
 	kvm->arch.metaphysical_rr4 = GUEST_PHYSICAL_RR4;
 	kvm->arch.vmm_init_rr = VMM_INIT_RR;
@@ -823,21 +825,8 @@ static void kvm_init_vm(struct kvm *kvm)
 
 	/* Reserve bit 0 of irq_sources_bitmap for userspace irq source */
 	set_bit(KVM_USERSPACE_IRQ_SOURCE_ID, &kvm->arch.irq_sources_bitmap);
-}
-
-struct  kvm *kvm_arch_create_vm(void)
-{
-	struct kvm *kvm = kvm_alloc_kvm();
-
-	if (IS_ERR(kvm))
-		return ERR_PTR(-ENOMEM);
-
-	kvm->arch.is_sn2 = ia64_platform_is("sn2");
-
-	kvm_init_vm(kvm);
-
-	return kvm;
 
+	return 0;
 }
 
 static int kvm_vm_ioctl_get_irqchip(struct kvm *kvm,
@@ -1357,7 +1346,7 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
 	return -EINVAL;
 }
 
-static void free_kvm(struct kvm *kvm)
+void kvm_arch_free_vm(struct kvm *kvm)
 {
 	unsigned long vm_base = kvm->arch.vm_base;
 
@@ -1399,9 +1388,6 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
 #endif
 	kfree(kvm->arch.vioapic);
 	kvm_release_vm_pages(kvm);
-	kvm_free_physmem(kvm);
-	cleanup_srcu_struct(&kvm->srcu);
-	free_kvm(kvm);
 }
 
 void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
index 38f756f..ce3dd65 100644
--- a/arch/powerpc/kvm/powerpc.c
+++ b/arch/powerpc/kvm/powerpc.c
@@ -145,18 +145,12 @@ void kvm_arch_check_processor_compat(void *rtn)
 	*(int *)rtn = kvmppc_core_check_processor_compat();
 }
 
-struct kvm *kvm_arch_create_vm(void)
+int kvm_arch_init_vm(struct kvm *)
 {
-	struct kvm *kvm;
-
-	kvm = kzalloc(sizeof(struct kvm), GFP_KERNEL);
-	if (!kvm)
-		return ERR_PTR(-ENOMEM);
-
-	return kvm;
+	return 0;
 }
 
-static void kvmppc_free_vcpus(struct kvm *kvm)
+void kvm_arch_destroy_vm(struct kvm *kvm)
 {
 	unsigned int i;
 	struct kvm_vcpu *vcpu;
@@ -176,14 +170,6 @@ void kvm_arch_sync_events(struct kvm *kvm)
 {
 }
 
-void kvm_arch_destroy_vm(struct kvm *kvm)
-{
-	kvmppc_free_vcpus(kvm);
-	kvm_free_physmem(kvm);
-	cleanup_srcu_struct(&kvm->srcu);
-	kfree(kvm);
-}
-
 int kvm_dev_ioctl_check_extension(long ext)
 {
 	int r;
diff --git a/arch/s390/kvm/kvm-s390.c b/arch/s390/kvm/kvm-s390.c
index 985d825..bade533 100644
--- a/arch/s390/kvm/kvm-s390.c
+++ b/arch/s390/kvm/kvm-s390.c
@@ -164,24 +164,18 @@ long kvm_arch_vm_ioctl(struct file *filp,
 	return r;
 }
 
-struct kvm *kvm_arch_create_vm(void)
+int kvm_arch_init_vm(struct kvm *kvm)
 {
-	struct kvm *kvm;
 	int rc;
 	char debug_name[16];
 
 	rc = s390_enable_sie();
 	if (rc)
-		goto out_nokvm;
-
-	rc = -ENOMEM;
-	kvm = kzalloc(sizeof(struct kvm), GFP_KERNEL);
-	if (!kvm)
-		goto out_nokvm;
+		goto out_err;
 
 	kvm->arch.sca = (struct sca_block *) get_zeroed_page(GFP_KERNEL);
 	if (!kvm->arch.sca)
-		goto out_nosca;
+		goto out_err;
 
 	sprintf(debug_name, "kvm-%u", current->pid);
 
@@ -195,13 +189,11 @@ struct kvm *kvm_arch_create_vm(void)
 	debug_register_view(kvm->arch.dbf, &debug_sprintf_view);
 	VM_EVENT(kvm, 3, "%s", "vm created");
 
-	return kvm;
+	return 0;
 out_nodbf:
 	free_page((unsigned long)(kvm->arch.sca));
-out_nosca:
-	kfree(kvm);
-out_nokvm:
-	return ERR_PTR(rc);
+out_err:
+	return rc;
 }
 
 void kvm_arch_vcpu_destroy(struct kvm_vcpu *vcpu)
@@ -240,11 +232,8 @@ void kvm_arch_sync_events(struct kvm *kvm)
 void kvm_arch_destroy_vm(struct kvm *kvm)
 {
 	kvm_free_vcpus(kvm);
-	kvm_free_physmem(kvm);
 	free_page((unsigned long)(kvm->arch.sca));
 	debug_unregister(kvm->arch.dbf);
-	cleanup_srcu_struct(&kvm->srcu);
-	kfree(kvm);
 }
 
 /* Section: vcpu related */
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 2044302..fc29223 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5962,13 +5962,8 @@ void kvm_arch_vcpu_uninit(struct kvm_vcpu *vcpu)
 	free_page((unsigned long)vcpu->arch.pio_data);
 }
 
-struct  kvm *kvm_arch_create_vm(void)
+int kvm_arch_init_vm(struct kvm *kvm)
 {
-	struct kvm *kvm = kzalloc(sizeof(struct kvm), GFP_KERNEL);
-
-	if (!kvm)
-		return ERR_PTR(-ENOMEM);
-
 	INIT_LIST_HEAD(&kvm->arch.active_mmu_pages);
 	INIT_LIST_HEAD(&kvm->arch.assigned_dev_head);
 
@@ -5977,7 +5972,7 @@ struct  kvm *kvm_arch_create_vm(void)
 
 	spin_lock_init(&kvm->arch.tsc_write_lock);
 
-	return kvm;
+	return 0;
 }
 
 static void kvm_unload_vcpu_mmu(struct kvm_vcpu *vcpu)
@@ -6022,13 +6017,10 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
 	kfree(kvm->arch.vpic);
 	kfree(kvm->arch.vioapic);
 	kvm_free_vcpus(kvm);
-	kvm_free_physmem(kvm);
 	if (kvm->arch.apic_access_page)
 		put_page(kvm->arch.apic_access_page);
 	if (kvm->arch.ept_identity_pagetable)
 		put_page(kvm->arch.ept_identity_pagetable);
-	cleanup_srcu_struct(&kvm->srcu);
-	kfree(kvm);
 }
 
 int kvm_arch_prepare_memory_region(struct kvm *kvm,
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index bcf71c7..2d63f2c 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -16,6 +16,7 @@
 #include <linux/mm.h>
 #include <linux/preempt.h>
 #include <linux/msi.h>
+#include <linux/slab.h>
 #include <asm/signal.h>
 
 #include <linux/kvm.h>
@@ -441,7 +442,19 @@ int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu);
 
 void kvm_free_physmem(struct kvm *kvm);
 
-struct  kvm *kvm_arch_create_vm(void);
+#ifndef __KVM_HAVE_ARCH_VM_ALLOC
+static inline struct kvm *kvm_arch_alloc_vm(void)
+{
+	return kzalloc(sizeof(struct kvm), GFP_KERNEL);
+}
+
+static inline void kvm_arch_free_vm(struct kvm *kvm)
+{
+	kfree(kvm);
+}
+#endif
+
+int kvm_arch_init_vm(struct kvm *kvm);
 void kvm_arch_destroy_vm(struct kvm *kvm);
 void kvm_free_all_assigned_devices(struct kvm *kvm);
 void kvm_arch_sync_events(struct kvm *kvm);
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index f7aa0e8..339dd43 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -383,11 +383,15 @@ static int kvm_init_mmu_notifier(struct kvm *kvm)
 
 static struct kvm *kvm_create_vm(void)
 {
-	int r = 0, i;
-	struct kvm *kvm = kvm_arch_create_vm();
+	int r, i;
+	struct kvm *kvm = kvm_arch_alloc_vm();
 
-	if (IS_ERR(kvm))
-		goto out;
+	if (!kvm)
+		return ERR_PTR(-ENOMEM);
+
+	r = kvm_arch_init_vm(kvm);
+	if (r)
+		goto out_err_nodisable;
 
 	r = hardware_enable_all();
 	if (r)
@@ -427,7 +431,7 @@ static struct kvm *kvm_create_vm(void)
 	spin_lock(&kvm_lock);
 	list_add(&kvm->vm_list, &vm_list);
 	spin_unlock(&kvm_lock);
-out:
+
 	return kvm;
 
 out_err:
@@ -438,7 +442,7 @@ out_err_nodisable:
 	for (i = 0; i < KVM_NR_BUSES; i++)
 		kfree(kvm->buses[i]);
 	kfree(kvm->memslots);
-	kfree(kvm);
+	kvm_arch_free_vm(kvm);
 	return ERR_PTR(r);
 }
 
@@ -512,6 +516,9 @@ static void kvm_destroy_vm(struct kvm *kvm)
 	kvm_arch_flush_shadow(kvm);
 #endif
 	kvm_arch_destroy_vm(kvm);
+	kvm_free_physmem(kvm);
+	cleanup_srcu_struct(&kvm->srcu);
+	kvm_arch_free_vm(kvm);
 	hardware_disable_all();
 	mmdrop(mm);
 }
-- 
1.7.1

Re: [PATCH v2] KVM: Clean up vm creation and release

[Alexander Graf]

On 09.11.2010, at 17:02, Jan Kiszka wrote:

> IA64 support forces us to abstract the allocation of the kvm structure.
> But instead of mixing this up with arch-specific initialization and
> doing the same on destruction, split both steps. This allows to move
> generic destruction calls into generic code.
> 
> It also fixes error clean-up on failures of kvm_create_vm for IA64.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
> ---
> 
> Changes in v2:
> - Fixed s390 conversion and added linux/slab.h
>   as remarked by Christian (thanks!)
> 
> arch/ia64/include/asm/kvm_host.h |    4 ++++
> arch/ia64/kvm/kvm-ia64.c         |   28 +++++++---------------------
> arch/powerpc/kvm/powerpc.c       |   20 +++-----------------
> arch/s390/kvm/kvm-s390.c         |   23 ++++++-----------------
> arch/x86/kvm/x86.c               |   12 ++----------
> include/linux/kvm_host.h         |   15 ++++++++++++++-
> virt/kvm/kvm_main.c              |   19 +++++++++++++------
> 7 files changed, 49 insertions(+), 72 deletions(-)
> 
> diff --git a/arch/ia64/include/asm/kvm_host.h b/arch/ia64/include/asm/kvm_host.h
> index 2f229e5..2689ee5 100644
> --- a/arch/ia64/include/asm/kvm_host.h
> +++ b/arch/ia64/include/asm/kvm_host.h
> @@ -590,6 +590,10 @@ int kvm_emulate_halt(struct kvm_vcpu *vcpu);
> int kvm_pal_emul(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run);
> void kvm_sal_emul(struct kvm_vcpu *vcpu);
> 
> +#define __KVM_HAVE_ARCH_VM_ALLOC 1
> +struct kvm *kvm_arch_alloc_vm(void);
> +void kvm_arch_free_vm(struct kvm *kvm);
> +
> #endif /* __ASSEMBLY__*/
> 
> #endif
> diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
> index f56a631..48a48bd 100644
> --- a/arch/ia64/kvm/kvm-ia64.c
> +++ b/arch/ia64/kvm/kvm-ia64.c
> @@ -749,7 +749,7 @@ out:
> 	return r;
> }
> 
> -static struct kvm *kvm_alloc_kvm(void)
> +struct kvm *kvm_arch_alloc_vm(void)
> {
> 
> 	struct kvm *kvm;
> @@ -760,7 +760,7 @@ static struct kvm *kvm_alloc_kvm(void)
> 	vm_base = __get_free_pages(GFP_KERNEL, get_order(KVM_VM_DATA_SIZE));
> 
> 	if (!vm_base)
> -		return ERR_PTR(-ENOMEM);
> +		return NULL;
> 
> 	memset((void *)vm_base, 0, KVM_VM_DATA_SIZE);
> 	kvm = (struct kvm *)(vm_base +
> @@ -806,10 +806,12 @@ static void kvm_build_io_pmt(struct kvm *kvm)
> #define GUEST_PHYSICAL_RR4	0x2739
> #define VMM_INIT_RR		0x1660
> 
> -static void kvm_init_vm(struct kvm *kvm)
> +int kvm_arch_init_vm(struct kvm *kvm)
> {
> 	BUG_ON(!kvm);
> 
> +	kvm->arch.is_sn2 = ia64_platform_is("sn2");
> +
> 	kvm->arch.metaphysical_rr0 = GUEST_PHYSICAL_RR0;
> 	kvm->arch.metaphysical_rr4 = GUEST_PHYSICAL_RR4;
> 	kvm->arch.vmm_init_rr = VMM_INIT_RR;
> @@ -823,21 +825,8 @@ static void kvm_init_vm(struct kvm *kvm)
> 
> 	/* Reserve bit 0 of irq_sources_bitmap for userspace irq source */
> 	set_bit(KVM_USERSPACE_IRQ_SOURCE_ID, &kvm->arch.irq_sources_bitmap);
> -}
> -
> -struct  kvm *kvm_arch_create_vm(void)
> -{
> -	struct kvm *kvm = kvm_alloc_kvm();
> -
> -	if (IS_ERR(kvm))
> -		return ERR_PTR(-ENOMEM);
> -
> -	kvm->arch.is_sn2 = ia64_platform_is("sn2");
> -
> -	kvm_init_vm(kvm);
> -
> -	return kvm;
> 
> +	return 0;
> }
> 
> static int kvm_vm_ioctl_get_irqchip(struct kvm *kvm,
> @@ -1357,7 +1346,7 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
> 	return -EINVAL;
> }
> 
> -static void free_kvm(struct kvm *kvm)
> +void kvm_arch_free_vm(struct kvm *kvm)
> {
> 	unsigned long vm_base = kvm->arch.vm_base;
> 
> @@ -1399,9 +1388,6 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
> #endif
> 	kfree(kvm->arch.vioapic);
> 	kvm_release_vm_pages(kvm);
> -	kvm_free_physmem(kvm);
> -	cleanup_srcu_struct(&kvm->srcu);
> -	free_kvm(kvm);
> }
> 
> void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
> diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> index 38f756f..ce3dd65 100644
> --- a/arch/powerpc/kvm/powerpc.c
> +++ b/arch/powerpc/kvm/powerpc.c
> @@ -145,18 +145,12 @@ void kvm_arch_check_processor_compat(void *rtn)
> 	*(int *)rtn = kvmppc_core_check_processor_compat();
> }
> 
> -struct kvm *kvm_arch_create_vm(void)
> +int kvm_arch_init_vm(struct kvm *)

Eh, no. This doesn't compile :).

Apart from that, the code seems to work on ppc64.


Tested-by: Alexander Graf <agraf@suse.de>

Alex

Re: [PATCH v2] KVM: Clean up vm creation and release

[Avi Kivity]

On 11/09/2010 11:01 PM, Alexander Graf wrote:
> On 09.11.2010, at 17:02, Jan Kiszka wrote:
>
> >  IA64 support forces us to abstract the allocation of the kvm structure.
> >  But instead of mixing this up with arch-specific initialization and
> >  doing the same on destruction, split both steps. This allows to move
> >  generic destruction calls into generic code.
> >
> >  It also fixes error clean-up on failures of kvm_create_vm for IA64.
> >
> >  Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
> >  ---
> >
> >  Changes in v2:
> >  - Fixed s390 conversion and added linux/slab.h
> >    as remarked by Christian (thanks!)
> >
> >  arch/ia64/include/asm/kvm_host.h |    4 ++++
> >  arch/ia64/kvm/kvm-ia64.c         |   28 +++++++---------------------
> >  arch/powerpc/kvm/powerpc.c       |   20 +++-----------------
> >  arch/s390/kvm/kvm-s390.c         |   23 ++++++-----------------
> >  arch/x86/kvm/x86.c               |   12 ++----------
> >  include/linux/kvm_host.h         |   15 ++++++++++++++-
> >  virt/kvm/kvm_main.c              |   19 +++++++++++++------
> >  7 files changed, 49 insertions(+), 72 deletions(-)
> >
> >  diff --git a/arch/ia64/include/asm/kvm_host.h b/arch/ia64/include/asm/kvm_host.h
> >  index 2f229e5..2689ee5 100644
> >  --- a/arch/ia64/include/asm/kvm_host.h
> >  +++ b/arch/ia64/include/asm/kvm_host.h
> >  @@ -590,6 +590,10 @@ int kvm_emulate_halt(struct kvm_vcpu *vcpu);
> >  int kvm_pal_emul(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run);
> >  void kvm_sal_emul(struct kvm_vcpu *vcpu);
> >
> >  +#define __KVM_HAVE_ARCH_VM_ALLOC 1
> >  +struct kvm *kvm_arch_alloc_vm(void);
> >  +void kvm_arch_free_vm(struct kvm *kvm);
> >  +
> >  #endif /* __ASSEMBLY__*/
> >
> >  #endif
> >  diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
> >  index f56a631..48a48bd 100644
> >  --- a/arch/ia64/kvm/kvm-ia64.c
> >  +++ b/arch/ia64/kvm/kvm-ia64.c
> >  @@ -749,7 +749,7 @@ out:
> >  	return r;
> >  }
> >
> >  -static struct kvm *kvm_alloc_kvm(void)
> >  +struct kvm *kvm_arch_alloc_vm(void)
> >  {
> >
> >  	struct kvm *kvm;
> >  @@ -760,7 +760,7 @@ static struct kvm *kvm_alloc_kvm(void)
> >  	vm_base = __get_free_pages(GFP_KERNEL, get_order(KVM_VM_DATA_SIZE));
> >
> >  	if (!vm_base)
> >  -		return ERR_PTR(-ENOMEM);
> >  +		return NULL;
> >
> >  	memset((void *)vm_base, 0, KVM_VM_DATA_SIZE);
> >  	kvm = (struct kvm *)(vm_base +
> >  @@ -806,10 +806,12 @@ static void kvm_build_io_pmt(struct kvm *kvm)
> >  #define GUEST_PHYSICAL_RR4	0x2739
> >  #define VMM_INIT_RR		0x1660
> >
> >  -static void kvm_init_vm(struct kvm *kvm)
> >  +int kvm_arch_init_vm(struct kvm *kvm)
> >  {
> >  	BUG_ON(!kvm);
> >
> >  +	kvm->arch.is_sn2 = ia64_platform_is("sn2");
> >  +
> >  	kvm->arch.metaphysical_rr0 = GUEST_PHYSICAL_RR0;
> >  	kvm->arch.metaphysical_rr4 = GUEST_PHYSICAL_RR4;
> >  	kvm->arch.vmm_init_rr = VMM_INIT_RR;
> >  @@ -823,21 +825,8 @@ static void kvm_init_vm(struct kvm *kvm)
> >
> >  	/* Reserve bit 0 of irq_sources_bitmap for userspace irq source */
> >  	set_bit(KVM_USERSPACE_IRQ_SOURCE_ID,&kvm->arch.irq_sources_bitmap);
> >  -}
> >  -
> >  -struct  kvm *kvm_arch_create_vm(void)
> >  -{
> >  -	struct kvm *kvm = kvm_alloc_kvm();
> >  -
> >  -	if (IS_ERR(kvm))
> >  -		return ERR_PTR(-ENOMEM);
> >  -
> >  -	kvm->arch.is_sn2 = ia64_platform_is("sn2");
> >  -
> >  -	kvm_init_vm(kvm);
> >  -
> >  -	return kvm;
> >
> >  +	return 0;
> >  }
> >
> >  static int kvm_vm_ioctl_get_irqchip(struct kvm *kvm,
> >  @@ -1357,7 +1346,7 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
> >  	return -EINVAL;
> >  }
> >
> >  -static void free_kvm(struct kvm *kvm)
> >  +void kvm_arch_free_vm(struct kvm *kvm)
> >  {
> >  	unsigned long vm_base = kvm->arch.vm_base;
> >
> >  @@ -1399,9 +1388,6 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
> >  #endif
> >  	kfree(kvm->arch.vioapic);
> >  	kvm_release_vm_pages(kvm);
> >  -	kvm_free_physmem(kvm);
> >  -	cleanup_srcu_struct(&kvm->srcu);
> >  -	free_kvm(kvm);
> >  }
> >
> >  void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
> >  diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> >  index 38f756f..ce3dd65 100644
> >  --- a/arch/powerpc/kvm/powerpc.c
> >  +++ b/arch/powerpc/kvm/powerpc.c
> >  @@ -145,18 +145,12 @@ void kvm_arch_check_processor_compat(void *rtn)
> >  	*(int *)rtn = kvmppc_core_check_processor_compat();
> >  }
> >
> >  -struct kvm *kvm_arch_create_vm(void)
> >  +int kvm_arch_init_vm(struct kvm *)
>
> Eh, no. This doesn't compile :).

What's the problem? lack of a formal argument?


> Apart from that, the code seems to work on ppc64.
>
>
> Tested-by: Alexander Graf<agraf@suse.de>
>

Seems my ppc builder has died, so thanks for the test.

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

Re: [PATCH v2] KVM: Clean up vm creation and release

[Jan Kiszka]

Am 10.11.2010 10:17, Avi Kivity wrote:
> On 11/09/2010 11:01 PM, Alexander Graf wrote:
>> On 09.11.2010, at 17:02, Jan Kiszka wrote:
>>
>>>  IA64 support forces us to abstract the allocation of the kvm structure.
>>>  But instead of mixing this up with arch-specific initialization and
>>>  doing the same on destruction, split both steps. This allows to move
>>>  generic destruction calls into generic code.
>>>
>>>  It also fixes error clean-up on failures of kvm_create_vm for IA64.
>>>
>>>  Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
>>>  ---
>>>
>>>  Changes in v2:
>>>  - Fixed s390 conversion and added linux/slab.h
>>>    as remarked by Christian (thanks!)
>>>
>>>  arch/ia64/include/asm/kvm_host.h |    4 ++++
>>>  arch/ia64/kvm/kvm-ia64.c         |   28 +++++++---------------------
>>>  arch/powerpc/kvm/powerpc.c       |   20 +++-----------------
>>>  arch/s390/kvm/kvm-s390.c         |   23 ++++++-----------------
>>>  arch/x86/kvm/x86.c               |   12 ++----------
>>>  include/linux/kvm_host.h         |   15 ++++++++++++++-
>>>  virt/kvm/kvm_main.c              |   19 +++++++++++++------
>>>  7 files changed, 49 insertions(+), 72 deletions(-)
>>>
>>>  diff --git a/arch/ia64/include/asm/kvm_host.h b/arch/ia64/include/asm/kvm_host.h
>>>  index 2f229e5..2689ee5 100644
>>>  --- a/arch/ia64/include/asm/kvm_host.h
>>>  +++ b/arch/ia64/include/asm/kvm_host.h
>>>  @@ -590,6 +590,10 @@ int kvm_emulate_halt(struct kvm_vcpu *vcpu);
>>>  int kvm_pal_emul(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run);
>>>  void kvm_sal_emul(struct kvm_vcpu *vcpu);
>>>
>>>  +#define __KVM_HAVE_ARCH_VM_ALLOC 1
>>>  +struct kvm *kvm_arch_alloc_vm(void);
>>>  +void kvm_arch_free_vm(struct kvm *kvm);
>>>  +
>>>  #endif /* __ASSEMBLY__*/
>>>
>>>  #endif
>>>  diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
>>>  index f56a631..48a48bd 100644
>>>  --- a/arch/ia64/kvm/kvm-ia64.c
>>>  +++ b/arch/ia64/kvm/kvm-ia64.c
>>>  @@ -749,7 +749,7 @@ out:
>>>  	return r;
>>>  }
>>>
>>>  -static struct kvm *kvm_alloc_kvm(void)
>>>  +struct kvm *kvm_arch_alloc_vm(void)
>>>  {
>>>
>>>  	struct kvm *kvm;
>>>  @@ -760,7 +760,7 @@ static struct kvm *kvm_alloc_kvm(void)
>>>  	vm_base = __get_free_pages(GFP_KERNEL, get_order(KVM_VM_DATA_SIZE));
>>>
>>>  	if (!vm_base)
>>>  -		return ERR_PTR(-ENOMEM);
>>>  +		return NULL;
>>>
>>>  	memset((void *)vm_base, 0, KVM_VM_DATA_SIZE);
>>>  	kvm = (struct kvm *)(vm_base +
>>>  @@ -806,10 +806,12 @@ static void kvm_build_io_pmt(struct kvm *kvm)
>>>  #define GUEST_PHYSICAL_RR4	0x2739
>>>  #define VMM_INIT_RR		0x1660
>>>
>>>  -static void kvm_init_vm(struct kvm *kvm)
>>>  +int kvm_arch_init_vm(struct kvm *kvm)
>>>  {
>>>  	BUG_ON(!kvm);
>>>
>>>  +	kvm->arch.is_sn2 = ia64_platform_is("sn2");
>>>  +
>>>  	kvm->arch.metaphysical_rr0 = GUEST_PHYSICAL_RR0;
>>>  	kvm->arch.metaphysical_rr4 = GUEST_PHYSICAL_RR4;
>>>  	kvm->arch.vmm_init_rr = VMM_INIT_RR;
>>>  @@ -823,21 +825,8 @@ static void kvm_init_vm(struct kvm *kvm)
>>>
>>>  	/* Reserve bit 0 of irq_sources_bitmap for userspace irq source */
>>>  	set_bit(KVM_USERSPACE_IRQ_SOURCE_ID,&kvm->arch.irq_sources_bitmap);
>>>  -}
>>>  -
>>>  -struct  kvm *kvm_arch_create_vm(void)
>>>  -{
>>>  -	struct kvm *kvm = kvm_alloc_kvm();
>>>  -
>>>  -	if (IS_ERR(kvm))
>>>  -		return ERR_PTR(-ENOMEM);
>>>  -
>>>  -	kvm->arch.is_sn2 = ia64_platform_is("sn2");
>>>  -
>>>  -	kvm_init_vm(kvm);
>>>  -
>>>  -	return kvm;
>>>
>>>  +	return 0;
>>>  }
>>>
>>>  static int kvm_vm_ioctl_get_irqchip(struct kvm *kvm,
>>>  @@ -1357,7 +1346,7 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>>>  	return -EINVAL;
>>>  }
>>>
>>>  -static void free_kvm(struct kvm *kvm)
>>>  +void kvm_arch_free_vm(struct kvm *kvm)
>>>  {
>>>  	unsigned long vm_base = kvm->arch.vm_base;
>>>
>>>  @@ -1399,9 +1388,6 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
>>>  #endif
>>>  	kfree(kvm->arch.vioapic);
>>>  	kvm_release_vm_pages(kvm);
>>>  -	kvm_free_physmem(kvm);
>>>  -	cleanup_srcu_struct(&kvm->srcu);
>>>  -	free_kvm(kvm);
>>>  }
>>>
>>>  void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>>>  diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
>>>  index 38f756f..ce3dd65 100644
>>>  --- a/arch/powerpc/kvm/powerpc.c
>>>  +++ b/arch/powerpc/kvm/powerpc.c
>>>  @@ -145,18 +145,12 @@ void kvm_arch_check_processor_compat(void *rtn)
>>>  	*(int *)rtn = kvmppc_core_check_processor_compat();
>>>  }
>>>
>>>  -struct kvm *kvm_arch_create_vm(void)
>>>  +int kvm_arch_init_vm(struct kvm *)
>>
>> Eh, no. This doesn't compile :).
> 
> What's the problem? lack of a formal argument?

The unnamed argument is referenced as 'kvm' in this function. Will you
fix this up on merge or should I resend?

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH v2] KVM: Clean up vm creation and release

[Avi Kivity]

On 11/10/2010 11:30 AM, Jan Kiszka wrote:
> Am 10.11.2010 10:17, Avi Kivity wrote:
> >  On 11/09/2010 11:01 PM, Alexander Graf wrote:
> >>  On 09.11.2010, at 17:02, Jan Kiszka wrote:
> >>
> >>>   IA64 support forces us to abstract the allocation of the kvm structure.
> >>>   But instead of mixing this up with arch-specific initialization and
> >>>   doing the same on destruction, split both steps. This allows to move
> >>>   generic destruction calls into generic code.
> >>>
> >>>   It also fixes error clean-up on failures of kvm_create_vm for IA64.
> >>>
> >>>   Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
> >>>   ---
> >>>
> >>>   Changes in v2:
> >>>   - Fixed s390 conversion and added linux/slab.h
> >>>     as remarked by Christian (thanks!)
> >>>
> >>>   arch/ia64/include/asm/kvm_host.h |    4 ++++
> >>>   arch/ia64/kvm/kvm-ia64.c         |   28 +++++++---------------------
> >>>   arch/powerpc/kvm/powerpc.c       |   20 +++-----------------
> >>>   arch/s390/kvm/kvm-s390.c         |   23 ++++++-----------------
> >>>   arch/x86/kvm/x86.c               |   12 ++----------
> >>>   include/linux/kvm_host.h         |   15 ++++++++++++++-
> >>>   virt/kvm/kvm_main.c              |   19 +++++++++++++------
> >>>   7 files changed, 49 insertions(+), 72 deletions(-)
> >>>
> >>>   diff --git a/arch/ia64/include/asm/kvm_host.h b/arch/ia64/include/asm/kvm_host.h
> >>>   index 2f229e5..2689ee5 100644
> >>>   --- a/arch/ia64/include/asm/kvm_host.h
> >>>   +++ b/arch/ia64/include/asm/kvm_host.h
> >>>   @@ -590,6 +590,10 @@ int kvm_emulate_halt(struct kvm_vcpu *vcpu);
> >>>   int kvm_pal_emul(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run);
> >>>   void kvm_sal_emul(struct kvm_vcpu *vcpu);
> >>>
> >>>   +#define __KVM_HAVE_ARCH_VM_ALLOC 1
> >>>   +struct kvm *kvm_arch_alloc_vm(void);
> >>>   +void kvm_arch_free_vm(struct kvm *kvm);
> >>>   +
> >>>   #endif /* __ASSEMBLY__*/
> >>>
> >>>   #endif
> >>>   diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
> >>>   index f56a631..48a48bd 100644
> >>>   --- a/arch/ia64/kvm/kvm-ia64.c
> >>>   +++ b/arch/ia64/kvm/kvm-ia64.c
> >>>   @@ -749,7 +749,7 @@ out:
> >>>   	return r;
> >>>   }
> >>>
> >>>   -static struct kvm *kvm_alloc_kvm(void)
> >>>   +struct kvm *kvm_arch_alloc_vm(void)
> >>>   {
> >>>
> >>>   	struct kvm *kvm;
> >>>   @@ -760,7 +760,7 @@ static struct kvm *kvm_alloc_kvm(void)
> >>>   	vm_base = __get_free_pages(GFP_KERNEL, get_order(KVM_VM_DATA_SIZE));
> >>>
> >>>   	if (!vm_base)
> >>>   -		return ERR_PTR(-ENOMEM);
> >>>   +		return NULL;
> >>>
> >>>   	memset((void *)vm_base, 0, KVM_VM_DATA_SIZE);
> >>>   	kvm = (struct kvm *)(vm_base +
> >>>   @@ -806,10 +806,12 @@ static void kvm_build_io_pmt(struct kvm *kvm)
> >>>   #define GUEST_PHYSICAL_RR4	0x2739
> >>>   #define VMM_INIT_RR		0x1660
> >>>
> >>>   -static void kvm_init_vm(struct kvm *kvm)
> >>>   +int kvm_arch_init_vm(struct kvm *kvm)
> >>>   {
> >>>   	BUG_ON(!kvm);
> >>>
> >>>   +	kvm->arch.is_sn2 = ia64_platform_is("sn2");
> >>>   +
> >>>   	kvm->arch.metaphysical_rr0 = GUEST_PHYSICAL_RR0;
> >>>   	kvm->arch.metaphysical_rr4 = GUEST_PHYSICAL_RR4;
> >>>   	kvm->arch.vmm_init_rr = VMM_INIT_RR;
> >>>   @@ -823,21 +825,8 @@ static void kvm_init_vm(struct kvm *kvm)
> >>>
> >>>   	/* Reserve bit 0 of irq_sources_bitmap for userspace irq source */
> >>>   	set_bit(KVM_USERSPACE_IRQ_SOURCE_ID,&kvm->arch.irq_sources_bitmap);
> >>>   -}
> >>>   -
> >>>   -struct  kvm *kvm_arch_create_vm(void)
> >>>   -{
> >>>   -	struct kvm *kvm = kvm_alloc_kvm();
> >>>   -
> >>>   -	if (IS_ERR(kvm))
> >>>   -		return ERR_PTR(-ENOMEM);
> >>>   -
> >>>   -	kvm->arch.is_sn2 = ia64_platform_is("sn2");
> >>>   -
> >>>   -	kvm_init_vm(kvm);
> >>>   -
> >>>   -	return kvm;
> >>>
> >>>   +	return 0;
> >>>   }
> >>>
> >>>   static int kvm_vm_ioctl_get_irqchip(struct kvm *kvm,
> >>>   @@ -1357,7 +1346,7 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
> >>>   	return -EINVAL;
> >>>   }
> >>>
> >>>   -static void free_kvm(struct kvm *kvm)
> >>>   +void kvm_arch_free_vm(struct kvm *kvm)
> >>>   {
> >>>   	unsigned long vm_base = kvm->arch.vm_base;
> >>>
> >>>   @@ -1399,9 +1388,6 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
> >>>   #endif
> >>>   	kfree(kvm->arch.vioapic);
> >>>   	kvm_release_vm_pages(kvm);
> >>>   -	kvm_free_physmem(kvm);
> >>>   -	cleanup_srcu_struct(&kvm->srcu);
> >>>   -	free_kvm(kvm);
> >>>   }
> >>>
> >>>   void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
> >>>   diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> >>>   index 38f756f..ce3dd65 100644
> >>>   --- a/arch/powerpc/kvm/powerpc.c
> >>>   +++ b/arch/powerpc/kvm/powerpc.c
> >>>   @@ -145,18 +145,12 @@ void kvm_arch_check_processor_compat(void *rtn)
> >>>   	*(int *)rtn = kvmppc_core_check_processor_compat();
> >>>   }
> >>>
> >>>   -struct kvm *kvm_arch_create_vm(void)
> >>>   +int kvm_arch_init_vm(struct kvm *)
> >>
> >>  Eh, no. This doesn't compile :).
> >
> >  What's the problem? lack of a formal argument?
>
> The unnamed argument is referenced as 'kvm' in this function.

It isn't referenced:

> int kvm_arch_init_vm(struct kvm *)
> {
>     return 0;
> }


> Will you
> fix this up on merge or should I resend?

I'll fix it, as soon as I understand it.

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

Re: [PATCH v2] KVM: Clean up vm creation and release

[Jan Kiszka]

Am 10.11.2010 10:45, Avi Kivity wrote:
> On 11/10/2010 11:30 AM, Jan Kiszka wrote:
>> Am 10.11.2010 10:17, Avi Kivity wrote:
>>>  On 11/09/2010 11:01 PM, Alexander Graf wrote:
>>>>  On 09.11.2010, at 17:02, Jan Kiszka wrote:
>>>>
>>>>>   IA64 support forces us to abstract the allocation of the kvm structure.
>>>>>   But instead of mixing this up with arch-specific initialization and
>>>>>   doing the same on destruction, split both steps. This allows to move
>>>>>   generic destruction calls into generic code.
>>>>>
>>>>>   It also fixes error clean-up on failures of kvm_create_vm for IA64.
>>>>>
>>>>>   Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
>>>>>   ---
>>>>>
>>>>>   Changes in v2:
>>>>>   - Fixed s390 conversion and added linux/slab.h
>>>>>     as remarked by Christian (thanks!)
>>>>>
>>>>>   arch/ia64/include/asm/kvm_host.h |    4 ++++
>>>>>   arch/ia64/kvm/kvm-ia64.c         |   28 +++++++---------------------
>>>>>   arch/powerpc/kvm/powerpc.c       |   20 +++-----------------
>>>>>   arch/s390/kvm/kvm-s390.c         |   23 ++++++-----------------
>>>>>   arch/x86/kvm/x86.c               |   12 ++----------
>>>>>   include/linux/kvm_host.h         |   15 ++++++++++++++-
>>>>>   virt/kvm/kvm_main.c              |   19 +++++++++++++------
>>>>>   7 files changed, 49 insertions(+), 72 deletions(-)
>>>>>
>>>>>   diff --git a/arch/ia64/include/asm/kvm_host.h b/arch/ia64/include/asm/kvm_host.h
>>>>>   index 2f229e5..2689ee5 100644
>>>>>   --- a/arch/ia64/include/asm/kvm_host.h
>>>>>   +++ b/arch/ia64/include/asm/kvm_host.h
>>>>>   @@ -590,6 +590,10 @@ int kvm_emulate_halt(struct kvm_vcpu *vcpu);
>>>>>   int kvm_pal_emul(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run);
>>>>>   void kvm_sal_emul(struct kvm_vcpu *vcpu);
>>>>>
>>>>>   +#define __KVM_HAVE_ARCH_VM_ALLOC 1
>>>>>   +struct kvm *kvm_arch_alloc_vm(void);
>>>>>   +void kvm_arch_free_vm(struct kvm *kvm);
>>>>>   +
>>>>>   #endif /* __ASSEMBLY__*/
>>>>>
>>>>>   #endif
>>>>>   diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
>>>>>   index f56a631..48a48bd 100644
>>>>>   --- a/arch/ia64/kvm/kvm-ia64.c
>>>>>   +++ b/arch/ia64/kvm/kvm-ia64.c
>>>>>   @@ -749,7 +749,7 @@ out:
>>>>>   	return r;
>>>>>   }
>>>>>
>>>>>   -static struct kvm *kvm_alloc_kvm(void)
>>>>>   +struct kvm *kvm_arch_alloc_vm(void)
>>>>>   {
>>>>>
>>>>>   	struct kvm *kvm;
>>>>>   @@ -760,7 +760,7 @@ static struct kvm *kvm_alloc_kvm(void)
>>>>>   	vm_base = __get_free_pages(GFP_KERNEL, get_order(KVM_VM_DATA_SIZE));
>>>>>
>>>>>   	if (!vm_base)
>>>>>   -		return ERR_PTR(-ENOMEM);
>>>>>   +		return NULL;
>>>>>
>>>>>   	memset((void *)vm_base, 0, KVM_VM_DATA_SIZE);
>>>>>   	kvm = (struct kvm *)(vm_base +
>>>>>   @@ -806,10 +806,12 @@ static void kvm_build_io_pmt(struct kvm *kvm)
>>>>>   #define GUEST_PHYSICAL_RR4	0x2739
>>>>>   #define VMM_INIT_RR		0x1660
>>>>>
>>>>>   -static void kvm_init_vm(struct kvm *kvm)
>>>>>   +int kvm_arch_init_vm(struct kvm *kvm)
>>>>>   {
>>>>>   	BUG_ON(!kvm);
>>>>>
>>>>>   +	kvm->arch.is_sn2 = ia64_platform_is("sn2");
>>>>>   +
>>>>>   	kvm->arch.metaphysical_rr0 = GUEST_PHYSICAL_RR0;
>>>>>   	kvm->arch.metaphysical_rr4 = GUEST_PHYSICAL_RR4;
>>>>>   	kvm->arch.vmm_init_rr = VMM_INIT_RR;
>>>>>   @@ -823,21 +825,8 @@ static void kvm_init_vm(struct kvm *kvm)
>>>>>
>>>>>   	/* Reserve bit 0 of irq_sources_bitmap for userspace irq source */
>>>>>   	set_bit(KVM_USERSPACE_IRQ_SOURCE_ID,&kvm->arch.irq_sources_bitmap);
>>>>>   -}
>>>>>   -
>>>>>   -struct  kvm *kvm_arch_create_vm(void)
>>>>>   -{
>>>>>   -	struct kvm *kvm = kvm_alloc_kvm();
>>>>>   -
>>>>>   -	if (IS_ERR(kvm))
>>>>>   -		return ERR_PTR(-ENOMEM);
>>>>>   -
>>>>>   -	kvm->arch.is_sn2 = ia64_platform_is("sn2");
>>>>>   -
>>>>>   -	kvm_init_vm(kvm);
>>>>>   -
>>>>>   -	return kvm;
>>>>>
>>>>>   +	return 0;
>>>>>   }
>>>>>
>>>>>   static int kvm_vm_ioctl_get_irqchip(struct kvm *kvm,
>>>>>   @@ -1357,7 +1346,7 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>>>>>   	return -EINVAL;
>>>>>   }
>>>>>
>>>>>   -static void free_kvm(struct kvm *kvm)
>>>>>   +void kvm_arch_free_vm(struct kvm *kvm)
>>>>>   {
>>>>>   	unsigned long vm_base = kvm->arch.vm_base;
>>>>>
>>>>>   @@ -1399,9 +1388,6 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
>>>>>   #endif
>>>>>   	kfree(kvm->arch.vioapic);
>>>>>   	kvm_release_vm_pages(kvm);
>>>>>   -	kvm_free_physmem(kvm);
>>>>>   -	cleanup_srcu_struct(&kvm->srcu);
>>>>>   -	free_kvm(kvm);
>>>>>   }
>>>>>
>>>>>   void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>>>>>   diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
>>>>>   index 38f756f..ce3dd65 100644
>>>>>   --- a/arch/powerpc/kvm/powerpc.c
>>>>>   +++ b/arch/powerpc/kvm/powerpc.c
>>>>>   @@ -145,18 +145,12 @@ void kvm_arch_check_processor_compat(void *rtn)
>>>>>   	*(int *)rtn = kvmppc_core_check_processor_compat();
>>>>>   }
>>>>>
>>>>>   -struct kvm *kvm_arch_create_vm(void)
>>>>>   +int kvm_arch_init_vm(struct kvm *)
>>>>
>>>>  Eh, no. This doesn't compile :).
>>>
>>>  What's the problem? lack of a formal argument?
>>
>> The unnamed argument is referenced as 'kvm' in this function.
> 
> It isn't referenced:
> 
>> int kvm_arch_init_vm(struct kvm *)
>> {
>>     return 0;
>> }
> 

Indeed. Anyway, this wasn't intended (even more if some compiler
versions/settings dislike it).

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [PATCH v2] KVM: Clean up vm creation and release

[Avi Kivity]

On 11/10/2010 12:18 PM, Jan Kiszka wrote:
> Am 10.11.2010 10:45, Avi Kivity wrote:
> >  On 11/10/2010 11:30 AM, Jan Kiszka wrote:
> >>  Am 10.11.2010 10:17, Avi Kivity wrote:
> >>>   On 11/09/2010 11:01 PM, Alexander Graf wrote:
> >>>>   On 09.11.2010, at 17:02, Jan Kiszka wrote:
> >>>>
> >>>>>    IA64 support forces us to abstract the allocation of the kvm structure.
> >>>>>    But instead of mixing this up with arch-specific initialization and
> >>>>>    doing the same on destruction, split both steps. This allows to move
> >>>>>    generic destruction calls into generic code.
> >>>>>
> >>>>>    It also fixes error clean-up on failures of kvm_create_vm for IA64.
> >>>>>
> >>>>>    Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
> >>>>>    ---
> >>>>>
> >>>>>    Changes in v2:
> >>>>>    - Fixed s390 conversion and added linux/slab.h
> >>>>>      as remarked by Christian (thanks!)
> >>>>>
> >>>>>    arch/ia64/include/asm/kvm_host.h |    4 ++++
> >>>>>    arch/ia64/kvm/kvm-ia64.c         |   28 +++++++---------------------
> >>>>>    arch/powerpc/kvm/powerpc.c       |   20 +++-----------------
> >>>>>    arch/s390/kvm/kvm-s390.c         |   23 ++++++-----------------
> >>>>>    arch/x86/kvm/x86.c               |   12 ++----------
> >>>>>    include/linux/kvm_host.h         |   15 ++++++++++++++-
> >>>>>    virt/kvm/kvm_main.c              |   19 +++++++++++++------
> >>>>>    7 files changed, 49 insertions(+), 72 deletions(-)
> >>>>>
> >>>>>    diff --git a/arch/ia64/include/asm/kvm_host.h b/arch/ia64/include/asm/kvm_host.h
> >>>>>    index 2f229e5..2689ee5 100644
> >>>>>    --- a/arch/ia64/include/asm/kvm_host.h
> >>>>>    +++ b/arch/ia64/include/asm/kvm_host.h
> >>>>>    @@ -590,6 +590,10 @@ int kvm_emulate_halt(struct kvm_vcpu *vcpu);
> >>>>>    int kvm_pal_emul(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run);
> >>>>>    void kvm_sal_emul(struct kvm_vcpu *vcpu);
> >>>>>
> >>>>>    +#define __KVM_HAVE_ARCH_VM_ALLOC 1
> >>>>>    +struct kvm *kvm_arch_alloc_vm(void);
> >>>>>    +void kvm_arch_free_vm(struct kvm *kvm);
> >>>>>    +
> >>>>>    #endif /* __ASSEMBLY__*/
> >>>>>
> >>>>>    #endif
> >>>>>    diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
> >>>>>    index f56a631..48a48bd 100644
> >>>>>    --- a/arch/ia64/kvm/kvm-ia64.c
> >>>>>    +++ b/arch/ia64/kvm/kvm-ia64.c
> >>>>>    @@ -749,7 +749,7 @@ out:
> >>>>>    	return r;
> >>>>>    }
> >>>>>
> >>>>>    -static struct kvm *kvm_alloc_kvm(void)
> >>>>>    +struct kvm *kvm_arch_alloc_vm(void)
> >>>>>    {
> >>>>>
> >>>>>    	struct kvm *kvm;
> >>>>>    @@ -760,7 +760,7 @@ static struct kvm *kvm_alloc_kvm(void)
> >>>>>    	vm_base = __get_free_pages(GFP_KERNEL, get_order(KVM_VM_DATA_SIZE));
> >>>>>
> >>>>>    	if (!vm_base)
> >>>>>    -		return ERR_PTR(-ENOMEM);
> >>>>>    +		return NULL;
> >>>>>
> >>>>>    	memset((void *)vm_base, 0, KVM_VM_DATA_SIZE);
> >>>>>    	kvm = (struct kvm *)(vm_base +
> >>>>>    @@ -806,10 +806,12 @@ static void kvm_build_io_pmt(struct kvm *kvm)
> >>>>>    #define GUEST_PHYSICAL_RR4	0x2739
> >>>>>    #define VMM_INIT_RR		0x1660
> >>>>>
> >>>>>    -static void kvm_init_vm(struct kvm *kvm)
> >>>>>    +int kvm_arch_init_vm(struct kvm *kvm)
> >>>>>    {
> >>>>>    	BUG_ON(!kvm);
> >>>>>
> >>>>>    +	kvm->arch.is_sn2 = ia64_platform_is("sn2");
> >>>>>    +
> >>>>>    	kvm->arch.metaphysical_rr0 = GUEST_PHYSICAL_RR0;
> >>>>>    	kvm->arch.metaphysical_rr4 = GUEST_PHYSICAL_RR4;
> >>>>>    	kvm->arch.vmm_init_rr = VMM_INIT_RR;
> >>>>>    @@ -823,21 +825,8 @@ static void kvm_init_vm(struct kvm *kvm)
> >>>>>
> >>>>>    	/* Reserve bit 0 of irq_sources_bitmap for userspace irq source */
> >>>>>    	set_bit(KVM_USERSPACE_IRQ_SOURCE_ID,&kvm->arch.irq_sources_bitmap);
> >>>>>    -}
> >>>>>    -
> >>>>>    -struct  kvm *kvm_arch_create_vm(void)
> >>>>>    -{
> >>>>>    -	struct kvm *kvm = kvm_alloc_kvm();
> >>>>>    -
> >>>>>    -	if (IS_ERR(kvm))
> >>>>>    -		return ERR_PTR(-ENOMEM);
> >>>>>    -
> >>>>>    -	kvm->arch.is_sn2 = ia64_platform_is("sn2");
> >>>>>    -
> >>>>>    -	kvm_init_vm(kvm);
> >>>>>    -
> >>>>>    -	return kvm;
> >>>>>
> >>>>>    +	return 0;
> >>>>>    }
> >>>>>
> >>>>>    static int kvm_vm_ioctl_get_irqchip(struct kvm *kvm,
> >>>>>    @@ -1357,7 +1346,7 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
> >>>>>    	return -EINVAL;
> >>>>>    }
> >>>>>
> >>>>>    -static void free_kvm(struct kvm *kvm)
> >>>>>    +void kvm_arch_free_vm(struct kvm *kvm)
> >>>>>    {
> >>>>>    	unsigned long vm_base = kvm->arch.vm_base;
> >>>>>
> >>>>>    @@ -1399,9 +1388,6 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
> >>>>>    #endif
> >>>>>    	kfree(kvm->arch.vioapic);
> >>>>>    	kvm_release_vm_pages(kvm);
> >>>>>    -	kvm_free_physmem(kvm);
> >>>>>    -	cleanup_srcu_struct(&kvm->srcu);
> >>>>>    -	free_kvm(kvm);
> >>>>>    }
> >>>>>
> >>>>>    void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
> >>>>>    diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
> >>>>>    index 38f756f..ce3dd65 100644
> >>>>>    --- a/arch/powerpc/kvm/powerpc.c
> >>>>>    +++ b/arch/powerpc/kvm/powerpc.c
> >>>>>    @@ -145,18 +145,12 @@ void kvm_arch_check_processor_compat(void *rtn)
> >>>>>    	*(int *)rtn = kvmppc_core_check_processor_compat();
> >>>>>    }
> >>>>>
> >>>>>    -struct kvm *kvm_arch_create_vm(void)
> >>>>>    +int kvm_arch_init_vm(struct kvm *)
> >>>>
> >>>>   Eh, no. This doesn't compile :).
> >>>
> >>>   What's the problem? lack of a formal argument?
> >>
> >>  The unnamed argument is referenced as 'kvm' in this function.
> >
> >  It isn't referenced:
> >
> >>  int kvm_arch_init_vm(struct kvm *)
> >>  {
> >>      return 0;
> >>  }
> >
>
> Indeed. Anyway, this wasn't intended (even more if some compiler
> versions/settings dislike it).

Ok.  I'll add the name and hope that this was the problem.

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

Re: [PATCH v2] KVM: Clean up vm creation and release

[Alexander Graf]

On 10.11.2010, at 11:21, Avi Kivity wrote:

> On 11/10/2010 12:18 PM, Jan Kiszka wrote:
>> Am 10.11.2010 10:45, Avi Kivity wrote:
>> >  On 11/10/2010 11:30 AM, Jan Kiszka wrote:
>> >>  Am 10.11.2010 10:17, Avi Kivity wrote:
>> >>>   On 11/09/2010 11:01 PM, Alexander Graf wrote:
>> >>>>   On 09.11.2010, at 17:02, Jan Kiszka wrote:
>> >>>>
>> >>>>>    IA64 support forces us to abstract the allocation of the kvm structure.
>> >>>>>    But instead of mixing this up with arch-specific initialization and
>> >>>>>    doing the same on destruction, split both steps. This allows to move
>> >>>>>    generic destruction calls into generic code.
>> >>>>>
>> >>>>>    It also fixes error clean-up on failures of kvm_create_vm for IA64.
>> >>>>>
>> >>>>>    Signed-off-by: Jan Kiszka<jan.kiszka@siemens.com>
>> >>>>>    ---
>> >>>>>
>> >>>>>    Changes in v2:
>> >>>>>    - Fixed s390 conversion and added linux/slab.h
>> >>>>>      as remarked by Christian (thanks!)
>> >>>>>
>> >>>>>    arch/ia64/include/asm/kvm_host.h |    4 ++++
>> >>>>>    arch/ia64/kvm/kvm-ia64.c         |   28 +++++++---------------------
>> >>>>>    arch/powerpc/kvm/powerpc.c       |   20 +++-----------------
>> >>>>>    arch/s390/kvm/kvm-s390.c         |   23 ++++++-----------------
>> >>>>>    arch/x86/kvm/x86.c               |   12 ++----------
>> >>>>>    include/linux/kvm_host.h         |   15 ++++++++++++++-
>> >>>>>    virt/kvm/kvm_main.c              |   19 +++++++++++++------
>> >>>>>    7 files changed, 49 insertions(+), 72 deletions(-)
>> >>>>>
>> >>>>>    diff --git a/arch/ia64/include/asm/kvm_host.h b/arch/ia64/include/asm/kvm_host.h
>> >>>>>    index 2f229e5..2689ee5 100644
>> >>>>>    --- a/arch/ia64/include/asm/kvm_host.h
>> >>>>>    +++ b/arch/ia64/include/asm/kvm_host.h
>> >>>>>    @@ -590,6 +590,10 @@ int kvm_emulate_halt(struct kvm_vcpu *vcpu);
>> >>>>>    int kvm_pal_emul(struct kvm_vcpu *vcpu, struct kvm_run *kvm_run);
>> >>>>>    void kvm_sal_emul(struct kvm_vcpu *vcpu);
>> >>>>>
>> >>>>>    +#define __KVM_HAVE_ARCH_VM_ALLOC 1
>> >>>>>    +struct kvm *kvm_arch_alloc_vm(void);
>> >>>>>    +void kvm_arch_free_vm(struct kvm *kvm);
>> >>>>>    +
>> >>>>>    #endif /* __ASSEMBLY__*/
>> >>>>>
>> >>>>>    #endif
>> >>>>>    diff --git a/arch/ia64/kvm/kvm-ia64.c b/arch/ia64/kvm/kvm-ia64.c
>> >>>>>    index f56a631..48a48bd 100644
>> >>>>>    --- a/arch/ia64/kvm/kvm-ia64.c
>> >>>>>    +++ b/arch/ia64/kvm/kvm-ia64.c
>> >>>>>    @@ -749,7 +749,7 @@ out:
>> >>>>>    	return r;
>> >>>>>    }
>> >>>>>
>> >>>>>    -static struct kvm *kvm_alloc_kvm(void)
>> >>>>>    +struct kvm *kvm_arch_alloc_vm(void)
>> >>>>>    {
>> >>>>>
>> >>>>>    	struct kvm *kvm;
>> >>>>>    @@ -760,7 +760,7 @@ static struct kvm *kvm_alloc_kvm(void)
>> >>>>>    	vm_base = __get_free_pages(GFP_KERNEL, get_order(KVM_VM_DATA_SIZE));
>> >>>>>
>> >>>>>    	if (!vm_base)
>> >>>>>    -		return ERR_PTR(-ENOMEM);
>> >>>>>    +		return NULL;
>> >>>>>
>> >>>>>    	memset((void *)vm_base, 0, KVM_VM_DATA_SIZE);
>> >>>>>    	kvm = (struct kvm *)(vm_base +
>> >>>>>    @@ -806,10 +806,12 @@ static void kvm_build_io_pmt(struct kvm *kvm)
>> >>>>>    #define GUEST_PHYSICAL_RR4	0x2739
>> >>>>>    #define VMM_INIT_RR		0x1660
>> >>>>>
>> >>>>>    -static void kvm_init_vm(struct kvm *kvm)
>> >>>>>    +int kvm_arch_init_vm(struct kvm *kvm)
>> >>>>>    {
>> >>>>>    	BUG_ON(!kvm);
>> >>>>>
>> >>>>>    +	kvm->arch.is_sn2 = ia64_platform_is("sn2");
>> >>>>>    +
>> >>>>>    	kvm->arch.metaphysical_rr0 = GUEST_PHYSICAL_RR0;
>> >>>>>    	kvm->arch.metaphysical_rr4 = GUEST_PHYSICAL_RR4;
>> >>>>>    	kvm->arch.vmm_init_rr = VMM_INIT_RR;
>> >>>>>    @@ -823,21 +825,8 @@ static void kvm_init_vm(struct kvm *kvm)
>> >>>>>
>> >>>>>    	/* Reserve bit 0 of irq_sources_bitmap for userspace irq source */
>> >>>>>    	set_bit(KVM_USERSPACE_IRQ_SOURCE_ID,&kvm->arch.irq_sources_bitmap);
>> >>>>>    -}
>> >>>>>    -
>> >>>>>    -struct  kvm *kvm_arch_create_vm(void)
>> >>>>>    -{
>> >>>>>    -	struct kvm *kvm = kvm_alloc_kvm();
>> >>>>>    -
>> >>>>>    -	if (IS_ERR(kvm))
>> >>>>>    -		return ERR_PTR(-ENOMEM);
>> >>>>>    -
>> >>>>>    -	kvm->arch.is_sn2 = ia64_platform_is("sn2");
>> >>>>>    -
>> >>>>>    -	kvm_init_vm(kvm);
>> >>>>>    -
>> >>>>>    -	return kvm;
>> >>>>>
>> >>>>>    +	return 0;
>> >>>>>    }
>> >>>>>
>> >>>>>    static int kvm_vm_ioctl_get_irqchip(struct kvm *kvm,
>> >>>>>    @@ -1357,7 +1346,7 @@ int kvm_arch_vcpu_ioctl_set_guest_debug(struct kvm_vcpu *vcpu,
>> >>>>>    	return -EINVAL;
>> >>>>>    }
>> >>>>>
>> >>>>>    -static void free_kvm(struct kvm *kvm)
>> >>>>>    +void kvm_arch_free_vm(struct kvm *kvm)
>> >>>>>    {
>> >>>>>    	unsigned long vm_base = kvm->arch.vm_base;
>> >>>>>
>> >>>>>    @@ -1399,9 +1388,6 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
>> >>>>>    #endif
>> >>>>>    	kfree(kvm->arch.vioapic);
>> >>>>>    	kvm_release_vm_pages(kvm);
>> >>>>>    -	kvm_free_physmem(kvm);
>> >>>>>    -	cleanup_srcu_struct(&kvm->srcu);
>> >>>>>    -	free_kvm(kvm);
>> >>>>>    }
>> >>>>>
>> >>>>>    void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>> >>>>>    diff --git a/arch/powerpc/kvm/powerpc.c b/arch/powerpc/kvm/powerpc.c
>> >>>>>    index 38f756f..ce3dd65 100644
>> >>>>>    --- a/arch/powerpc/kvm/powerpc.c
>> >>>>>    +++ b/arch/powerpc/kvm/powerpc.c
>> >>>>>    @@ -145,18 +145,12 @@ void kvm_arch_check_processor_compat(void *rtn)
>> >>>>>    	*(int *)rtn = kvmppc_core_check_processor_compat();
>> >>>>>    }
>> >>>>>
>> >>>>>    -struct kvm *kvm_arch_create_vm(void)
>> >>>>>    +int kvm_arch_init_vm(struct kvm *)
>> >>>>
>> >>>>   Eh, no. This doesn't compile :).
>> >>>
>> >>>   What's the problem? lack of a formal argument?
>> >>
>> >>  The unnamed argument is referenced as 'kvm' in this function.
>> >
>> >  It isn't referenced:
>> >
>> >>  int kvm_arch_init_vm(struct kvm *)
>> >>  {
>> >>      return 0;
>> >>  }
>> >
>> 
>> Indeed. Anyway, this wasn't intended (even more if some compiler
>> versions/settings dislike it).
> 
> Ok.  I'll add the name and hope that this was the problem.

Yes, that was the problem. Sorry for not being clear about it, I thought it was obvious :).


Alex

Re: [PATCH v2] KVM: Clean up vm creation and release

[Avi Kivity]

On 11/10/2010 12:21 PM, Avi Kivity wrote:
>> Indeed. Anyway, this wasn't intended (even more if some compiler
>> versions/settings dislike it).
>
>
> Ok.  I'll add the name and hope that this was the problem.
>

Interestingly, ia64 and powerpc fail the build for unrelated reasons.

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