Re: [PATCH v2 3/4] KVM: cleanup: remove kvm_get_dirty_log()

On 06/23/2010 12:01 PM, Takuya Yoshikawa wrote:
> (2010/06/23 17:48), Avi Kivity wrote:
>
>>> diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c
>>> index 801d9f3..bea6f7c 100644
>>> --- a/arch/powerpc/kvm/book3s.c
>>> +++ b/arch/powerpc/kvm/book3s.c
>>> @@ -1185,28 +1185,43 @@ int kvm_vm_ioctl_get_dirty_log(struct kvm *kvm,
>>> struct kvm_memory_slot *memslot;
>>> struct kvm_vcpu *vcpu;
>>> ulong ga, ga_end;
>>> - int is_dirty = 0;
>>> - int r;
>>> + unsigned long is_dirty = 0;
>>> + int r, i;
>>> unsigned long n;
>>>
>>> mutex_lock(&kvm->slots_lock);
>>>
>>> - r = kvm_get_dirty_log(kvm, log,&is_dirty);
>>> - if (r)
>>> + r = -EINVAL;
>>> + if (log->slot>= KVM_MEMORY_SLOTS)
>>> + goto out;
>>> +
>>> + memslot =&kvm->memslots->memslots[log->slot];
>>
>> Not introduced by this patch, but shouldn't this use rcu_dereference()?
>>
>>
>
> I was thinking like that, but sorry I don't know well about ppc.
>
> My final goal is to make everything except
>
>   /* If nothing is dirty, don't bother messing with page tables. */
>
> part arch independent, so that we can concentrate on the truely
> dirty logging things in the future.
>
> So, making ppc code same as x86, rcu_dereference() for example , really
> helps me.
>
>
> Do you like this approach?

Well yes, I expect that not using rcu_dereference() (and 
srcu_read_lock()) is a bug here.

Marcelo?

On a related note, I'd like to consolidate rcu locking:

- all vcpu ioctls take srcu_read_lock() when they start and drop it at 
the end.
- KVM_VCPU_RUN drops the lock when sleeping and entering the guest (also 
on context switch?)

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