public inbox for kvm@vger.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/4] KVM: Clean up "preempted in-kernel" logic
@ 2024-01-10  0:39 Sean Christopherson
  2024-01-10  0:39 ` [PATCH 1/4] KVM: Add dedicated arch hook for querying if vCPU was preempted in-kernel Sean Christopherson
                   ` (4 more replies)
  0 siblings, 5 replies; 11+ messages in thread
From: Sean Christopherson @ 2024-01-10  0:39 UTC (permalink / raw)
  To: Sean Christopherson, Paolo Bonzini; +Cc: kvm, linux-kernel, Like Xu

Provide a dedicated helper to query if a *different* vCPU was preempted
in-kernel.  x86's VMX is an oddball and can only check if the vCPU is in
kernel (versus userspace) if the vCPU is loaded on the current pCPU.

The existing kvm_arch_vcpu_in_kernel() "works", but it's an ugly mess as
KVM x86 is forced to check kvm_get_running_vcpu() to effectively undo the
multiplexing.

Note, I was sorely tempted to eliminate kvm_arch_dy_has_pending_interrupt()
and bury that logic in VMX code, but I ultimately decided to keep it as an
arch hook as it's entirely plausible that some other architecture can do
cross-vCPU IRQ checks, and if KVM is going to have the (somewhat dubious
IMO) logic, it's probably best to keep it in common code.

Sean Christopherson (4):
  KVM: Add dedicated arch hook for querying if vCPU was preempted
    in-kernel
  KVM: x86: Rely solely on preempted_in_kernel flag for directed yield
  KVM: x86: Clean up directed yield API for "has pending interrupt"
  KVM: Add a comment explaining the directed yield pending interrupt
    logic

 arch/x86/kvm/x86.c       | 16 +++++++---------
 include/linux/kvm_host.h |  1 +
 virt/kvm/kvm_main.c      | 22 ++++++++++++++++++++--
 3 files changed, 28 insertions(+), 11 deletions(-)


base-commit: 1c6d984f523f67ecfad1083bb04c55d91977bb15
-- 
2.43.0.472.g3155946c3a-goog


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/4] KVM: Add dedicated arch hook for querying if vCPU was preempted in-kernel
  2024-01-10  0:39 [PATCH 0/4] KVM: Clean up "preempted in-kernel" logic Sean Christopherson
