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

Avi Kivity wrote:
> 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.

Okay, keep it.

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

I didn't remove it, and just exclude the assembly code to include them.  See below.

ifndef __ASSEMBLY__
+
+/*Define the max vcpus and memory for Guests.*/
+#define KVM_MAX_VCPUS  (KVM_VM_DATA_SIZE - KVM_P2M_SIZE - KVM_VM_STRUCT_SIZE -\
+                       KVM_MEM_DIRTY_LOG_SIZE) / sizeof(struct kvm_vcpu_data)
+#define KVM_MAX_MEM_SIZE (KVM_P2M_SIZE >> 3 << PAGE_SHIFT)
+
+#include <linux/types.h>
+#include <linux/mm.h>
+#include <linux/kvm.h>
+#include <linux/kvm_para.h>
+#include <linux/kvm_types.h>
+
+#include <asm/pal.h>
+#include <asm/sal.h>
+#include <asm/page.h>


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

These fields is useless and should be  removed from the kvm_arch structure. I will remove them in next version.

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

Okay, I will separate them in next version.  Thanks!
Xiantao