Messages in this thread | | | Subject | Re: [PATCH v3 07/11] KVM: page track: add notifier support | From | Xiao Guangrong <> | Date | Tue, 23 Feb 2016 12:34:20 +0800 |
| |
On 02/19/2016 07:51 PM, Paolo Bonzini wrote: > > > On 14/02/2016 12:31, Xiao Guangrong wrote:
>> +void kvm_page_track_write(struct kvm_vcpu *vcpu, gpa_t gpa, const u8 *new, >> + int bytes) >> +{ >> + struct kvm_page_track_notifier_head *head; >> + struct kvm_page_track_notifier_node *n; >> + int idx; >> + >> + head = &vcpu->kvm->arch.track_notifier_head; > > Please check outside SRCU if the notifier list is empty. If so, there > is no need to do the (relatively) expensive srcu_read_lock/unlock. >
Good to me. I will check it by calling hlist_empty() first before holding the srcu read lock.
> Paolo > >> + idx = srcu_read_lock(&head->track_srcu); >> + hlist_for_each_entry_rcu(n, &head->track_notifier_list, node) >> + if (n->track_write) >> + n->track_write(vcpu, gpa, new, bytes); >> + srcu_read_unlock(&head->track_srcu, idx); >> +} >> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >> index e25ebb7..98019b6 100644 >> --- a/arch/x86/kvm/x86.c >> +++ b/arch/x86/kvm/x86.c >> @@ -4370,6 +4370,7 @@ int emulator_write_phys(struct kvm_vcpu *vcpu, gpa_t gpa, >> if (ret < 0) >> return 0; > > A kvm_vcpu_mark_page_dirty is missing here, isn't it? I can take care > of it, but it would be great if you double-checked this. If so, that > should be fixed in stable kernels too.
No. It's already been handled in emulator_write_phys() -> kvm_vcpu_write_guest() -> kvm_vcpu_write_guest_page() -> __kvm_write_guest_page().
> > Can you add a kvm_vcpu_note_page_write(vcpu, gpa, val, bytes) function > that takes care of calling kvm_vcpu_mark_page_dirty, kvm_mmu_pte_write > and kvm_page_track-write? >
After this patchset, kvm_mmu_pte_write is only a static notifier callback called by kvm_page_track_write().
And the dirty tracking in emulator_write_phys() is handled in a public API (as my explanation above), in emulator_cmpxchg_emulated is handled by itself. So i think it is better to leaving dirty tracking to the separate paths, no? :)
| |