@ 2024-01-10  0:39 ` Sean Christopherson
  2024-01-11 12:48   ` Yuan Yao
  2024-01-10  0:39 ` [PATCH 2/4] KVM: x86: Rely solely on preempted_in_kernel flag for directed yield Sean Christopherson
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 11+ messages in thread
From: Sean Christopherson @ 2024-01-10  0:39 UTC (permalink / raw)
  To: Sean Christopherson, Paolo Bonzini; +Cc: kvm, linux-kernel, Like Xu

Plumb in a dedicated hook for querying whether or not a vCPU was preempted
in-kernel.  Unlike literally every other architecture, x86's VMX can check
if a vCPU is in kernel context if and only if the vCPU is loaded on the
current pCPU.

x86's kvm_arch_vcpu_in_kernel() works around the limitation by querying
kvm_get_running_vcpu() and redirecting to vcpu->arch.preempted_in_kernel
as needed.  But that's unnecessary, confusing, and fragile, e.g. x86 has
had at least one bug where KVM incorrectly used a stale
preempted_in_kernel.

No functional change intended.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/x86.c       |  5 +++++
 include/linux/kvm_host.h |  1 +
 virt/kvm/kvm_main.c      | 15 +++++++++++++--
 3 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 27e23714e960..415509918c7f 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -13091,6 +13091,11 @@ bool kvm_arch_dy_has_pending_interrupt(struct kvm_vcpu *vcpu)
 	return false;
 }
 
+bool kvm_arch_vcpu_preempted_in_kernel(struct kvm_vcpu *vcpu)
+{
+	return kvm_arch_vcpu_in_kernel(vcpu);
+}
+
 bool kvm_arch_dy_runnable(struct kvm_vcpu *vcpu)
 {
 	if (READ_ONCE(vcpu->arch.pv.pv_unhalted))
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index 7e7fd25b09b3..28b020404a41 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -1505,6 +1505,7 @@ bool kvm_arch_vcpu_in_kernel(struct kvm_vcpu *vcpu);
 int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu);
 bool kvm_arch_dy_runnable(struct kvm_vcpu *vcpu);
 bool kvm_arch_dy_has_pending_interrupt(struct kvm_vcpu *vcpu);
+bool kvm_arch_vcpu_preempted_in_kernel(struct kvm_vcpu *vcpu);
 int kvm_arch_post_init_vm(struct kvm *kvm);
 void kvm_arch_pre_destroy_vm(struct kvm *kvm);
 int kvm_arch_create_vm_debugfs(struct kvm *kvm);
diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 10bfc88a69f7..6326852bfb3d 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -4042,11 +4042,22 @@ static bool vcpu_dy_runnable(struct kvm_vcpu *vcpu)
 	return false;
 }
 
+/*
+ * By default, simply query the target vCPU's current mode when checking if a
+ * vCPU was preempted in kernel mode.  All architectures except x86 (or more
+ * specifical, except VMX) allow querying whether or not a vCPU is in kernel
+ * mode even if the vCPU is NOT loaded, i.e. using kvm_arch_vcpu_in_kernel()
+ * directly for cross-vCPU checks is functionally correct and accurate.
+ */
+bool __weak kvm_arch_vcpu_preempted_in_kernel(struct kvm_vcpu *vcpu)
+{
+	return kvm_arch_vcpu_in_kernel(vcpu);
+}
+
 bool __weak kvm_arch_dy_has_pending_interrupt(struct kvm_vcpu *vcpu)
 {
 	return false;
 }
-
 void kvm_vcpu_on_spin(struct kvm_vcpu *me, bool yield_to_kernel_mode)
 {
 	struct kvm *kvm = me->kvm;
@@ -4080,7 +4091,7 @@ void kvm_vcpu_on_spin(struct kvm_vcpu *me, bool yield_to_kernel_mode)
 				continue;
 			if (READ_ONCE(vcpu->preempted) && yield_to_kernel_mode &&
 			    !kvm_arch_dy_has_pending_interrupt(vcpu) &&
-			    !kvm_arch_vcpu_in_kernel(vcpu))
+			    !kvm_arch_vcpu_preempted_in_kernel(vcpu))
 				continue;
 			if (!kvm_vcpu_eligible_for_directed_yield(vcpu))
 				continue;
-- 
2.43.0.472.g3155946c3a-goog


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/4] KVM: x86: Rely solely on preempted_in_kernel flag for directed yield
  2024-01-10  0:39 [PATCH 0/4] KVM: Clean up "preempted in-kernel" logic Sean Christopherson
  2024-01-10  0:39 ` [PATCH 1/4] KVM: Add dedicated arch hook for querying if vCPU was preempted in-kernel Sean Christopherson
