Re: [PATCH v3 1/2] KVM: Disable IRQs in kvm_online_cpu()/kvm_offline_cpu()

["Huang, Kai" <kai.huang@intel.com>]

On Tue, 2025-09-16 at 14:07 +0800, Hou Wenlong wrote:
> After the commit aaf12a7b4323 ("KVM: Rename and move
> CPUHP_AP_KVM_STARTING to ONLINE section"), KVM's hotplug callbacks have
> been moved into the ONLINE section, where IRQs and preemption are
> enabled according to the documentation. However, if IRQs are not
> guaranteed to be disabled, it could theoretically be a bug, because
> virtualization_enabled may be stale (with respect to the actual state of
> the hardware) when read from IRQ context, making the callback
> potentially reentrant. Therefore, disable IRQs in kvm_online_cpu() and
> kvm_offline_cpu() to ensure that all paths for
> kvm_enable_virtualization_cpu() and kvm_disable_virtualization_cpu() are
> in an IRQ-disabled state.

Reading the v1 thread [*], IIUC the "virtualization_enabled being stale
when read from IRQ context" is referring to the case where
kvm_disable_virtualization_cpu() got interrupted by IRQ and re-entered.

But IIUC this shouldn't happen.  If I am not missing anything, the
syscore_shutdown() (from which KVM sends IRQ to call
kvm_disable_virtualization_cpu()) is always called after
migrate_to_reboot_cpu(), which internally waits for currently running CPU
hotplug to complete (if any) and disables future CPU hotplug.  Therefore
it shouldn't be possible that kvm_disable_virtualization_cpu() could be
interrupted and re-entered via IRQ.

I don't oppose this code change, but I think this should somehow
documented in the changelog, if I am not missing anything?

[*]: https://lore.kernel.org/kvm/aMirvo9Xly5fVmbY@google.com/
> 
> Suggested-by: Sean Christopherson <seanjc@google.com>
> Signed-off-by: Hou Wenlong <houwenlong.hwl@antgroup.com>
> ---
>  virt/kvm/kvm_main.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 18f29ef93543..cf8dddeed37e 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -5580,6 +5580,8 @@ __weak void kvm_arch_disable_virtualization(void)
>  
>  static int kvm_enable_virtualization_cpu(void)
>  {
> +	lockdep_assert_irqs_disabled();
> +
>  	if (__this_cpu_read(virtualization_enabled))
>  		return 0;
>  
> @@ -5595,6 +5597,8 @@ static int kvm_enable_virtualization_cpu(void)
>  
>  static int kvm_online_cpu(unsigned int cpu)
>  {
> +	guard(irqsave)();
> +
>  	/*
>  	 * Abort the CPU online process if hardware virtualization cannot
>  	 * be enabled. Otherwise running VMs would encounter unrecoverable
> @@ -5605,6 +5609,8 @@ static int kvm_online_cpu(unsigned int cpu)
>  
>  static void kvm_disable_virtualization_cpu(void *ign)
>  {
> +	lockdep_assert_irqs_disabled();
> +
>  	if (!__this_cpu_read(virtualization_enabled))
>  		return;
>  
> @@ -5615,6 +5621,8 @@ static void kvm_disable_virtualization_cpu(void *ign)
>  
>  static int kvm_offline_cpu(unsigned int cpu)
>  {
> +	guard(irqsave)();
> +
>  	kvm_disable_virtualization_cpu(NULL);
>  	return 0;
>  }
> @@ -5648,7 +5656,6 @@ static int kvm_suspend(void)
>  	 * dropped all locks (userspace tasks are frozen via a fake signal).
>  	 */
>  	lockdep_assert_not_held(&kvm_usage_lock);
> -	lockdep_assert_irqs_disabled();
>  
>  	kvm_disable_virtualization_cpu(NULL);
>  	return 0;
> @@ -5657,7 +5664,6 @@ static int kvm_suspend(void)
>  static void kvm_resume(void)
>  {
>  	lockdep_assert_not_held(&kvm_usage_lock);
> -	lockdep_assert_irqs_disabled();
>  
>  	WARN_ON_ONCE(kvm_enable_virtualization_cpu());
>  }
> 
> base-commit: a6ad54137af92535cfe32e19e5f3bc1bb7dbd383