From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dave Hansen Subject: [RFC][PATCH] reduce KVM stack usage Date: Thu, 17 Jul 2008 08:32:15 -0700 Message-ID: <1216308735.9161.3.camel@nimitz> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: kvm-devel , Anthony Liguori , Roland Dreier To: Avi Kivity Return-path: Received: from e34.co.us.ibm.com ([32.97.110.152]:58862 "EHLO e34.co.us.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751663AbYGQPch (ORCPT ); Thu, 17 Jul 2008 11:32:37 -0400 Received: from d03relay04.boulder.ibm.com (d03relay04.boulder.ibm.com [9.17.195.106]) by e34.co.us.ibm.com (8.13.8/8.13.8) with ESMTP id m6HFWacB022234 for ; Thu, 17 Jul 2008 11:32:36 -0400 Received: from d03av02.boulder.ibm.com (d03av02.boulder.ibm.com [9.17.195.168]) by d03relay04.boulder.ibm.com (8.13.8/8.13.8/NCO v9.0) with ESMTP id m6HFWL4O110584 for ; Thu, 17 Jul 2008 09:32:21 -0600 Received: from d03av02.boulder.ibm.com (loopback [127.0.0.1]) by d03av02.boulder.ibm.com (8.12.11.20060308/8.13.3) with ESMTP id m6HFWK2Z006544 for ; Thu, 17 Jul 2008 09:32:21 -0600 Sender: kvm-owner@vger.kernel.org List-ID: 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 take= s 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? =EF= =BB=BFI 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, unsign= ed long bytes, gpa_t addr, unsigned long *ret) { int r; - struct kvm_pv_mmu_op_buffer buffer; + struct kvm_pv_mmu_op_buffer *buffer =3D + kmalloc(GFP_KERNEL, sizeof(struct kvm_pv_mmu_op_buffer)); =20 - buffer.ptr =3D buffer.buf; - buffer.len =3D min_t(unsigned long, bytes, sizeof buffer.buf); - buffer.processed =3D 0; + if (!buffer) { + *ret =3D 0; + return -ENOMEM; + } + + buffer->ptr =3D buffer->buf; + buffer->len =3D min_t(unsigned long, bytes, sizeof buffer->buf); + buffer->processed =3D 0; =20 - r =3D kvm_read_guest(vcpu->kvm, addr, buffer.buf, buffer.len); + r =3D kvm_read_guest(vcpu->kvm, addr, buffer->buf, buffer->len); if (r) goto out; =20 - while (buffer.len) { - r =3D kvm_pv_mmu_op_one(vcpu, &buffer); + while (buffer->len) { + r =3D kvm_pv_mmu_op_one(vcpu, buffer); if (r < 0) goto out; if (r =3D=3D 0) @@ -2174,7 +2180,8 @@ int kvm_pv_mmu_op(struct kvm_vcpu *vcpu, unsigned= long bytes, =20 r =3D 1; out: - *ret =3D buffer.processed; + *ret =3D buffer->processed; + kfree(buffer); return r; } =20 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 =3D filp->private_data; void __user *argp =3D (void __user *)arg; int r; + struct kvm_lapic_state *lapic =3D NULL; =20 switch (ioctl) { case KVM_GET_LAPIC: { - struct kvm_lapic_state lapic; + lapic =3D kzalloc(GFP_KERNEL, sizeof(*lapic)); =20 - memset(&lapic, 0, sizeof lapic); + r =3D -ENOMEM; + if (!lapic) + goto out; r =3D 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 =3D kmalloc(GFP_KERNEL, sizeof(*lapic)); + r =3D -ENOMEM; + if (!lapic) + goto out; r =3D -EFAULT; if (copy_from_user(&lapic, argp, sizeof lapic)) goto out; @@ -1402,6 +1407,8 @@ long kvm_arch_vcpu_ioctl(struct file *filp, r =3D -EINVAL; } out: + if (lapic) + kfree(lapic); return r; } =20 @@ -1602,12 +1609,73 @@ out: return r; } =20 +static int kvm_arch_vm_irqchip_ioctl(struct kvm *kvm, void *argp, + unsigned int ioctl) +{ + int ret =3D 0; + struct kvm_irqchip *chip =3D kmalloc(GFP_KERNEL, sizeof(*chip)); + + if (!chip) + return -ENOMEM; + + /* cheaper than the copy, so do this first */ + if (!irqchip_in_kernel(kvm)) { + ret =3D -ENXIO; + goto out; + } + if (copy_from_user(chip, argp, sizeof *chip)) { + ret =3D -EFAULT; + goto out; + } + switch (ioctl) { + case KVM_GET_IRQCHIP: + ret =3D kvm_vm_ioctl_get_irqchip(kvm, chip); + if (ret) + goto out; + ret =3D copy_to_user(argp, chip, sizeof *chip); + if (ret) { + ret =3D -EFAULT; + goto out; + } + break; + case KVM_SET_IRQCHIP: + ret =3D kvm_vm_ioctl_set_irqchip(kvm, chip); + break; + default: + ret =3D -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 =3D kvm_mem.slot; + kvm_userspace_mem.flags =3D kvm_mem.flags; + kvm_userspace_mem.guest_phys_addr =3D kvm_mem.guest_phys_addr; + kvm_userspace_mem.memory_size =3D 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 =3D filp->private_data; void __user *argp =3D (void __user *)arg; int r =3D -EINVAL; + union { + /* 0: PIC master, 1: PIC slave, 2: IOAPIC */ + struct kvm_pit_state ps; + struct kvm_memory_alias alias; + } u; =20 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 =3D 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 =3D -EFAULT; - if (copy_from_user(&alias, argp, sizeof alias)) + if (copy_from_user(&u.alias, argp, sizeof u.alias)) goto out; - r =3D kvm_vm_ioctl_set_memory_alias(kvm, &alias); + r =3D kvm_vm_ioctl_set_memory_alias(kvm, &u.alias); if (r) goto out; break; - } case KVM_CREATE_IRQCHIP: r =3D -ENOMEM; kvm->arch.vpic =3D 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 =3D -EFAULT; - if (copy_from_user(&chip, argp, sizeof chip)) - goto out; - r =3D -ENXIO; - if (!irqchip_in_kernel(kvm)) - goto out; - r =3D kvm_vm_ioctl_get_irqchip(kvm, &chip); - if (r) - goto out; - r =3D -EFAULT; - if (copy_to_user(argp, &chip, sizeof chip)) - goto out; - r =3D 0; - break; - } - case KVM_SET_IRQCHIP: { - /* 0: PIC master, 1: PIC slave, 2: IOAPIC */ - struct kvm_irqchip chip; - - r =3D -EFAULT; - if (copy_from_user(&chip, argp, sizeof chip)) - goto out; - r =3D -ENXIO; - if (!irqchip_in_kernel(kvm)) - goto out; - r =3D kvm_vm_ioctl_set_irqchip(kvm, &chip); + case KVM_GET_IRQCHIP: + case KVM_SET_IRQCHIP: + r =3D kvm_arch_vm_irqchip_ioctl(kvm, argp, ioctl); if (r) goto out; r =3D 0; break; - } case KVM_GET_PIT: { - struct kvm_pit_state ps; r =3D -EFAULT; - if (copy_from_user(&ps, argp, sizeof ps)) + if (copy_from_user(&u.ps, argp, sizeof u.ps)) goto out; r =3D -ENXIO; if (!kvm->arch.vpit) goto out; - r =3D kvm_vm_ioctl_get_pit(kvm, &ps); + r =3D kvm_vm_ioctl_get_pit(kvm, &u.ps); if (r) goto out; r =3D -EFAULT; - if (copy_to_user(argp, &ps, sizeof ps)) + if (copy_to_user(argp, &u.ps, sizeof u.ps)) goto out; r =3D 0; break; } case KVM_SET_PIT: { - struct kvm_pit_state ps; r =3D -EFAULT; - if (copy_from_user(&ps, argp, sizeof ps)) + if (copy_from_user(&u.ps, argp, sizeof u.ps)) goto out; r =3D -ENXIO; if (!kvm->arch.vpit) goto out; - r =3D kvm_vm_ioctl_set_pit(kvm, &ps); + r =3D kvm_vm_ioctl_set_pit(kvm, &u.ps); if (r) goto out; r =3D 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 =3D filp->private_data; void __user *argp =3D (void __user *)arg; int r; + struct kvm_fpu *fpu =3D NULL; + struct kvm_sregs *kvm_sregs =3D NULL; + =20 if (vcpu->kvm->mm !=3D 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 =3D kvm_arch_vcpu_ioctl_get_sregs(vcpu, &kvm_sregs); + kvm_sregs =3D kzalloc(GFP_KERNEL, sizeof *kvm_sregs); + r =3D -ENOMEM; + if (!kvm_sregs) + goto out; + memset(kvm_sregs, 0, sizeof kvm_sregs); + r =3D kvm_arch_vcpu_ioctl_get_sregs(vcpu, kvm_sregs); if (r) goto out; r =3D -EFAULT; - if (copy_to_user(argp, &kvm_sregs, sizeof kvm_sregs)) + if (copy_to_user(argp, kvm_sregs, sizeof *kvm_sregs)) goto out; r =3D 0; break; } case KVM_SET_SREGS: { - struct kvm_sregs kvm_sregs; - + kvm_sregs =3D kmalloc(GFP_KERNEL, sizeof kvm_sregs); + r =3D -ENOMEM; + if (!kvm_sregs) + goto out; r =3D -EFAULT; - if (copy_from_user(&kvm_sregs, argp, sizeof kvm_sregs)) + if (copy_from_user(kvm_sregs, argp, sizeof *kvm_sregs)) goto out; - r =3D kvm_arch_vcpu_ioctl_set_sregs(vcpu, &kvm_sregs); + r =3D kvm_arch_vcpu_ioctl_set_sregs(vcpu, kvm_sregs); if (r) goto out; r =3D 0; @@ -1051,25 +1058,28 @@ out_free2: break; } case KVM_GET_FPU: { - struct kvm_fpu fpu; - - memset(&fpu, 0, sizeof fpu); - r =3D kvm_arch_vcpu_ioctl_get_fpu(vcpu, &fpu); + fpu =3D kzalloc(GFP_KERNEL, sizeof(*fpu)); + r =3D -ENOMEM; + if (!fpu) + goto out; + r =3D kvm_arch_vcpu_ioctl_get_fpu(vcpu, fpu); if (r) goto out; r =3D -EFAULT; - if (copy_to_user(argp, &fpu, sizeof fpu)) + if (copy_to_user(argp, fpu, sizeof *fpu)) goto out; r =3D 0; break; } case KVM_SET_FPU: { - struct kvm_fpu fpu; - + fpu =3D kmalloc(GFP_KERNEL, sizeof(*fpu)); + r =3D -ENOMEM; + if (!fpu) + goto out; r =3D -EFAULT; - if (copy_from_user(&fpu, argp, sizeof fpu)) + if (copy_from_user(fpu, argp, sizeof *fpu)) goto out; - r =3D kvm_arch_vcpu_ioctl_set_fpu(vcpu, &fpu); + r =3D kvm_arch_vcpu_ioctl_set_fpu(vcpu, fpu); if (r) goto out; r =3D 0; @@ -1079,6 +1089,10 @@ out_free2: r =3D kvm_arch_vcpu_ioctl(filp, ioctl, arg); } out: + if (fpu) + kfree(fpu); + if (kvm_sregs) + kfree(kvm_sregs); return r; } =20 -- Dave