@ 2024-01-10  0:39 ` Sean Christopherson
  2024-01-10  7:55   ` Yuan Yao
  2024-01-10  0:39 ` [PATCH 3/4] KVM: x86: Clean up directed yield API for "has pending interrupt" Sean Christopherson
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 11+ messages in thread
From: Sean Christopherson @ 2024-01-10  0:39 UTC (permalink / raw)
  To: Sean Christopherson, Paolo Bonzini; +Cc: kvm, linux-kernel, Like Xu

Snapshot preempted_in_kernel using kvm_arch_vcpu_in_kernel() so that the
flag is "accurate" (or rather, consistent and deterministic within KVM)
for guest with protected state, and explicitly use preempted_in_kernel
when checking if a vCPU was preempted in kernel mode instead of bouncing
through kvm_arch_vcpu_in_kernel().

Drop the gnarly logic in kvm_arch_vcpu_in_kernel() that redirects to
preempted_in_kernel if the target vCPU is not the "running", i.e. loaded,
vCPU, as the only reason that code existed was for the directed yield case
where KVM wants to check the CPL of a vCPU that may or may not be loaded
on the current pCPU.

Cc: Like Xu <like.xu.linux@gmail.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/x86.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 415509918c7f..77494f9c8d49 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -5062,8 +5062,7 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
 	int idx;
 
 	if (vcpu->preempted) {
-		if (!vcpu->arch.guest_state_protected)
-			vcpu->arch.preempted_in_kernel = !static_call(kvm_x86_get_cpl)(vcpu);
+		vcpu->arch.preempted_in_kernel = kvm_arch_vcpu_in_kernel(vcpu);
 
 		/*
 		 * Take the srcu lock as memslots will be accessed to check the gfn
@@ -13093,7 +13092,7 @@ bool kvm_arch_dy_has_pending_interrupt(struct kvm_vcpu *vcpu)
 
 bool kvm_arch_vcpu_preempted_in_kernel(struct kvm_vcpu *vcpu)
 {
-	return kvm_arch_vcpu_in_kernel(vcpu);
+	return vcpu->arch.preempted_in_kernel;
 }
 
 bool kvm_arch_dy_runnable(struct kvm_vcpu *vcpu)
@@ -13116,9 +13115,6 @@ bool kvm_arch_vcpu_in_kernel(struct kvm_vcpu *vcpu)
 	if (vcpu->arch.guest_state_protected)
 		return true;
 
-	if (vcpu != kvm_get_running_vcpu())
-		return vcpu->arch.preempted_in_kernel;
-
 	return static_call(kvm_x86_get_cpl)(vcpu) == 0;
 }
 
-- 
2.43.0.472.g3155946c3a-goog


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 3/4] KVM: x86: Clean up directed yield API for "has pending interrupt"
  2024-01-10  0:39 [PATCH 0/4] KVM: Clean up "preempted in-kernel" logic Sean Christopherson
  2024-01-10  0:39 ` [PATCH 1/4] KVM: Add dedicated arch hook for querying if vCPU was preempted in-kernel Sean Christopherson
  2024-01-10  0:39 ` [PATCH 2/4] KVM: x86: Rely solely on preempted_in_kernel flag for directed yield Sean Christopherson
@ 2024-01-10  0:39 ` Sean Christopherson
  2024-01-11 12:49   ` Yuan Yao
  2024-01-10  0:39 ` [PATCH 4/4] KVM: Add a comment explaining the directed yield pending interrupt logic Sean Christopherson
  2024-02-23  1:35 ` [PATCH 0/4] KVM: Clean up "preempted in-kernel" logic Sean Christopherson
  4 siblings, 1 reply; 11+ messages in thread
From: Sean Christopherson @ 2024-01-10  0:39 UTC (permalink / raw)
  To: Sean Christopherson, Paolo Bonzini; +Cc: kvm, linux-kernel, Like Xu

Directly return the boolean result of whether or not a vCPU has a pending
interrupt instead of effectively doing:

  if (true)
	return true;

  return false;

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/kvm/x86.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 77494f9c8d49..b7996a75d9a3 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -13083,11 +13083,8 @@ int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu)
 
 bool kvm_arch_dy_has_pending_interrupt(struct kvm_vcpu *vcpu)
 {
-	if (kvm_vcpu_apicv_active(vcpu) &&
-	    static_call(kvm_x86_dy_apicv_has_pending_interrupt)(vcpu))
-		return true;
-
-	return false;
+	return kvm_vcpu_apicv_active(vcpu) &&
+	       static_call(kvm_x86_dy_apicv_has_pending_interrupt)(vcpu);
 }
 
 bool kvm_arch_vcpu_preempted_in_kernel(struct kvm_vcpu *vcpu)
-- 
2.43.0.472.g3155946c3a-goog


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 4/4] KVM: Add a comment explaining the directed yield pending interrupt logic
  2024-01-10  0:39 [PATCH 0/4] KVM: Clean up "preempted in-kernel" logic Sean Christopherson
                   ` (2 preceding siblings ...)
  2024-01-10  0:39 ` [PATCH 3/4] KVM: x86: Clean up directed yield API for "has pending interrupt" Sean Christopherson
@ 2024-01-10  0:39 ` Sean Christopherson
  2024-02-23  1:35 ` [PATCH 0/4] KVM: Clean up "preempted in-kernel" logic Sean Christopherson
  4 siblings, 0 replies; 11+ messages in thread
From: Sean Christopherson @ 2024-01-10  0:39 UTC (permalink / raw)
  To: Sean Christopherson, Paolo Bonzini; +Cc: kvm, linux-kernel, Like Xu

Add a comment to explain why KVM treats vCPUs with pending interrupts as
in-kernel when a vCPU wants to yield to a vCPU that was preempted while
running in kernel mode.

Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 virt/kvm/kvm_main.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
index 6326852bfb3d..4a9e7513c585 100644
--- a/virt/kvm/kvm_main.c
+++ b/virt/kvm/kvm_main.c
@@ -4089,6 +4089,13 @@ void kvm_vcpu_on_spin(struct kvm_vcpu *me, bool yield_to_kernel_mode)
 				continue;
 			if (kvm_vcpu_is_blocking(vcpu) && !vcpu_dy_runnable(vcpu))
 				continue;
+
+			/*
+			 * Treat the target vCPU as being in-kernel if it has a
+			 * pending interrupt, as the vCPU trying to yield may
+			 * be spinning waiting on IPI delivery, i.e. the target
+			 * vCPU is in-kernel for the purposes of directed yield.
+			 */
 			if (READ_ONCE(vcpu->preempted) && yield_to_kernel_mode &&
 			    !kvm_arch_dy_has_pending_interrupt(vcpu) &&
 			    !kvm_arch_vcpu_preempted_in_kernel(vcpu))
-- 
2.43.0.472.g3155946c3a-goog


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/4] KVM: x86: Rely solely on preempted_in_kernel flag for directed yield
  2024-01-10  0:39 ` [PATCH 2/4] KVM: x86: Rely solely on preempted_in_kernel flag for directed yield Sean Christopherson
