From patchwork Fri Jan 11 09:28:35 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Takuya Yoshikawa X-Patchwork-Id: 1965271 Return-Path: X-Original-To: patchwork-kvm@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork1.kernel.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by patchwork1.kernel.org (Postfix) with ESMTP id 0CCA23FE37 for ; Fri, 11 Jan 2013 09:28:05 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753043Ab3AKJ17 (ORCPT ); Fri, 11 Jan 2013 04:27:59 -0500 Received: from tama50.ecl.ntt.co.jp ([129.60.39.147]:32971 "EHLO tama50.ecl.ntt.co.jp" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751591Ab3AKJ16 (ORCPT ); Fri, 11 Jan 2013 04:27:58 -0500 Received: from mfs6.rdh.ecl.ntt.co.jp (mfs6.rdh.ecl.ntt.co.jp [129.60.39.149]) by tama50.ecl.ntt.co.jp (8.13.8/8.13.8) with ESMTP id r0B9RtAp021617; Fri, 11 Jan 2013 18:27:55 +0900 Received: from mfs6.rdh.ecl.ntt.co.jp (localhost.localdomain [127.0.0.1]) by mfs6.rdh.ecl.ntt.co.jp (Postfix) with ESMTP id 7CB37E0194; Fri, 11 Jan 2013 18:27:55 +0900 (JST) Received: from imail2.m.ecl.ntt.co.jp (imail2.m.ecl.ntt.co.jp [129.60.5.247]) by mfs6.rdh.ecl.ntt.co.jp (Postfix) with ESMTP id 70EC5E018E; Fri, 11 Jan 2013 18:27:55 +0900 (JST) Received: from yshpad ([129.60.241.180]) by imail2.m.ecl.ntt.co.jp (8.13.8/8.13.8) with SMTP id r0B9RtbO029399; Fri, 11 Jan 2013 18:27:55 +0900 Date: Fri, 11 Jan 2013 18:28:35 +0900 From: Takuya Yoshikawa To: mtosatti@redhat.com, gleb@redhat.com Cc: kvm@vger.kernel.org Subject: [PATCH 4/4] KVM: set_memory_region: Identify the requested change explicitly Message-Id: <20130111182835.fbf8b783.yoshikawa_takuya_b1@lab.ntt.co.jp> In-Reply-To: <20130111182518.6c5975d9.yoshikawa_takuya_b1@lab.ntt.co.jp> References: <20130111182518.6c5975d9.yoshikawa_takuya_b1@lab.ntt.co.jp> X-Mailer: Sylpheed 3.1.0 (GTK+ 2.24.4; x86_64-pc-linux-gnu) Mime-Version: 1.0 Sender: kvm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: kvm@vger.kernel.org KVM_SET_USER_MEMORY_REGION forces __kvm_set_memory_region() to identify what kind of change is being requested by checking the parameters. The current code does this checking at various points in code and each condition used there is not easy to understand at first glance. This patch consolidates these and introduces an enum to name the possible changes to clean up the code. Although this does not introduce any user visible changes, there are two changes which optimize the code a bit: 1. If we have nothing to change, the new code returns immediately. 2. If we just modify the flags, no changes about the mapping, the new code does not call kvm_iommu_map_pages(). One interesting thing we noticed was about the case 1. Although this case seemed unimportant, QEMU was relying on the current behaviour: when we returned a value other than 0, e.g. -EINVAL, live migration failed at the final stage with a section mismatch error. Signed-off-by: Takuya Yoshikawa --- virt/kvm/kvm_main.c | 64 +++++++++++++++++++++++++++++++++++---------------- 1 files changed, 44 insertions(+), 20 deletions(-) diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 8cb4e71..5426e96 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -714,6 +714,24 @@ static struct kvm_memslots *install_new_memslots(struct kvm *kvm, } /* + * KVM_SET_USER_MEMORY_REGION ioctl allows the following operations: + * - create a new memory slot + * - delete an existing memory slot + * - modify an existing memory slot + * -- move it in the guest physical memory space + * -- just change its flags + * + * Since flags can be changed by some of these operations, the following + * differentiation is the best we can do for __kvm_set_memory_region(): + */ +enum kvm_mr_change { + KVM_MR_CREATE, + KVM_MR_DELETE, + KVM_MR_MOVE, + KVM_MR_FLAGS_ONLY, +}; + +/* * Allocate some memory and give it an address in the guest physical address * space. * @@ -731,6 +749,7 @@ int __kvm_set_memory_region(struct kvm *kvm, struct kvm_memory_slot *slot; struct kvm_memory_slot old, new; struct kvm_memslots *slots = NULL, *old_memslots; + enum kvm_mr_change change; r = check_memory_region_flags(mem); if (r) @@ -772,17 +791,30 @@ int __kvm_set_memory_region(struct kvm *kvm, new.npages = npages; new.flags = mem->flags; - /* - * Disallow changing a memory slot's size or changing anything about - * zero sized slots that doesn't involve making them non-zero. - */ r = -EINVAL; - if (npages && old.npages && npages != old.npages) - goto out; - if (!npages && !old.npages) + if (npages) { + if (!old.npages) + change = KVM_MR_CREATE; + else { /* Modify an existing slot. */ + if ((mem->userspace_addr != old.userspace_addr) || + (npages != old.npages)) + goto out; + + if (base_gfn != old.base_gfn) + change = KVM_MR_MOVE; + else if (new.flags != old.flags) + change = KVM_MR_FLAGS_ONLY; + else { /* Nothing to change. */ + r = 0; + goto out; + } + } + } else if (old.npages) { + change = KVM_MR_DELETE; + } else /* Modify a non-existent slot: disallowed. */ goto out; - if ((npages && !old.npages) || (base_gfn != old.base_gfn)) { + if ((change == KVM_MR_CREATE) || (change == KVM_MR_MOVE)) { /* Check for overlaps */ r = -EEXIST; kvm_for_each_memslot(slot, kvm->memslots) { @@ -800,20 +832,12 @@ int __kvm_set_memory_region(struct kvm *kvm, new.dirty_bitmap = NULL; r = -ENOMEM; - - /* - * Allocate if a slot is being created. If modifying a slot, - * the userspace_addr cannot change. - */ - if (!old.npages) { + if (change == KVM_MR_CREATE) { new.user_alloc = user_alloc; new.userspace_addr = mem->userspace_addr; if (kvm_arch_create_memslot(&new, npages)) goto out_free; - } else if (npages && mem->userspace_addr != old.userspace_addr) { - r = -EINVAL; - goto out_free; } /* Allocate page dirty bitmap if needed */ @@ -823,7 +847,7 @@ int __kvm_set_memory_region(struct kvm *kvm, /* destroy any largepage mappings for dirty tracking */ } - if (!npages || base_gfn != old.base_gfn) { + if ((change == KVM_MR_DELETE) || (change == KVM_MR_MOVE)) { r = -ENOMEM; slots = kmemdup(kvm->memslots, sizeof(struct kvm_memslots), GFP_KERNEL); @@ -865,14 +889,14 @@ int __kvm_set_memory_region(struct kvm *kvm, } /* map new memory slot into the iommu */ - if (npages) { + if ((change == KVM_MR_CREATE) || (change == KVM_MR_MOVE)) { r = kvm_iommu_map_pages(kvm, &new); if (r) goto out_slots; } /* actual memory is freed via old in kvm_free_physmem_slot below */ - if (!npages) { + if (change == KVM_MR_DELETE) { new.dirty_bitmap = NULL; memset(&new.arch, 0, sizeof(new.arch)); }