Re: [PATCH v5 15/19] KVM: Provide common implementation for generic dirty log functions
From: Peter Xu <peterx@redhat.com>
Date: 2020-02-06 20:02:09
Also in:
kvm, kvmarm, linux-mips, lkml
On Tue, Jan 21, 2020 at 02:31:53PM -0800, Sean Christopherson wrote: [...]
-int kvm_vm_ioctl_clear_dirty_log(struct kvm *kvm, struct kvm_clear_dirty_log *log) +void kvm_arch_dirty_log_tlb_flush(struct kvm *kvm, + struct kvm_memory_slot *memslot)
If it's to flush TLB for a memslot, shall we remove the "dirty_log" in the name of the function, because it has nothing to do with dirty logging any more? And...
{
- struct kvm_memslots *slots;
- struct kvm_memory_slot *memslot;
- bool flush = false;
- int r;
-
- mutex_lock(&kvm->slots_lock);
-
- r = kvm_clear_dirty_log_protect(kvm, log, &flush);
-
- if (flush) {
- slots = kvm_memslots(kvm);
- memslot = id_to_memslot(slots, log->slot);
-
- /* Let implementation handle TLB/GVA invalidation */
- kvm_mips_callbacks->flush_shadow_memslot(kvm, memslot);
- }
-
- mutex_unlock(&kvm->slots_lock);
- return r;
+ /* Let implementation handle TLB/GVA invalidation */
+ kvm_mips_callbacks->flush_shadow_memslot(kvm, memslot);... This may not directly related to the current patch, but I'm confused on why MIPS cannot use kvm_flush_remote_tlbs() to flush TLBs. I know nothing about MIPS code, but IIUC here flush_shadow_memslot() is a heavier operation that will also invalidate the shadow pages. Seems to be an overkill here when we only changed write permission of the PTEs? I tried to check the first occurance (2a31b9db15353) but I didn't find out any clue of it so far. But that matters to this patch because if MIPS can use kvm_flush_remote_tlbs(), then we probably don't need this arch-specific hook any more and we can directly call kvm_flush_remote_tlbs() after sync dirty log when flush==true.
quoted hunk ↗ jump to hunk
} long kvm_arch_vm_ioctl(struct file *filp, unsigned int ioctl, unsigned long arg)diff --git a/arch/powerpc/kvm/book3s.c b/arch/powerpc/kvm/book3s.c index 97ce6c4f7b48..0adaf4791a6d 100644 --- a/arch/powerpc/kvm/book3s.c +++ b/arch/powerpc/kvm/book3s.c@@ -799,6 +799,11 @@ int kvmppc_core_check_requests(struct kvm_vcpu *vcpu) return vcpu->kvm->arch.kvm_ops->check_requests(vcpu); } +void kvm_arch_sync_dirty_log(struct kvm *kvm, struct kvm_memory_slot *memslot)
Since at it, maybe we can start to use __weak attribute for new hooks
especially when it's empty for most archs?
E.g., define:
void __weak kvm_arch_sync_dirty_log(...) {}
In the common code, then only define it again in arch that has
non-empty implementation of this method?
--
Peter Xu
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel