On Thu, Feb 06, 2020 at 05:03:55PM -0500, Peter Xu wrote:

On Tue, Jan 14, 2020 at 10:25:07AM -0800, Sean Christopherson wrote:

quoted

On Tue, Dec 24, 2019 at 01:19:30PM -0500, Peter Xu wrote:

quoted

On Tue, Dec 17, 2019 at 12:40:38PM -0800, Sean Christopherson wrote:

quoted

+int kvm_get_dirty_log(struct kvm *kvm, struct kvm_dirty_log *log,
+		      int *is_dirty, struct kvm_memory_slot **memslot)
 {
 	struct kvm_memslots *slots;
-	struct kvm_memory_slot *memslot;
 	int i, as_id, id;
 	unsigned long n;
 	unsigned long any = 0;
 
+	*memslot = NULL;
+	*is_dirty = 0;
+
 	as_id = log->slot >> 16;
 	id = (u16)log->slot;
 	if (as_id >= KVM_ADDRESS_SPACE_NUM || id >= KVM_USER_MEM_SLOTS)
 		return -EINVAL;
 
 	slots = __kvm_memslots(kvm, as_id);
-	memslot = id_to_memslot(slots, id);
-	if (!memslot->dirty_bitmap)
+	*memslot = id_to_memslot(slots, id);
+	if (!(*memslot)->dirty_bitmap)
 		return -ENOENT;
 
-	n = kvm_dirty_bitmap_bytes(memslot);
+	kvm_arch_sync_dirty_log(kvm, *memslot);

Should this line belong to previous patch?

No.

The previous patch, "KVM: Provide common implementation for generic dirty
log functions", is consolidating the implementation of dirty log functions
for architectures with CONFIG_KVM_GENERIC_DIRTYLOG_READ_PROTECT=y.

This code is being moved from s390's kvm_vm_ioctl_get_dirty_log(), as s390
doesn't select KVM_GENERIC_DIRTYLOG_READ_PROTECT.  It's functionally a nop
as kvm_arch_sync_dirty_log() is empty for PowerPC, the only other arch that
doesn't select KVM_GENERIC_DIRTYLOG_READ_PROTECT.

Arguably, the call to kvm_arch_sync_dirty_log() should be moved in a
separate prep patch.  It can't be a follow-on patch as that would swap the
ordering of kvm_arch_sync_dirty_log() and kvm_dirty_bitmap_bytes(), etc...

My reasoning for not splitting it to a separate patch is that prior to this
patch, the common code and arch specific code are doing separate memslot
lookups via id_to_memslot(), i.e. moving the kvm_arch_sync_dirty_log() call
would operate on a "different" memslot.   It can't actually be a different
memslot because slots_lock is held, it just felt weird.

All that being said, I don't have a strong opinion on moving the call to
kvm_arch_sync_dirty_log() in a separate patch; IIRC, I vascillated between
the two options when writing the code.  If anyone wants it to be a separate
patch I'll happily split it out.

(Sorry to respond so late)

I think the confusing part is the subject, where you only mentioned
the memslot change.  IMHO you can split the change to make it clearer,
or at least would you mind mention that kvm_arch_sync_dirty_log() move
in the commit message?  Thanks,

I'll add a few paragraphs to the changelog.  Splitting it out still feels
weird.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel