[PATCH] KVM: remove vm mmap method

[PATCH] KVM: remove vm mmap method

[Gleb Natapov]

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 <gleb@redhat.com>
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.

Re: [PATCH] KVM: remove vm mmap method

[Paolo Bonzini]

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 <gleb@redhat.com>

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.

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.
> 

Re: [PATCH] KVM: remove vm mmap method

[Gleb Natapov]

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 <gleb@redhat.com>
> 
> 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.

Re: [PATCH] KVM: remove vm mmap method

[Paolo Bonzini]

Il 05/11/2013 17:12, Gleb Natapov ha scritto:
> > 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?

Usable, not useful.  Perhaps someone prefers to map the guest address
space instead of using the memory areas they registrated.

> 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).

You're right it's broken for RO slots.  Go ahead and zap it. :)

Paolo