@ 2024-01-10  7:55   ` Yuan Yao
  2024-01-10 17:13     ` Sean Christopherson
  0 siblings, 1 reply; 11+ messages in thread
From: Yuan Yao @ 2024-01-10  7:55 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: Paolo Bonzini, kvm, linux-kernel, Like Xu

On Tue, Jan 09, 2024 at 04:39:36PM -0800, Sean Christopherson wrote:
> Snapshot preempted_in_kernel using kvm_arch_vcpu_in_kernel() so that the
> flag is "accurate" (or rather, consistent and deterministic within KVM)
> for guest with protected state, and explicitly use preempted_in_kernel
> when checking if a vCPU was preempted in kernel mode instead of bouncing
> through kvm_arch_vcpu_in_kernel().
>
> Drop the gnarly logic in kvm_arch_vcpu_in_kernel() that redirects to
> preempted_in_kernel if the target vCPU is not the "running", i.e. loaded,
> vCPU, as the only reason that code existed was for the directed yield case
> where KVM wants to check the CPL of a vCPU that may or may not be loaded
> on the current pCPU.
>
> Cc: Like Xu <like.xu.linux@gmail.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>  arch/x86/kvm/x86.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 415509918c7f..77494f9c8d49 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -5062,8 +5062,7 @@ void kvm_arch_vcpu_put(struct kvm_vcpu *vcpu)
>  	int idx;
>
>  	if (vcpu->preempted) {
> -		if (!vcpu->arch.guest_state_protected)
> -			vcpu->arch.preempted_in_kernel = !static_call(kvm_x86_get_cpl)(vcpu);
> +		vcpu->arch.preempted_in_kernel = kvm_arch_vcpu_in_kernel(vcpu);
>
>  		/*
>  		 * Take the srcu lock as memslots will be accessed to check the gfn
> @@ -13093,7 +13092,7 @@ bool kvm_arch_dy_has_pending_interrupt(struct kvm_vcpu *vcpu)
>
>  bool kvm_arch_vcpu_preempted_in_kernel(struct kvm_vcpu *vcpu)
>  {
> -	return kvm_arch_vcpu_in_kernel(vcpu);
> +	return vcpu->arch.preempted_in_kernel;
>  }
>
>  bool kvm_arch_dy_runnable(struct kvm_vcpu *vcpu)
> @@ -13116,9 +13115,6 @@ bool kvm_arch_vcpu_in_kernel(struct kvm_vcpu *vcpu)
>  	if (vcpu->arch.guest_state_protected)
>  		return true;
>
> -	if (vcpu != kvm_get_running_vcpu())
> -		return vcpu->arch.preempted_in_kernel;
> -

Now this function accepts vcpu parameter but can only get
information from "current" vcpu loaded on hardware for VMX.
I'm not sure whether need "WARN_ON(vcpu != kvm_get_running_vcpu())"
here to guard it. i.e. kvm_guest_state() still
uses this function (although it did chekcing before).

>  	return static_call(kvm_x86_get_cpl)(vcpu) == 0;
>  }
>
> --
> 2.43.0.472.g3155946c3a-goog
>
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/4] KVM: x86: Rely solely on preempted_in_kernel flag for directed yield
  2024-01-10  7:55   ` Yuan Yao
@ 2024-01-10 17:13     ` Sean Christopherson
  2024-01-11 12:47       ` Yuan Yao
  0 siblings, 1 reply; 11+ messages in thread
From: Sean Christopherson @ 2024-01-10 17:13 UTC (permalink / raw)
  To: Yuan Yao; +Cc: Paolo Bonzini, kvm, linux-kernel, Like Xu

On Wed, Jan 10, 2024, Yuan Yao wrote:
> On Tue, Jan 09, 2024 at 04:39:36PM -0800, Sean Christopherson wrote:
> > @@ -13093,7 +13092,7 @@ bool kvm_arch_dy_has_pending_interrupt(struct kvm_vcpu *vcpu)
> >
> >  bool kvm_arch_vcpu_preempted_in_kernel(struct kvm_vcpu *vcpu)
> >  {
> > -	return kvm_arch_vcpu_in_kernel(vcpu);
> > +	return vcpu->arch.preempted_in_kernel;
> >  }
> >
> >  bool kvm_arch_dy_runnable(struct kvm_vcpu *vcpu)
> > @@ -13116,9 +13115,6 @@ bool kvm_arch_vcpu_in_kernel(struct kvm_vcpu *vcpu)
> >  	if (vcpu->arch.guest_state_protected)
> >  		return true;
> >
> > -	if (vcpu != kvm_get_running_vcpu())
> > -		return vcpu->arch.preempted_in_kernel;
> > -
> 
> Now this function accepts vcpu parameter but can only get information from
> "current" vcpu loaded on hardware for VMX.  I'm not sure whether need
> "WARN_ON(vcpu != kvm_get_running_vcpu())" here to guard it. i.e.
> kvm_guest_state() still uses this function (although it did chekcing before).

Eh, I don't think it's worth adding a one-off kvm_get_running_vcpu() sanity check.
In the vast majority of cases, if VMREAD or VMWRITE is used improperly, the
instruction will fail at some point due to the pCPU not having any VMCS loaded.
It's really just cross-vCPU checks that could silently do the wrong thing, and
those flows are so few and far between that I'm comfortable taking a "just get
it right stance".

If we want to add sanity checks, I think my vote would be to plumb @vcpu down
into vmcs_read{16,32,64,l} and add sanity checks there, probably with some sort
of guard so that the sanity checks can be enabled only for debug kernels.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/4] KVM: x86: Rely solely on preempted_in_kernel flag for directed yield
  2024-01-10 17:13     ` Sean Christopherson
@ 2024-01-11 12:47       ` Yuan Yao
  0 siblings, 0 replies; 11+ messages in thread
From: Yuan Yao @ 2024-01-11 12:47 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: Paolo Bonzini, kvm, linux-kernel, Like Xu

On Wed, Jan 10, 2024 at 09:13:28AM -0800, Sean Christopherson wrote:
> On Wed, Jan 10, 2024, Yuan Yao wrote:
> > On Tue, Jan 09, 2024 at 04:39:36PM -0800, Sean Christopherson wrote:
> > > @@ -13093,7 +13092,7 @@ bool kvm_arch_dy_has_pending_interrupt(struct kvm_vcpu *vcpu)
> > >
> > >  bool kvm_arch_vcpu_preempted_in_kernel(struct kvm_vcpu *vcpu)
> > >  {
> > > -	return kvm_arch_vcpu_in_kernel(vcpu);
> > > +	return vcpu->arch.preempted_in_kernel;
> > >  }
> > >
> > >  bool kvm_arch_dy_runnable(struct kvm_vcpu *vcpu)
> > > @@ -13116,9 +13115,6 @@ bool kvm_arch_vcpu_in_kernel(struct kvm_vcpu *vcpu)
> > >  	if (vcpu->arch.guest_state_protected)
> > >  		return true;
> > >
> > > -	if (vcpu != kvm_get_running_vcpu())
> > > -		return vcpu->arch.preempted_in_kernel;
> > > -
> >
> > Now this function accepts vcpu parameter but can only get information from
> > "current" vcpu loaded on hardware for VMX.  I'm not sure whether need
> > "WARN_ON(vcpu != kvm_get_running_vcpu())" here to guard it. i.e.
> > kvm_guest_state() still uses this function (although it did chekcing before).
>
> Eh, I don't think it's worth adding a one-off kvm_get_running_vcpu() sanity check.
> In the vast majority of cases, if VMREAD or VMWRITE is used improperly, the
> instruction will fail at some point due to the pCPU not having any VMCS loaded.
> It's really just cross-vCPU checks that could silently do the wrong thing, and
> those flows are so few and far between that I'm comfortable taking a "just get
> it right stance".
>
> If we want to add sanity checks, I think my vote would be to plumb @vcpu down
> into vmcs_read{16,32,64,l} and add sanity checks there, probably with some sort
> of guard so that the sanity checks can be enabled only for debug kernels.

I got your point.

Reviewed-by: Yuan Yao <yuan.yao@intel.com>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/4] KVM: Add dedicated arch hook for querying if vCPU was preempted in-kernel
  2024-01-10  0:39 ` [PATCH 1/4] KVM: Add dedicated arch hook for querying if vCPU was preempted in-kernel Sean Christopherson
@ 2024-01-11 12:48   ` Yuan Yao
  0 siblings, 0 replies; 11+ messages in thread
From: Yuan Yao @ 2024-01-11 12:48 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: Paolo Bonzini, kvm, linux-kernel, Like Xu

On Tue, Jan 09, 2024 at 04:39:35PM -0800, Sean Christopherson wrote:
> Plumb in a dedicated hook for querying whether or not a vCPU was preempted
> in-kernel.  Unlike literally every other architecture, x86's VMX can check
> if a vCPU is in kernel context if and only if the vCPU is loaded on the
> current pCPU.
>
> x86's kvm_arch_vcpu_in_kernel() works around the limitation by querying
> kvm_get_running_vcpu() and redirecting to vcpu->arch.preempted_in_kernel
> as needed.  But that's unnecessary, confusing, and fragile, e.g. x86 has
> had at least one bug where KVM incorrectly used a stale
> preempted_in_kernel.
>
> No functional change intended.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>

Reviewed-by: Yuan Yao <yuan.yao@intel.com>

> ---
>  arch/x86/kvm/x86.c       |  5 +++++
>  include/linux/kvm_host.h |  1 +
>  virt/kvm/kvm_main.c      | 15 +++++++++++++--
>  3 files changed, 19 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 27e23714e960..415509918c7f 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -13091,6 +13091,11 @@ bool kvm_arch_dy_has_pending_interrupt(struct kvm_vcpu *vcpu)
>  	return false;
>  }
>
> +bool kvm_arch_vcpu_preempted_in_kernel(struct kvm_vcpu *vcpu)
> +{
> +	return kvm_arch_vcpu_in_kernel(vcpu);
> +}
> +
>  bool kvm_arch_dy_runnable(struct kvm_vcpu *vcpu)
>  {
>  	if (READ_ONCE(vcpu->arch.pv.pv_unhalted))
> diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
> index 7e7fd25b09b3..28b020404a41 100644
> --- a/include/linux/kvm_host.h
> +++ b/include/linux/kvm_host.h
> @@ -1505,6 +1505,7 @@ bool kvm_arch_vcpu_in_kernel(struct kvm_vcpu *vcpu);
>  int kvm_arch_vcpu_should_kick(struct kvm_vcpu *vcpu);
>  bool kvm_arch_dy_runnable(struct kvm_vcpu *vcpu);
>  bool kvm_arch_dy_has_pending_interrupt(struct kvm_vcpu *vcpu);
> +bool kvm_arch_vcpu_preempted_in_kernel(struct kvm_vcpu *vcpu);
>  int kvm_arch_post_init_vm(struct kvm *kvm);
>  void kvm_arch_pre_destroy_vm(struct kvm *kvm);
>  int kvm_arch_create_vm_debugfs(struct kvm *kvm);
> diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c
> index 10bfc88a69f7..6326852bfb3d 100644
> --- a/virt/kvm/kvm_main.c
> +++ b/virt/kvm/kvm_main.c
> @@ -4042,11 +4042,22 @@ static bool vcpu_dy_runnable(struct kvm_vcpu *vcpu)
>  	return false;
>  }
>
> +/*
> + * By default, simply query the target vCPU's current mode when checking if a
> + * vCPU was preempted in kernel mode.  All architectures except x86 (or more
> + * specifical, except VMX) allow querying whether or not a vCPU is in kernel
> + * mode even if the vCPU is NOT loaded, i.e. using kvm_arch_vcpu_in_kernel()
> + * directly for cross-vCPU checks is functionally correct and accurate.
> + */
> +bool __weak kvm_arch_vcpu_preempted_in_kernel(struct kvm_vcpu *vcpu)
> +{
> +	return kvm_arch_vcpu_in_kernel(vcpu);
> +}
> +
>  bool __weak kvm_arch_dy_has_pending_interrupt(struct kvm_vcpu *vcpu)
>  {
>  	return false;
>  }
> -
>  void kvm_vcpu_on_spin(struct kvm_vcpu *me, bool yield_to_kernel_mode)
>  {
>  	struct kvm *kvm = me->kvm;
> @@ -4080,7 +4091,7 @@ void kvm_vcpu_on_spin(struct kvm_vcpu *me, bool yield_to_kernel_mode)
>  				continue;
>  			if (READ_ONCE(vcpu->preempted) && yield_to_kernel_mode &&
>  			    !kvm_arch_dy_has_pending_interrupt(vcpu) &&
> -			    !kvm_arch_vcpu_in_kernel(vcpu))
> +			    !kvm_arch_vcpu_preempted_in_kernel(vcpu))
>  				continue;
>  			if (!kvm_vcpu_eligible_for_directed_yield(vcpu))
>  				continue;
> --
> 2.43.0.472.g3155946c3a-goog
>
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 3/4] KVM: x86: Clean up directed yield API for "has pending interrupt"
  2024-01-10  0:39 ` [PATCH 3/4] KVM: x86: Clean up directed yield API for "has pending interrupt" Sean Christopherson
@ 2024-01-11 12:49   ` Yuan Yao
  0 siblings, 0 replies; 11+ messages in thread
