From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Date: Wed, 22 Oct 2008 10:40:17 +0000 Subject: Re: [PATCH] kvm/ia64: Code cleanup for Linux- 2.6.28. Message-Id: <48FF0311.6050100@redhat.com> List-Id: References: <42DFA526FC41B1429CE7279EF83C6BDC01B47EC3@pdsmsx415.ccr.corp.intel.com> In-Reply-To: <42DFA526FC41B1429CE7279EF83C6BDC01B47EC3@pdsmsx415.ccr.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: kvm-ia64@vger.kernel.org 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 > -#include > -#include > -#include > -#include > - > 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: Avi Kivity Subject: Re: [PATCH] kvm/ia64: Code cleanup for Linux- 2.6.28. Date: Wed, 22 Oct 2008 12:40:17 +0200 Message-ID: <48FF0311.6050100@redhat.com> References: <42DFA526FC41B1429CE7279EF83C6BDC01B47EC3@pdsmsx415.ccr.corp.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: kvm-ia64@vger.kernel.org, kvm@vger.kernel.org To: "Zhang, Xiantao" Return-path: Received: from mx2.redhat.com ([66.187.237.31]:39837 "EHLO mx2.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756152AbYJVKkW (ORCPT ); Wed, 22 Oct 2008 06:40:22 -0400 In-Reply-To: <42DFA526FC41B1429CE7279EF83C6BDC01B47EC3@pdsmsx415.ccr.corp.intel.com> Sender: kvm-owner@vger.kernel.org List-ID: 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 > -#include > -#include > -#include > -#include > - > 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