From mboxrd@z Thu Jan 1 00:00:00 1970 From: Gleb Natapov Subject: Re: [PATCH] KVM: remove vm mmap method Date: Tue, 5 Nov 2013 18:12:11 +0200 Message-ID: <20131105161211.GF31020@redhat.com> References: <20131105140417.GP7513@redhat.com> <5278FDC7.6070908@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: kvm@vger.kernel.org To: Paolo Bonzini Return-path: Received: from mx1.redhat.com ([209.132.183.28]:26881 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750732Ab3KEQMN (ORCPT ); Tue, 5 Nov 2013 11:12:13 -0500 Received: from int-mx01.intmail.prod.int.phx2.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id rA5GCD5o018256 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Tue, 5 Nov 2013 11:12:13 -0500 Content-Disposition: inline In-Reply-To: <5278FDC7.6070908@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Tue, Nov 05, 2013 at 03:16:39PM +0100, Paolo Bonzini wrote: > Il 05/11/2013 15:04, Gleb Natapov ha scritto: > > It was used in conjunction with KVM_SET_MEMORY_REGION ioctl which was > > removed by b74a07beed0 in 2010, QEMU stopped using it in 2008, so > > it is time to remove the code finally. > > > > Signed-off-by: Gleb Natapov > > I think it is usable (perhaps not useful...) on its own, though. It is > just 40 lines of self-contained code, keeping it doesn't do any harm. > Usable for what? Everything you can do with this code you can do without and it is unused for at least 5 years by anyone. You can't even say that the code is needed for backwards compatibility since KVM_SET_MEMORY_REGION, that this code was used with, is not longer exist. So this is just 40 lines of dead code, but it is not only dead it is outdated. The code around it evolved and nobody adjusted it. The code will fail to access memory in read only slot for instance (there was no read only slots back then). Yes, it can be fixed, but what for? If those 40 lines of code require maintenance we better off without them. > Paolo > > > diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c > > index 82c4047..a42f019 100644 > > --- a/virt/kvm/kvm_main.c > > +++ b/virt/kvm/kvm_main.c > > @@ -2524,44 +2524,12 @@ out: > > } > > #endif > > > > -static int kvm_vm_fault(struct vm_area_struct *vma, struct vm_fault *vmf) > > -{ > > - struct page *page[1]; > > - unsigned long addr; > > - int npages; > > - gfn_t gfn = vmf->pgoff; > > - struct kvm *kvm = vma->vm_file->private_data; > > - > > - addr = gfn_to_hva(kvm, gfn); > > - if (kvm_is_error_hva(addr)) > > - return VM_FAULT_SIGBUS; > > - > > - npages = get_user_pages(current, current->mm, addr, 1, 1, 0, page, > > - NULL); > > - if (unlikely(npages != 1)) > > - return VM_FAULT_SIGBUS; > > - > > - vmf->page = page[0]; > > - return 0; > > -} > > - > > -static const struct vm_operations_struct kvm_vm_vm_ops = { > > - .fault = kvm_vm_fault, > > -}; > > - > > -static int kvm_vm_mmap(struct file *file, struct vm_area_struct *vma) > > -{ > > - vma->vm_ops = &kvm_vm_vm_ops; > > - return 0; > > -} > > - > > static struct file_operations kvm_vm_fops = { > > .release = kvm_vm_release, > > .unlocked_ioctl = kvm_vm_ioctl, > > #ifdef CONFIG_COMPAT > > .compat_ioctl = kvm_vm_compat_ioctl, > > #endif > > - .mmap = kvm_vm_mmap, > > .llseek = noop_llseek, > > }; > > > > -- > > Gleb. > > -- Gleb.