From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dave Hansen Subject: Re: [RFC][PATCH] reduce KVM stack usage Date: Thu, 17 Jul 2008 10:23:11 -0700 Message-ID: <1216315391.9311.3.camel@nimitz> References: <1216308735.9161.3.camel@nimitz> Mime-Version: 1.0 Content-Type: text/plain Content-Transfer-Encoding: 7bit Cc: Avi Kivity , kvm-devel , Anthony Liguori To: Roland Dreier Return-path: Received: from e36.co.us.ibm.com ([32.97.110.154]:32992 "EHLO e36.co.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757257AbYGQRXS (ORCPT ); Thu, 17 Jul 2008 13:23:18 -0400 Received: from d03relay04.boulder.ibm.com (d03relay04.boulder.ibm.com [9.17.195.106]) by e36.co.us.ibm.com (8.13.8/8.13.8) with ESMTP id m6HHNEb1020196 for ; Thu, 17 Jul 2008 13:23:14 -0400 Received: from d03av01.boulder.ibm.com (d03av01.boulder.ibm.com [9.17.195.167]) by d03relay04.boulder.ibm.com (8.13.8/8.13.8/NCO v9.0) with ESMTP id m6HHNEhq072848 for ; Thu, 17 Jul 2008 11:23:14 -0600 Received: from d03av01.boulder.ibm.com (loopback [127.0.0.1]) by d03av01.boulder.ibm.com (8.12.11.20060308/8.13.3) with ESMTP id m6HHNDTL019568 for ; Thu, 17 Jul 2008 11:23:14 -0600 In-Reply-To: Sender: kvm-owner@vger.kernel.org List-ID: 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