[RFC][PATCH] reduce KVM stack usage

[RFC][PATCH] reduce KVM stack usage

[Dave Hansen]

KVM uses a lot of kernel stack on x86, especially with gcc 3.x.  It
likes to overflow it and make my poor machine go boom.  This patch takes
the worst stack users and makes them use kmalloc().  It also saves ~30
bytes in kvm_arch_vm_ioctl() by using a union.

I haven't tested this at all yet.  Just posting to get some comments.

Avi, you said that one of these was a hot path.  Which one is that?  I
kinda doubt you'll be able to see any impact anyway.

before:

0x000042d3 kvm_arch_vcpu_ioctl [kvm]: 2148
0x000012e3 kvm_vcpu_ioctl [kvm]: 1620
0x00004a83 kvm_arch_vm_ioctl [kvm]: 1332
0x0000d472 kvm_pv_mmu_op [kvm]: 536
0x0000d4eb kvm_pv_mmu_op [kvm]: 536
0x00000476 __kvm_set_memory_region [kvm]: 184
0x000007e3 __kvm_set_memory_region [kvm]: 184
0x00007eb3 kvm_task_switch_32 [kvm]: 128
0x0000b553 paging64_page_fault [kvm]: 112
0x0000bfa3 paging32_page_fault [kvm]: 112
0x0000e48b x86_emulate_insn [kvm]: 112
0x0000e785 x86_emulate_insn [kvm]: 112

after:

