Re: [PATCH v4 10/26] KVM: Drop kvm_count_lock and instead protect kvm_usage_count with kvm_lock

[Chao Gao]

On Thu, Sep 08, 2022 at 04:25:26PM -0700, isaku.yamahata@xxxxxxxxx wrote:
>-
>-``kvm_count_lock``
>-^^^^^^^^^^^^^^^^^^
>-
>-:Type:		raw_spinlock_t
>-:Arch:		any
>-:Protects:	- hardware virtualization enable/disable
>-:Comment:	'raw' because hardware enabling/disabling must be atomic /wrt
>-		migration.
>+                - kvm_usage_count
>+                - hardware virtualization enable/disable
>+:Comment:	Use cpus_read_lock() for hardware virtualization enable/disable
>+                because hardware enabling/disabling must be atomic /wrt
>+                migration.  The lock order is cpus lock => kvm_lock.

Probably "/wrt CPU hotplug" is better.

> 
>@@ -5708,8 +5728,18 @@ static void kvm_init_debug(void)
> 
> static int kvm_suspend(void)
> {
>-	if (kvm_usage_count)
>+	/*
>+	 * The caller ensures that CPU hotlug is disabled by

					^hotplug

>+	 * cpu_hotplug_disable() and other CPUs are offlined.  No need for
>+	 * locking.
>+	 */
>+	lockdep_assert_not_held(&kvm_lock);
>+
>+	if (kvm_usage_count) {
>+		preempt_disable();
> 		hardware_disable_nolock(NULL);
>+		preempt_enable();

kvm_suspend() is called with interrupt disabled. So, no need to disable
preemption.

/**
 * syscore_suspend - Execute all the registered system core suspend callbacks.
 *
 * This function is executed with one CPU on-line and disabled interrupts.
 */
int syscore_suspend(void)


>+	}
> 	return 0;
> }
> 
>@@ -5723,8 +5753,10 @@ static void kvm_resume(void)
> 		return; /* FIXME: disable KVM */
> 
> 	if (kvm_usage_count) {
>-		lockdep_assert_not_held(&kvm_count_lock);
>+		lockdep_assert_not_held(&kvm_lock);
>+		preempt_disable();
> 		hardware_enable_nolock((void *)__func__);
>+		preempt_enable();

ditto.