From: Yuan Yao @ 2024-01-11 12:49 UTC (permalink / raw)
  To: Sean Christopherson; +Cc: Paolo Bonzini, kvm, linux-kernel, Like Xu

On Tue, Jan 09, 2024 at 04:39:37PM -0800, Sean Christopherson wrote:
> Directly return the boolean result of whether or not a vCPU has a pending
> interrupt instead of effectively doing:
>
>   if (true)
> 	return true;
>
>   return false;
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>

Reviewed-by: Yuan Yao <yuan.yao@intel.com>

> ---
>  arch/x86/kvm/x86.c | 7 ++-----
>  1 file changed, 2 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 77494f9c8d49..b7996a75d9a3 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -13083,11 +13083,8 @@ int kvm_arch_vcpu_runnable(struct kvm_vcpu *vcpu)
>
>  bool kvm_arch_dy_has_pending_interrupt(struct kvm_vcpu *vcpu)
>  {
> -	if (kvm_vcpu_apicv_active(vcpu) &&
> -	    static_call(kvm_x86_dy_apicv_has_pending_interrupt)(vcpu))
> -		return true;
> -
> -	return false;
> +	return kvm_vcpu_apicv_active(vcpu) &&
> +	       static_call(kvm_x86_dy_apicv_has_pending_interrupt)(vcpu);
>  }
>
>  bool kvm_arch_vcpu_preempted_in_kernel(struct kvm_vcpu *vcpu)
> --
> 2.43.0.472.g3155946c3a-goog
>
>

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/4] KVM: Clean up "preempted in-kernel" logic
  2024-01-10  0:39 [PATCH 0/4] KVM: Clean up "preempted in-kernel" logic Sean Christopherson
                   ` (3 preceding siblings ...)
  2024-01-10  0:39 ` [PATCH 4/4] KVM: Add a comment explaining the directed yield pending interrupt logic Sean Christopherson
