Re: [PATCH 5/5] KVM: VMX: Handle NMIs, #MCs and async #PFs in common irqs-disabled fn

From: Sean Christopherson <sean.j.christopherson@intel.com>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: "Radim Krčmář" <rkrcmar@redhat.com>,
	"Joerg Roedel" <joro@8bytes.org>,
	kvm@vger.kernel.org, "Jim Mattson" <jmattson@google.com>
Subject: Re: [PATCH 5/5] KVM: VMX: Handle NMIs, #MCs and async #PFs in common irqs-disabled fn
Date: Thu, 6 Jun 2019 08:14:20 -0700	[thread overview]
Message-ID: <20190606151420.GB23169@linux.intel.com> (raw)
In-Reply-To: <746c7e2c-176f-d772-e37d-41bb9f524dd6@redhat.com>

On Thu, Jun 06, 2019 at 03:20:49PM +0200, Paolo Bonzini wrote:
> On 20/04/19 07:50, Sean Christopherson wrote:
> > Per commit 1b6269db3f833 ("KVM: VMX: Handle NMIs before enabling
> > interrupts and preemption"), NMIs are handled directly in vmx_vcpu_run()
> > to "make sure we handle NMI on the current cpu, and that we don't
> > service maskable interrupts before non-maskable ones".  The other
> > exceptions handled by complete_atomic_exit(), e.g. async #PF and #MC,
> > have similar requirements, and are located there to avoid extra VMREADs
> > since VMX bins hardware exceptions and NMIs into a single exit reason.
> > 
> > Clean up the code and eliminate the vaguely named complete_atomic_exit()
> > by moving the interrupts-disabled exception and NMI handling into the
> > existing handle_external_intrs() callback, and rename the callback to
> > a more appropriate name.
> > 
> > In addition to improving code readability, this also ensures the NMI
> > handler is run with the host's debug registers loaded in the unlikely
> > event that the user is debugging NMIs.  Accuracy of the last_guest_tsc
> > field is also improved when handling NMIs (and #MCs) as the handler
> > will run after updating said field.
> > 
> > Signed-off-by: Sean Christopherson <sean.j.christopherson@intel.com>
> 
> Very nice, just some changes I'd like to propose. "atomic" is Linux 
> lingo for "irqs disabled", so I'd like to rename the handler to 

The code disagrees, e.g.

  /*
   * Are we running in atomic context?  WARNING: this macro cannot
   * always detect atomic context; in particular, it cannot know about
   * held spinlocks in non-preemptible kernels.  Thus it should not be
   * used in the general case to determine whether sleeping is possible.
   * Do not use in_atomic() in driver code.
   */
  #define in_atomic()	(preempt_count() != 0)

and

  void ___might_sleep(...)
  {
	...

	printk(KERN_ERR
		"in_atomic(): %d, irqs_disabled(): %d, pid: %d, name: %s\n",
			in_atomic(), irqs_disabled(),
			current->pid, current->comm);
  }

and

  static inline void *kmap_atomic(struct page *page)
  {
	preempt_disable();
	pagefault_disable();
	return page_address(page);
  }

My interpretation of things is that the kernel's definition of an atomic
context is with respect to preemption.  Disabling IRQs would also provide
atomicity, but the reverse is not true, i.e. entering an atomic context
does not imply IRQs are disabled.

As it pertains to KVM, we specifically care about IRQs being disabled,
e.g. VMX needs to ensure #MC and NMI are handled before any pending IRQs,
and both VMX and SVM need to ensure a pending perf interrupt is handled
in the callback.

And if "atomic" is interpreted as "IRQs disabled", one could argue that
the SVM behavior is buggy since enabling IRQs would break atomicity.

> handle_exit_atomic so it has a correspondance with handle_exit.  
> Likewise we could have handle_exception_nmi_atomic and 
> handle_external_interrupt_atomic.