Re: [PATCH] KVM portability split : move mmu code to arch

[Avi Kivity <avi-atKUWr5tajBWk0Htik3J/w@public.gmane.org>]

Zhang, Xiantao wrote:
> According to privious decisions, mmu code should be moved to arch.  
>
> >From 43a629932b6babcbf2af75299192c1d77c2393d5 Mon Sep 17 00:00:00 2001
> From: Zhang Xiantao <xiantao.zhang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Date: Fri, 16 Nov 2007 13:16:40 +0800
> Subject: [PATCH] KVM: MMU split for portability.
> Since mmu code is very arch-denpendent, move them to archs.
> 1. For kvm_vm_ioctl_get_dirty_log, this ioctl should be first transfered
> to arch-specific code, and will 
> call the common code implemtend in kvm_get_dirty_log to perform common
> task. 
> 2. For set_memory_region,  I moved the mmu-speicific operations to an
> arch-ops. 
> Signed-off-by: Zhang Xiantao <xiantao.zhang-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/kvm/kvm.h      |    8 ++++
>  drivers/kvm/kvm_main.c |   49 +++------------------------
>  drivers/kvm/x86.c      |   85
> +++++++++++++++++++++++++++++++++++++++++++++---
>  3 files changed, 94 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/kvm/kvm_main.c b/drivers/kvm/kvm_main.c
> index 0414e92..84eec47 100644
> --- a/drivers/kvm/kvm_main.c
> +++ b/drivers/kvm/kvm_main.c
> @@ -362,29 +362,9 @@ int __kvm_set_memory_region(struct kvm *kvm,
>  	if (mem->slot >= kvm->nmemslots)
>  		kvm->nmemslots = mem->slot + 1;
>  
> -	if (!kvm->n_requested_mmu_pages) {
> -		unsigned int n_pages;
> -
> -		if (npages) {
> -			n_pages = npages * KVM_PERMILLE_MMU_PAGES /
> 1000;
> -			kvm_mmu_change_mmu_pages(kvm,
> kvm->n_alloc_mmu_pages +
> -						 n_pages);
> -		} else {
> -			unsigned int nr_mmu_pages;
> -
> -			n_pages = old.npages * KVM_PERMILLE_MMU_PAGES /
>   

We rely on the values in 'old' here...

> 1000;
> -			nr_mmu_pages = kvm->n_alloc_mmu_pages - n_pages;
> -			nr_mmu_pages = max(nr_mmu_pages,
> -				        (unsigned int)
> KVM_MIN_ALLOC_MMU_PAGES);
> -			kvm_mmu_change_mmu_pages(kvm, nr_mmu_pages);
> -		}
> -	}
>  
>  	*memslot = new;
>  
> -	kvm_mmu_slot_remove_write_access(kvm, mem->slot);
> -	kvm_flush_remote_tlbs(kvm);
> -
>  	kvm_free_physmem_slot(&old, &new);
>  	return 0;
>  
> @@ -404,6 +384,8 @@ int kvm_set_memory_region(struct kvm *kvm,
>  
>  	mutex_lock(&kvm->lock);
>  	r = __kvm_set_memory_region(kvm, mem, user_alloc);
> +	if (r == 0)
> +		kvm_arch_set_memory_region(kvm, mem, user_alloc);
>   

But 'old' does not longer exist here.  You recalculate it from  
kvm->memslots[] but the value is not longer identical to the 'old' value.

Suggest changing this code not to rely on 'old' at all; instead 
recalculate nr_mmu_pages from kvm->memslots[] each time.  That will make 
the code more robust, enabling the movement later.


-- 
error compiling committee.c: too many arguments to function


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2005.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/