0x00000476 __kvm_set_memory_region [kvm]: 184
0x000007e3 __kvm_set_memory_region [kvm]: 184
0x00004c83 kvm_arch_vm_ioctl [kvm]: 164
0x000012e3 kvm_vcpu_ioctl [kvm]: 152
0x00008013 kvm_task_switch_32 [kvm]: 128
0x0000b6b3 paging64_page_fault [kvm]: 112
0x0000c103 paging32_page_fault [kvm]: 112
0x0000e5fb x86_emulate_insn [kvm]: 112
0x0000e8f5 x86_emulate_insn [kvm]: 112
0x00004353 kvm_arch_vcpu_ioctl [kvm]: 104

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 7e7c396..f4e62f2 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -2154,18 +2154,24 @@ int kvm_pv_mmu_op(struct kvm_vcpu *vcpu, unsigned long bytes,
 		  gpa_t addr, unsigned long *ret)
 {
 	int r;
-	struct kvm_pv_mmu_op_buffer buffer;
+	struct kvm_pv_mmu_op_buffer *buffer =
+		kmalloc(GFP_KERNEL, sizeof(struct kvm_pv_mmu_op_buffer));
 
-	buffer.ptr = buffer.buf;
-	buffer.len = min_t(unsigned long, bytes, sizeof buffer.buf);
-	buffer.processed = 0;
+	if (!buffer) {
+		*ret = 0;
+		return -ENOMEM;
+	}
+
+	buffer->ptr = buffer->buf;
+	buffer->len = min_t(unsigned long, bytes, sizeof buffer->buf);
+	buffer->processed = 0;
 
-	r = kvm_read_guest(vcpu->kvm, addr, buffer.buf, buffer.len);
+	r = kvm_read_guest(vcpu->kvm, addr, buffer->buf, buffer->len);
 	if (r)
 		goto out;
 
-	while (buffer.len) {
-		r = kvm_pv_mmu_op_one(vcpu, &buffer);
+	while (buffer->len) {
+		r = kvm_pv_mmu_op_one(vcpu, buffer);
 		if (r < 0)
 			goto out;
 		if (r == 0)
@@ -2174,7 +2180,8 @@ int kvm_pv_mmu_op(struct kvm_vcpu *vcpu, unsigned long bytes,
 
 	r = 1;
 out:
-	*ret = buffer.processed;
+	*ret = buffer->processed;
+	kfree(buffer);
 	return r;
 }
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 63a77ca..dbb0edd 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1283,12 +1283,15 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 	struct kvm_vcpu *vcpu = filp->private_data;
 	void __user *argp = (void __user *)arg;
 	int r;
+	struct kvm_lapic_state *lapic = NULL;
 
 	switch (ioctl) {
 	case KVM_GET_LAPIC: {
-		struct kvm_lapic_state lapic;
+		lapic = kzalloc(GFP_KERNEL, sizeof(*lapic));
 
-		memset(&lapic, 0, sizeof lapic);
+		r = -ENOMEM;
+		if (!lapic)
+			goto out;
 		r = kvm_vcpu_ioctl_get_lapic(vcpu, &lapic);
 		if (r)
 			goto out;
@@ -1299,8 +1302,10 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 		break;
 	}
 	case KVM_SET_LAPIC: {
-		struct kvm_lapic_state lapic;
-
+		lapic = kmalloc(GFP_KERNEL, sizeof(*lapic));
+		r = -ENOMEM;
+		if (!lapic)
+			goto out;
 		r = -EFAULT;
 		if (copy_from_user(&lapic, argp, sizeof lapic))
 			goto out;
@@ -1402,6 +1407,8 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 		r = -EINVAL;
 	}
 out:
+	if (lapic)
+		kfree(lapic);
 	return r;
 }
 
@@ -1602,12 +1609,73 @@ out:
 	return r;
 }
 
+static int kvm_arch_vm_irqchip_ioctl(struct kvm *kvm, void *argp,
+				     unsigned int ioctl)
+{
+	int ret = 0;
+	struct kvm_irqchip *chip = kmalloc(GFP_KERNEL, sizeof(*chip));
+
+	if (!chip)
+		return -ENOMEM;
+
+	/* cheaper than the copy, so do this first */
+	if (!irqchip_in_kernel(kvm)) {
+		ret = -ENXIO;
+		goto out;
+	}
+	if (copy_from_user(chip, argp, sizeof *chip)) {
+		ret = -EFAULT;
+		goto out;
+	}
+	switch (ioctl) {
+	case KVM_GET_IRQCHIP:
+		ret = kvm_vm_ioctl_get_irqchip(kvm, chip);
+		if (ret)
+			goto out;
+		ret = copy_to_user(argp, chip, sizeof *chip);
+		if (ret) {
+			ret = -EFAULT;
+			goto out;
+		}
+		break;
+	case KVM_SET_IRQCHIP:
+		ret = kvm_vm_ioctl_set_irqchip(kvm, chip);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+out:
+	kfree(chip);
+	return ret;
+}
+
+
+int x86_kvm_vm_ioctl_set_memory_region(struct kvm *kvm, void *argp)
+{
+	struct kvm_memory_region kvm_mem;
+	struct kvm_userspace_memory_region kvm_userspace_mem;
+
+	if (copy_from_user(&kvm_mem, argp, sizeof kvm_mem))
+		return -EFAULT;
+	kvm_userspace_mem.slot = kvm_mem.slot;
+	kvm_userspace_mem.flags = kvm_mem.flags;
+	kvm_userspace_mem.guest_phys_addr = kvm_mem.guest_phys_addr;
+	kvm_userspace_mem.memory_size = kvm_mem.memory_size;
+	return kvm_vm_ioctl_set_memory_region(kvm, &kvm_userspace_mem, 0);
+}
+
 long kvm_arch_vm_ioctl(struct file *filp,
 		       unsigned int ioctl, unsigned long arg)
 {
 	struct kvm *kvm = filp->private_data;
 	void __user *argp = (void __user *)arg;
 	int r = -EINVAL;
+	union {
+		/* 0: PIC master, 1: PIC slave, 2: IOAPIC */
+		struct kvm_pit_state ps;
+		struct kvm_memory_alias alias;
+	} u;
 
 	switch (ioctl) {
 	case KVM_SET_TSS_ADDR:
@@ -1639,17 +1707,14 @@ long kvm_arch_vm_ioctl(struct file *filp,
 	case KVM_GET_NR_MMU_PAGES:
 		r = kvm_vm_ioctl_get_nr_mmu_pages(kvm);
 		break;
-	case KVM_SET_MEMORY_ALIAS: {
-		struct kvm_memory_alias alias;
-
+	case KVM_SET_MEMORY_ALIAS:
 		r = -EFAULT;
-		if (copy_from_user(&alias, argp, sizeof alias))
+		if (copy_from_user(&u.alias, argp, sizeof u.alias))
 			goto out;
-		r = kvm_vm_ioctl_set_memory_alias(kvm, &alias);
+		r = kvm_vm_ioctl_set_memory_alias(kvm, &u.alias);
 		if (r)
 			goto out;
 		break;
-	}
 	case KVM_CREATE_IRQCHIP:
 		r = -ENOMEM;
 		kvm->arch.vpic = kvm_create_pic(kvm);
@@ -1689,67 +1754,37 @@ long kvm_arch_vm_ioctl(struct file *filp,
 		}
 		break;
 	}
-	case KVM_GET_IRQCHIP: {
-		/* 0: PIC master, 1: PIC slave, 2: IOAPIC */
-		struct kvm_irqchip chip;
-
-		r = -EFAULT;
-		if (copy_from_user(&chip, argp, sizeof chip))
-			goto out;
-		r = -ENXIO;
-		if (!irqchip_in_kernel(kvm))
-			goto out;
-		r = kvm_vm_ioctl_get_irqchip(kvm, &chip);
-		if (r)
-			goto out;
-		r = -EFAULT;
-		if (copy_to_user(argp, &chip, sizeof chip))
-			goto out;
-		r = 0;
-		break;
-	}
-	case KVM_SET_IRQCHIP: {
-		/* 0: PIC master, 1: PIC slave, 2: IOAPIC */
-		struct kvm_irqchip chip;
-
-		r = -EFAULT;
-		if (copy_from_user(&chip, argp, sizeof chip))
-			goto out;
-		r = -ENXIO;
-		if (!irqchip_in_kernel(kvm))
-			goto out;
-		r = kvm_vm_ioctl_set_irqchip(kvm, &chip);
+	case KVM_GET_IRQCHIP:
+	case KVM_SET_IRQCHIP:
+		r = kvm_arch_vm_irqchip_ioctl(kvm, argp, ioctl);
 		if (r)
 			goto out;
 		r = 0;
 		break;
-	}
 	case KVM_GET_PIT: {
-		struct kvm_pit_state ps;
 		r = -EFAULT;
-		if (copy_from_user(&ps, argp, sizeof ps))
+		if (copy_from_user(&u.ps, argp, sizeof u.ps))
 			goto out;
 		r = -ENXIO;
 		if (!kvm->arch.vpit)
 			goto out;
-		r = kvm_vm_ioctl_get_pit(kvm, &ps);
+		r = kvm_vm_ioctl_get_pit(kvm, &u.ps);
 		if (r)
 			goto out;
 		r = -EFAULT;
-		if (copy_to_user(argp, &ps, sizeof ps))
+		if (copy_to_user(argp, &u.ps, sizeof u.ps))
 			goto out;
 		r = 0;
 		break;
 	}
 	case KVM_SET_PIT: {
-		struct kvm_pit_state ps;
 		r = -EFAULT;
-		if (copy_from_user(&ps, argp, sizeof ps))
+		if (copy_from_user(&u.ps, argp, sizeof u.ps))
 			goto out;
 		r = -ENXIO;
 		if (!kvm->arch.vpit)
 			goto out;
-		r = kvm_vm_ioctl_set_pit(kvm, &ps);
+		r = kvm_vm_ioctl_set_pit(kvm, &u.ps);
 		if (r)
 			goto out;
 		r = 0;
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 2d29e26..be2816a 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -905,6 +905,9 @@ static long kvm_vcpu_ioctl(struct file *filp,
 	struct kvm_vcpu *vcpu = filp->private_data;
 	void __user *argp = (void __user *)arg;
 	int r;
+	struct kvm_fpu *fpu = NULL;
+	struct kvm_sregs *kvm_sregs = NULL;
+
 
 	if (vcpu->kvm->mm != current->mm)
 		return -EIO;
@@ -952,25 +955,29 @@ out_free2:
 		break;
 	}
 	case KVM_GET_SREGS: {
-		struct kvm_sregs kvm_sregs;
-
-		memset(&kvm_sregs, 0, sizeof kvm_sregs);
-		r = kvm_arch_vcpu_ioctl_get_sregs(vcpu, &kvm_sregs);
+		kvm_sregs = kzalloc(GFP_KERNEL, sizeof *kvm_sregs);
+		r = -ENOMEM;
+		if (!kvm_sregs)
+			goto out;
+		memset(kvm_sregs, 0, sizeof kvm_sregs);
+		r = kvm_arch_vcpu_ioctl_get_sregs(vcpu, kvm_sregs);
 		if (r)
 			goto out;
 		r = -EFAULT;
-		if (copy_to_user(argp, &kvm_sregs, sizeof kvm_sregs))
+		if (copy_to_user(argp, kvm_sregs, sizeof *kvm_sregs))
 			goto out;
 		r = 0;
 		break;
 	}
 	case KVM_SET_SREGS: {
-		struct kvm_sregs kvm_sregs;
-
+		kvm_sregs = kmalloc(GFP_KERNEL, sizeof kvm_sregs);
+		r = -ENOMEM;
+		if (!kvm_sregs)
+			goto out;
 		r = -EFAULT;
-		if (copy_from_user(&kvm_sregs, argp, sizeof kvm_sregs))
+		if (copy_from_user(kvm_sregs, argp, sizeof *kvm_sregs))
 			goto out;
-		r = kvm_arch_vcpu_ioctl_set_sregs(vcpu, &kvm_sregs);
+		r = kvm_arch_vcpu_ioctl_set_sregs(vcpu, kvm_sregs);
 		if (r)
 			goto out;
 		r = 0;
@@ -1051,25 +1058,28 @@ out_free2:
 		break;
 	}
 	case KVM_GET_FPU: {
-		struct kvm_fpu fpu;
-
-		memset(&fpu, 0, sizeof fpu);
-		r = kvm_arch_vcpu_ioctl_get_fpu(vcpu, &fpu);
+		fpu = kzalloc(GFP_KERNEL, sizeof(*fpu));
+		r = -ENOMEM;
+		if (!fpu)
+			goto out;
+		r = kvm_arch_vcpu_ioctl_get_fpu(vcpu, fpu);
 		if (r)
 			goto out;
 		r = -EFAULT;
-		if (copy_to_user(argp, &fpu, sizeof fpu))
+		if (copy_to_user(argp, fpu, sizeof *fpu))
 			goto out;
 		r = 0;
 		break;
 	}
 	case KVM_SET_FPU: {
-		struct kvm_fpu fpu;
-
+		fpu = kmalloc(GFP_KERNEL, sizeof(*fpu));
+		r = -ENOMEM;
+		if (!fpu)
+			goto out;
 		r = -EFAULT;
-		if (copy_from_user(&fpu, argp, sizeof fpu))
+		if (copy_from_user(fpu, argp, sizeof *fpu))
 			goto out;
-		r = kvm_arch_vcpu_ioctl_set_fpu(vcpu, &fpu);
+		r = kvm_arch_vcpu_ioctl_set_fpu(vcpu, fpu);
 		if (r)
 			goto out;
 		r = 0;
@@ -1079,6 +1089,10 @@ out_free2:
 		r = kvm_arch_vcpu_ioctl(filp, ioctl, arg);
 	}
 out:
+	if (fpu)
+		kfree(fpu);
+	if (kvm_sregs)
+		kfree(kvm_sregs);
 	return r;
 }
 



-- Dave

Re: [RFC][PATCH] reduce KVM stack usage

[Roland Dreier]

 > +	struct kvm_pv_mmu_op_buffer *buffer =
 > +		kmalloc(GFP_KERNEL, sizeof(struct kvm_pv_mmu_op_buffer));

Surely this produces a warning?  kmalloc takes (size, flags) -- you have
them reversed here.

 > +		lapic = kzalloc(GFP_KERNEL, sizeof(*lapic));
 > +		lapic = kmalloc(GFP_KERNEL, sizeof(*lapic));
 > +	struct kvm_irqchip *chip = kmalloc(GFP_KERNEL, sizeof(*chip));
 > +		kvm_sregs = kmalloc(GFP_KERNEL, sizeof kvm_sregs);
 > +		fpu = kmalloc(GFP_KERNEL, sizeof(*fpu));

same for all of these places.

 > +	if (lapic)
 > +		kfree(lapic);

 > +	if (fpu)
 > +		kfree(fpu);
 > +	if (kvm_sregs)
 > +		kfree(kvm_sregs);

kfree(NULL) is fine, so you can remove the if()s here.

Re: [RFC][PATCH] reduce KVM stack usage

[Dave Hansen]

On Thu, 2008-07-17 at 08:40 -0700, Roland Dreier wrote: 
> > +	struct kvm_pv_mmu_op_buffer *buffer =
>  > +		kmalloc(GFP_KERNEL, sizeof(struct kvm_pv_mmu_op_buffer));
> 
> Surely this produces a warning?  kmalloc takes (size, flags) -- you have
> them reversed here.

Heh.  It actually doesn't.  

> > +		lapic = kzalloc(GFP_KERNEL, sizeof(*lapic));
>  > +		lapic = kmalloc(GFP_KERNEL, sizeof(*lapic));
>  > +	struct kvm_irqchip *chip = kmalloc(GFP_KERNEL, sizeof(*chip));
>  > +		kvm_sregs = kmalloc(GFP_KERNEL, sizeof kvm_sregs);
>  > +		fpu = kmalloc(GFP_KERNEL, sizeof(*fpu));
> 
> same for all of these places.
> 
>  > +	if (lapic)
>  > +		kfree(lapic);
> 
>  > +	if (fpu)
>  > +		kfree(fpu);
>  > +	if (kvm_sregs)
>  > +		kfree(kvm_sregs);
> 
> kfree(NULL) is fine, so you can remove the if()s here.

I know it is fine, but I kinda like putting the if()s, just to let
people know that we don't always *expect* something to be in there.
But, it doesn't matter to me too much either way.

Here's another try.  This actually lets kvm come up for me.

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 7e7c396..fd1f303 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -2154,18 +2154,24 @@ int kvm_pv_mmu_op(struct kvm_vcpu *vcpu, unsigned long bytes,
 		  gpa_t addr, unsigned long *ret)
 {
 	int r;
-	struct kvm_pv_mmu_op_buffer buffer;
+	struct kvm_pv_mmu_op_buffer *buffer =
+		kmalloc(sizeof(struct kvm_pv_mmu_op_buffer), GFP_KERNEL);
 
-	buffer.ptr = buffer.buf;
-	buffer.len = min_t(unsigned long, bytes, sizeof buffer.buf);
-	buffer.processed = 0;
+	if (!buffer) {
+		*ret = 0;
+		return -ENOMEM;
+	}
+
+	buffer->ptr = buffer->buf;
+	buffer->len = min_t(unsigned long, bytes, sizeof buffer->buf);
+	buffer->processed = 0;
 
-	r = kvm_read_guest(vcpu->kvm, addr, buffer.buf, buffer.len);
+	r = kvm_read_guest(vcpu->kvm, addr, buffer->buf, buffer->len);
 	if (r)
 		goto out;
 
-	while (buffer.len) {
-		r = kvm_pv_mmu_op_one(vcpu, &buffer);
+	while (buffer->len) {
+		r = kvm_pv_mmu_op_one(vcpu, buffer);
 		if (r < 0)
 			goto out;
 		if (r == 0)
@@ -2174,7 +2180,8 @@ int kvm_pv_mmu_op(struct kvm_vcpu *vcpu, unsigned long bytes,
 
 	r = 1;
 out:
-	*ret = buffer.processed;
+	*ret = buffer->processed;
+	kfree(buffer);
 	return r;
 }
 
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 0faa254..14eae49 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -1283,13 +1283,16 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 	struct kvm_vcpu *vcpu = filp->private_data;
 	void __user *argp = (void __user *)arg;
 	int r;
+	struct kvm_lapic_state *lapic = NULL;
 
 	switch (ioctl) {
 	case KVM_GET_LAPIC: {
-		struct kvm_lapic_state lapic;
+		lapic = kzalloc(sizeof(struct kvm_lapic_state), GFP_KERNEL);
 
-		memset(&lapic, 0, sizeof lapic);
-		r = kvm_vcpu_ioctl_get_lapic(vcpu, &lapic);
+		r = -ENOMEM;
+		if (!lapic)
+			goto out;
+		r = kvm_vcpu_ioctl_get_lapic(vcpu, lapic);
 		if (r)
 			goto out;
 		r = -EFAULT;
@@ -1299,12 +1302,14 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 		break;
 	}
 	case KVM_SET_LAPIC: {
-		struct kvm_lapic_state lapic;
-
+		lapic = kmalloc(sizeof(struct kvm_lapic_state), GFP_KERNEL);
+		r = -ENOMEM;
+		if (!lapic)
+			goto out;
 		r = -EFAULT;
-		if (copy_from_user(&lapic, argp, sizeof lapic))
+		if (copy_from_user(lapic, argp, sizeof(struct kvm_lapic_state)))
 			goto out;
-		r = kvm_vcpu_ioctl_set_lapic(vcpu, &lapic);;
+		r = kvm_vcpu_ioctl_set_lapic(vcpu, lapic);
 		if (r)
 			goto out;
 		r = 0;
@@ -1402,6 +1407,7 @@ long kvm_arch_vcpu_ioctl(struct file *filp,
 		r = -EINVAL;
 	}
 out:
+	kfree(lapic);
 	return r;
 }
 
@@ -1602,12 +1608,73 @@ out:
 	return r;
 }
 
+static int kvm_arch_vm_irqchip_ioctl(struct kvm *kvm, void *argp,
+				     unsigned int ioctl)
+{
+	int ret = 0;
+	struct kvm_irqchip *chip = kmalloc(sizeof(struct kvm_irqchip), GFP_KERNEL);
+
+	if (!chip)
+		return -ENOMEM;
+
+	/* cheaper than the copy, so do this first */
+	if (!irqchip_in_kernel(kvm)) {
+		ret = -ENXIO;
+		goto out;
+	}
+	if (copy_from_user(chip, argp, sizeof(struct kvm_irqchip))) {
+		ret = -EFAULT;
+		goto out;
+	}
+	switch (ioctl) {
+	case KVM_GET_IRQCHIP:
+		ret = kvm_vm_ioctl_get_irqchip(kvm, chip);
+		if (ret)
+			goto out;
+		ret = copy_to_user(argp, chip, sizeof(struct kvm_irqchip));
+		if (ret) {
+			ret = -EFAULT;
+			goto out;
+		}
+		break;
+	case KVM_SET_IRQCHIP:
+		ret = kvm_vm_ioctl_set_irqchip(kvm, chip);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+out:
+	kfree(chip);
+	return ret;
+}
+
+
+int x86_kvm_vm_ioctl_set_memory_region(struct kvm *kvm, void *argp)
+{
+	struct kvm_memory_region kvm_mem;
+	struct kvm_userspace_memory_region kvm_userspace_mem;
+
+	if (copy_from_user(&kvm_mem, argp, sizeof(struct kvm_memory_region)))
+		return -EFAULT;
+	kvm_userspace_mem.slot = kvm_mem.slot;
+	kvm_userspace_mem.flags = kvm_mem.flags;
+	kvm_userspace_mem.guest_phys_addr = kvm_mem.guest_phys_addr;
+	kvm_userspace_mem.memory_size = kvm_mem.memory_size;
+	return kvm_vm_ioctl_set_memory_region(kvm, &kvm_userspace_mem, 0);
+}
+
 long kvm_arch_vm_ioctl(struct file *filp,
 		       unsigned int ioctl, unsigned long arg)
 {
 	struct kvm *kvm = filp->private_data;
 	void __user *argp = (void __user *)arg;
 	int r = -EINVAL;
+	union {
+		/* 0: PIC master, 1: PIC slave, 2: IOAPIC */
+		struct kvm_pit_state ps;
+		struct kvm_memory_alias alias;
+	} u;
 
 	switch (ioctl) {
 	case KVM_SET_TSS_ADDR:
@@ -1639,17 +1706,14 @@ long kvm_arch_vm_ioctl(struct file *filp,
 	case KVM_GET_NR_MMU_PAGES:
 		r = kvm_vm_ioctl_get_nr_mmu_pages(kvm);
 		break;
-	case KVM_SET_MEMORY_ALIAS: {
-		struct kvm_memory_alias alias;
-
+	case KVM_SET_MEMORY_ALIAS:
 		r = -EFAULT;
-		if (copy_from_user(&alias, argp, sizeof alias))
+		if (copy_from_user(&u.alias, argp, sizeof(struct kvm_memory_alias)))
 			goto out;
-		r = kvm_vm_ioctl_set_memory_alias(kvm, &alias);
+		r = kvm_vm_ioctl_set_memory_alias(kvm, &u.alias);
 		if (r)
 			goto out;
 		break;
-	}
 	case KVM_CREATE_IRQCHIP:
 		r = -ENOMEM;
 		kvm->arch.vpic = kvm_create_pic(kvm);
@@ -1689,67 +1753,37 @@ long kvm_arch_vm_ioctl(struct file *filp,
 		}
 		break;
 	}
-	case KVM_GET_IRQCHIP: {
-		/* 0: PIC master, 1: PIC slave, 2: IOAPIC */
-		struct kvm_irqchip chip;
-
-		r = -EFAULT;
-		if (copy_from_user(&chip, argp, sizeof chip))
-			goto out;
-		r = -ENXIO;
-		if (!irqchip_in_kernel(kvm))
-			goto out;
-		r = kvm_vm_ioctl_get_irqchip(kvm, &chip);
-		if (r)
-			goto out;
-		r = -EFAULT;
-		if (copy_to_user(argp, &chip, sizeof chip))
-			goto out;
-		r = 0;
-		break;
-	}
-	case KVM_SET_IRQCHIP: {
-		/* 0: PIC master, 1: PIC slave, 2: IOAPIC */
-		struct kvm_irqchip chip;
-
-		r = -EFAULT;
-		if (copy_from_user(&chip, argp, sizeof chip))
-			goto out;
-		r = -ENXIO;
-		if (!irqchip_in_kernel(kvm))
-			goto out;
-		r = kvm_vm_ioctl_set_irqchip(kvm, &chip);
+	case KVM_GET_IRQCHIP:
+	case KVM_SET_IRQCHIP:
+		r = kvm_arch_vm_irqchip_ioctl(kvm, argp, ioctl);
 		if (r)
 			goto out;
 		r = 0;
 		break;
-	}
 	case KVM_GET_PIT: {
-		struct kvm_pit_state ps;
 		r = -EFAULT;
-		if (copy_from_user(&ps, argp, sizeof ps))
+		if (copy_from_user(&u.ps, argp, sizeof(struct kvm_pit_state)))
 			goto out;
 		r = -ENXIO;
 		if (!kvm->arch.vpit)
 			goto out;
-		r = kvm_vm_ioctl_get_pit(kvm, &ps);
+		r = kvm_vm_ioctl_get_pit(kvm, &u.ps);
 		if (r)
 			goto out;
 		r = -EFAULT;
-		if (copy_to_user(argp, &ps, sizeof ps))
+		if (copy_to_user(argp, &u.ps, sizeof(struct kvm_pit_state)))
 			goto out;
 		r = 0;
 		break;
 	}
 	case KVM_SET_PIT: {
-		struct kvm_pit_state ps;
 		r = -EFAULT;
-		if (copy_from_user(&ps, argp, sizeof ps))
+		if (copy_from_user(&u.ps, argp, sizeof u.ps))
 			goto out;
 		r = -ENXIO;
 		if (!kvm->arch.vpit)
 			goto out;
-		r = kvm_vm_ioctl_set_pit(kvm, &ps);
+		r = kvm_vm_ioctl_set_pit(kvm, &u.ps);
 		if (r)
 			goto out;
 		r = 0;
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index d4eae6a..c31adbb 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -905,6 +905,9 @@ static long kvm_vcpu_ioctl(struct file *filp,
 	struct kvm_vcpu *vcpu = filp->private_data;
 	void __user *argp = (void __user *)arg;
 	int r;
+	struct kvm_fpu *fpu = NULL;
+	struct kvm_sregs *kvm_sregs = NULL;
+
 
 	if (vcpu->kvm->mm != current->mm)
 		return -EIO;
@@ -952,25 +955,29 @@ out_free2:
 		break;
 	}
 	case KVM_GET_SREGS: {
-		struct kvm_sregs kvm_sregs;
-
-		memset(&kvm_sregs, 0, sizeof kvm_sregs);
-		r = kvm_arch_vcpu_ioctl_get_sregs(vcpu, &kvm_sregs);
+		kvm_sregs = kzalloc(sizeof(struct kvm_sregs), GFP_KERNEL);
+		r = -ENOMEM;
+		if (!kvm_sregs)
+			goto out;
+		memset(kvm_sregs, 0, sizeof(struct kvm_sregs));
+		r = kvm_arch_vcpu_ioctl_get_sregs(vcpu, kvm_sregs);
 		if (r)
 			goto out;
 		r = -EFAULT;
-		if (copy_to_user(argp, &kvm_sregs, sizeof kvm_sregs))
+		if (copy_to_user(argp, kvm_sregs, sizeof(struct kvm_sregs)))
 			goto out;
 		r = 0;
 		break;
 	}
 	case KVM_SET_SREGS: {
-		struct kvm_sregs kvm_sregs;
-
+		kvm_sregs = kmalloc(sizeof(struct kvm_sregs), GFP_KERNEL);
+		r = -ENOMEM;
+		if (!kvm_sregs)
+			goto out;
 		r = -EFAULT;
-		if (copy_from_user(&kvm_sregs, argp, sizeof kvm_sregs))
+		if (copy_from_user(kvm_sregs, argp, sizeof(struct kvm_sregs)))
 			goto out;
-		r = kvm_arch_vcpu_ioctl_set_sregs(vcpu, &kvm_sregs);
+		r = kvm_arch_vcpu_ioctl_set_sregs(vcpu, kvm_sregs);
 		if (r)
 			goto out;
 		r = 0;
@@ -1051,25 +1058,28 @@ out_free2:
 		break;
 	}
 	case KVM_GET_FPU: {
-		struct kvm_fpu fpu;
-
-		memset(&fpu, 0, sizeof fpu);
-		r = kvm_arch_vcpu_ioctl_get_fpu(vcpu, &fpu);
+		fpu = kzalloc(sizeof(struct kvm_fpu), GFP_KERNEL);
+		r = -ENOMEM;
+		if (!fpu)
+			goto out;
+		r = kvm_arch_vcpu_ioctl_get_fpu(vcpu, fpu);
 		if (r)
 			goto out;
 		r = -EFAULT;
-		if (copy_to_user(argp, &fpu, sizeof fpu))
+		if (copy_to_user(argp, fpu, sizeof(struct kvm_fpu)))
 			goto out;
 		r = 0;
 		break;
 	}
 	case KVM_SET_FPU: {
-		struct kvm_fpu fpu;
-
+		fpu = kmalloc(sizeof(struct kvm_fpu), GFP_KERNEL);
+		r = -ENOMEM;
+		if (!fpu)
+			goto out;
 		r = -EFAULT;
-		if (copy_from_user(&fpu, argp, sizeof fpu))
+		if (copy_from_user(fpu, argp, sizeof(struct kvm_fpu)))
 			goto out;
-		r = kvm_arch_vcpu_ioctl_set_fpu(vcpu, &fpu);
+		r = kvm_arch_vcpu_ioctl_set_fpu(vcpu, fpu);
 		if (r)
 			goto out;
 		r = 0;
@@ -1079,6 +1089,8 @@ out_free2:
 		r = kvm_arch_vcpu_ioctl(filp, ioctl, arg);
 	}
 out:
+	kfree(fpu);
+	kfree(kvm_sregs);
 	return r;
 }
 


-- Dave

Re: [RFC][PATCH] reduce KVM stack usage

[Roland Dreier]

 > >  > +		kmalloc(GFP_KERNEL, sizeof(struct kvm_pv_mmu_op_buffer));

 > > Surely this produces a warning?  kmalloc takes (size, flags) -- you have
 > > them reversed here.

 > Heh.  It actually doesn't.  

Yeah, I guess you need sparse to catch the gfp_t mismatch.

 > > kfree(NULL) is fine, so you can remove the if()s here.

 > I know it is fine, but I kinda like putting the if()s, just to let
 > people know that we don't always *expect* something to be in there.
 > But, it doesn't matter to me too much either way.

It's not really a big deal, but "if (x) kfree(x);" does bloat the object
code with the extra test of 'x'.  I guess you could put a comment like
/* free any temp structures we allocated */ or something like that.

 - R.

Re: [RFC][PATCH] reduce KVM stack usage

[Avi Kivity]

Dave Hansen wrote:
> KVM uses a lot of kernel stack on x86, especially with gcc 3.x.  It
> likes to overflow it and make my poor machine go boom.  This patch takes
> the worst stack users and makes them use kmalloc().  It also saves ~30
> bytes in kvm_arch_vm_ioctl() by using a union.
>
> I haven't tested this at all yet.  Just posting to get some comments.
>
> Avi, you said that one of these was a hot path.  Which one is that?  I
> kinda doubt you'll be able to see any impact anyway.
>
>   

The pv_mmu_ops thing.

> diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
> index 7e7c396..f4e62f2 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -2154,18 +2154,24 @@ int kvm_pv_mmu_op(struct kvm_vcpu *vcpu, unsigned long bytes,
>  		  gpa_t addr, unsigned long *ret)
>  {
>  	int r;
> -	struct kvm_pv_mmu_op_buffer buffer;
> +	struct kvm_pv_mmu_op_buffer *buffer =
> +		kmalloc(GFP_KERNEL, sizeof(struct kvm_pv_mmu_op_buffer));
>  
> -	buffer.ptr = buffer.buf;
> -	buffer.len = min_t(unsigned long, bytes, sizeof buffer.buf);
> -	buffer.processed = 0;
> +	if (!buffer) {
> +		*ret = 0;
> +		return -ENOMEM;
> +	}
> +
> +	buffer->ptr = buffer->buf;
> +	buffer->len = min_t(unsigned long, bytes, sizeof buffer->buf);
> +	buffer->processed = 0;
>  
> -	r = kvm_read_guest(vcpu->kvm, addr, buffer.buf, buffer.len);
> +	r = kvm_read_guest(vcpu->kvm, addr, buffer->buf, buffer->len);
>  	if (r)
>  		goto out;
>  
> -	while (buffer.len) {
> -		r = kvm_pv_mmu_op_one(vcpu, &buffer);
> +	while (buffer->len) {
> +		r = kvm_pv_mmu_op_one(vcpu, buffer);
>  		if (r < 0)
>  			goto out;
>  		if (r == 0)
> @@ -2174,7 +2180,8 @@ int kvm_pv_mmu_op(struct kvm_vcpu *vcpu, unsigned long bytes,
>  
>  	r = 1;
>  out:
> -	*ret = buffer.processed;
> +	*ret = buffer->processed;
> +	kfree(buffer);
>   

Please change this to store the buffer in vcpu->arch.something, and free 
it on vcpu destruction.  If this path is called at all, it is called 
very often, so it's fine to "waste" those 512 bytes.

Might even allocate it as a field in vcpu->arch, since it's only 512 bytes.

>  
> +static int kvm_arch_vm_irqchip_ioctl(struct kvm *kvm, void *argp,
> +				     unsigned int ioctl)
> +{
> +
> +int x86_kvm_vm_ioctl_set_memory_region(struct kvm *kvm, void *argp)
> +{
>   

Please keep the code inlined in kvm_arch_vcpu_ioctl(), as this is a 
-stable candidate and I want to keep the patch obvious.  A later patch 
can break it up.

Please separate the vm ioctl, vcpu ioctl, and pv_mmu_op thing into 
separate patches, for bisect goodness.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.