Re: [PATCH] Documentation: kvm: fix SRCU locking order docs

[Michal Luczaj <mhal@rbox.co>]

On 1/13/23 12:03, David Woodhouse wrote:
> On Fri, 2023-01-13 at 10:33 +0000, David Woodhouse wrote:
>> So everything seems to be working as it should... *except* for the fact
>> that I don't quite understand why xen_shinfo_test didn't trigger the
>> warning. Michal, I guess you already worked that out when you came up
>> with your deadlock-test instead... is there something we should add to
>> xen_shinfo_test that would mean it *would* have triggered?

No, I didn't implement those deadlock selftests out of xen_shinfo_test
because there was some problem. I just wanted to have a cleaner workspace
and then, maybe, move them to xen_shinfo_test, which, well, did not happen
:) I guess there's no need for them filthy races anymore; lockdep does a
better job.

> Got it. It only happens when kvm_xen_set_evtchn() takes the slow path
> when kvm_xen_set_evtchn_fast() fails.

I fully agree. And sorry for late reply.

> Not utterly sure why that works
> in your deadlock_test but I can make it happen in xen_shinfo_test just
> by invalidating the GPC by changing the memslots:

Could it be that deadlocks_test starts with the right conditions, i.e.
invalid KVM_XEN_ATTR_TYPE_SHARED_INFO along with valid
KVM_XEN_VCPU_ATTR_TYPE_VCPU_INFO? xen_shinfo_test, on the other hand, have
them both valid, and so the fast path is taken.

I suppose instead of changing memslots, you can invalidate the
KVM_XEN_ATTR_TYPE_SHARED_INFO for that particular test unit, e.g.

	struct kvm_xen_hvm_attr ha = {
		.type = KVM_XEN_ATTR_TYPE_SHARED_INFO,
		.u.shared_info.gfn = KVM_XEN_INVALID_GFN,
	};
	vm_ioctl(vm, KVM_XEN_HVM_SET_ATTR, &ha);

One more thing concerning the lockdep priming you did in kvm_create_vm();

	mutex_lock(&kvm->lock);
	synchronize_srcu(&kvm->srcu);
	mutex_unlock(&kvm->lock)

It seems that deadlocks_test's set_msr_filter() effectively did the same
thanks to kvm_vm_ioctl_set_msr_filter()'s sync-under-mutex (which won't
happen if those I-used-to-be-a-deadlock optimization patches[*] get
merged). Naturally, xen_shinfo_test do not mess with MSR filters, so that
could be another reason for inconsistencies you've noticed before the
priming?

[*] https://lore.kernel.org/kvm/20230107001256.2365304-1-mhal@rbox.co/

Michal