Re: [PATCH][1/3] Move kvm_vcpu_iotcl_get_dirty_log to arch

[Carsten Otte <cotte-tA70FqPdS9bQT0dZR+AlfA@public.gmane.org>]

Avi Kivity wrote:
> Carsten Otte wrote:
>> Zhang, Xiantao wrote:
>>> Move kvm_vcpu_ioctl_get_dirty_log to arch, and still keep the interface
>>> in common.
>> diff --git a/drivers/kvm/kvm_main.c b/drivers/kvm/kvm_main.c
>> index 45b18e1..3400265 100644
>> --- a/drivers/kvm/kvm_main.c
>> +++ b/drivers/kvm/kvm_main.c
>> @@ -419,19 +419,14 @@ int kvm_vm_ioctl_set_memory_region(struct kvm *kvm,
>>      return kvm_set_memory_region(kvm, mem, user_alloc);
>>  }
>>
>> -/*
>> - * Get (and clear) the dirty memory log for a memory slot.
>> - */
>> -static int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
>> -                      struct kvm_dirty_log *log)
>> +int kvm_get_dirty_log(struct kvm *kvm,
>> +            struct kvm_dirty_log *log, int *is_dirty)
>>
>> Any reason to remove that comment? It looks helpful to me.
>>
>>
>>  {
>>      struct kvm_memory_slot *memslot;
>>      int r, i;
>>      int n;
>>      unsigned long any = 0;
>>
>> -    mutex_lock(&kvm->lock);
>> -
>>      r = -EINVAL;
>>      if (log->slot >= KVM_MEMORY_SLOTS)
>>          goto out;
>> @@ -450,17 +445,11 @@ static int kvm_vm_ioctl_get_dirty_log(struct kvm 
>> *kvm,
>>      if (copy_to_user(log->dirty_bitmap, memslot->dirty_bitmap, n))
>>          goto out;
>>
>> -    /* If nothing is dirty, don't bother messing with page tables. */
>> -    if (any) {
>> -        kvm_mmu_slot_remove_write_access(kvm, log->slot);
>> -        kvm_flush_remote_tlbs(kvm);
>> -        memset(memslot->dirty_bitmap, 0, n);
>> -    }
>> +    if (any)
>> +        *is_dirty = 1;
>>
>>      r = 0;
>> -
>>  out:
>> -    mutex_unlock(&kvm->lock);
>>      return r;
>>  }
>>
>> This split won't work on s390. I think kvm_get_dirty_log should be 
>> arch dependent alltogether: we're not going to have a dirty bitmap on 
>> s390, we rather rely on our hardware support to update our storage 
>> keys accordingly without guest/host intervention required. We'd want 
>> to use that, and thus this implementation of kvm_get_dirty_log makes 
>> no sense for us. On the other hand, I'd really want to keep the ioctl 
>> common, so that guest migration code in userland can be common for all 
>> archs.
> 
> On the other hand, it will work for all others IIUC.  Duplicating it in 
> all other archs is not a good idea.
> 
> We can special case it using #ifdef ARCH_HAS_KVM_GUEST_DIRTY_BITMAP or 
> something.
> 
> (hides from the #ifdef police)

Yea, I agree that it would make sense in case it works for power too. 
We could have an
#ifdef CONFIG_ARCH_S390
	return cool_big_iron_get_dirty_log(args);
#endif
at the beginning of that function. It would'nt hurt readability too much.

Hollis? Christian? How about ppc?

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