@ 2024-02-23  1:35 ` Sean Christopherson
  4 siblings, 0 replies; 11+ messages in thread
From: Sean Christopherson @ 2024-02-23  1:35 UTC (permalink / raw)
  To: Sean Christopherson, Paolo Bonzini; +Cc: kvm, linux-kernel, Like Xu

On Tue, 09 Jan 2024 16:39:34 -0800, Sean Christopherson wrote:
> Provide a dedicated helper to query if a *different* vCPU was preempted
> in-kernel.  x86's VMX is an oddball and can only check if the vCPU is in
> kernel (versus userspace) if the vCPU is loaded on the current pCPU.
> 
> The existing kvm_arch_vcpu_in_kernel() "works", but it's an ugly mess as
> KVM x86 is forced to check kvm_get_running_vcpu() to effectively undo the
> multiplexing.
> 
> [...]

Applied to kvm-x86 misc, thanks!

[1/4] KVM: Add dedicated arch hook for querying if vCPU was preempted in-kernel
      https://github.com/kvm-x86/linux/commit/77bcd9e6231a
[2/4] KVM: x86: Rely solely on preempted_in_kernel flag for directed yield
      https://github.com/kvm-x86/linux/commit/9b8615c5d37f
[3/4] KVM: x86: Clean up directed yield API for "has pending interrupt"
      https://github.com/kvm-x86/linux/commit/322d79f1db4b
[4/4] KVM: Add a comment explaining the directed yield pending interrupt logic
      https://github.com/kvm-x86/linux/commit/dafc17dd529a

--
https://github.com/kvm-x86/linux/tree/next

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2024-02-23  1:36 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-01-10  0:39 [PATCH 0/4] KVM: Clean up "preempted in-kernel" logic Sean Christopherson
2024-01-10  0:39 ` [PATCH 1/4] KVM: Add dedicated arch hook for querying if vCPU was preempted in-kernel Sean Christopherson
2024-01-11 12:48   ` Yuan Yao
2024-01-10  0:39 ` [PATCH 2/4] KVM: x86: Rely solely on preempted_in_kernel flag for directed yield Sean Christopherson
2024-01-10  7:55   ` Yuan Yao
2024-01-10 17:13     ` Sean Christopherson
2024-01-11 12:47       ` Yuan Yao
2024-01-10  0:39 ` [PATCH 3/4] KVM: x86: Clean up directed yield API for "has pending interrupt" Sean Christopherson
2024-01-11 12:49   ` Yuan Yao
2024-01-10  0:39 ` [PATCH 4/4] KVM: Add a comment explaining the directed yield pending interrupt logic Sean Christopherson
2024-02-23  1:35 ` [PATCH 0/4] KVM: Clean up "preempted in-kernel" logic Sean Christopherson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox