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

[Marcelo Tosatti <mtosatti@redhat.com>]

On Wed, Jun 23, 2010 at 12:00:47PM +0300, Avi Kivity wrote:
> 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?

No because slots_lock is held which serializes with memslot updates.

> 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?)

Yep, makes sense.