Re: [PATCH] kvm/ia64: Code cleanup for Linux- 2.6.28.

[Avi Kivity <avi@redhat.com>]

Zhang, Xiantao wrote:
> Hi, Avi 
> Could you help to queue this patch for 2.6.28? It has no any logic
> changes, and just re-organizes the structure of vm data area to allow
> users to configure large memory and more vcpus. Since it changes many in
> header files, so it had better go into rc1. Thanks!
>   

If there are no logic changes, why is it needed to 2.6.28?

> diff --git a/arch/ia64/include/asm/kvm.h b/arch/ia64/include/asm/kvm.h
> index f38472a..de02c18 100644
> --- a/arch/ia64/include/asm/kvm.h
> +++ b/arch/ia64/include/asm/kvm.h
> @@ -207,5 +207,4 @@ struct kvm_sregs {
>  
>  struct kvm_fpu {
>  };
> -
>  #endif
>   

Actually the blank line is helpful here.

> diff --git a/arch/ia64/include/asm/kvm_host.h
> b/arch/ia64/include/asm/kvm_host.h
> index b9e3c7f..e98f6f0 100644
> --- a/arch/ia64/include/asm/kvm_host.h
> +++ b/arch/ia64/include/asm/kvm_host.h
> @@ -23,17 +23,6 @@
>  #ifndef __ASM_KVM_HOST_H
>  #define __ASM_KVM_HOST_H
>  
> -
> -#include <linux/types.h>
> -#include <linux/mm.h>
> -#include <linux/kvm.h>
> -#include <linux/kvm_para.h>
> -#include <linux/kvm_types.h>
> -
>   

Are you sure these are unneeded here?  For example, if u8 or u32 are 
referenced in this file, you should keep linux/types.h.  A header file 
should require no other header files to be previously included.

> @ -765,21 +763,12 @@ static void kvm_build_io_pmt(struct kvm *kvm)
>  
>  static void kvm_init_vm(struct kvm *kvm)
>  {
> -	long vm_base;
> -
>  	BUG_ON(!kvm);
>  
>  	kvm->arch.metaphysical_rr0 = GUEST_PHYSICAL_RR0;
>  	kvm->arch.metaphysical_rr4 = GUEST_PHYSICAL_RR4;
>  	kvm->arch.vmm_init_rr = VMM_INIT_RR;
>  
> -	vm_base = kvm->arch.vm_base;
> -	if (vm_base) {
> -		kvm->arch.vhpt_base = vm_base + KVM_VHPT_OFS;
> -		kvm->arch.vtlb_base = vm_base + KVM_VTLB_OFS;
> -		kvm->arch.vpd_base  = vm_base + KVM_VPD_OFS;
> -	}
> -
>   

Is this guaranteed to be zero?  It doesn't seem like a code cleanup.  
Best in a separate patch with an explanation why it is safe.

> @@ -183,8 +183,8 @@ void mark_pages_dirty(struct kvm_vcpu *v, u64 pte,
> u64 ps)
>  	u64 i, dirty_pages = 1;
>  	u64 base_gfn = (pte&_PAGE_PPN_MASK) >> PAGE_SHIFT;
>  	spinlock_t *lock = __kvm_va(v->arch.dirty_log_lock_pa);
> -	void *dirty_bitmap = (void *)v - (KVM_VCPU_OFS + v->vcpu_id *
> VCPU_SIZE)
> -						+ KVM_MEM_DIRTY_LOG_OFS;
> +	void *dirty_bitmap = (void *)KVM_MEM_DIRTY_LOG_BASE;
> +
>   

This doesn't seem like a cleanup.

In general the offsetof() changes are very good, but please separate 
them from the typos and coding style fixes.

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