[PATCH] x86/kvm: refine condition for checking vCPU preempted

[PATCH] x86/kvm: refine condition for checking vCPU preempted

[lirongqing]

From: Li RongQing <lirongqing@baidu.com>

Check whether vcpu is preempted or not when HLT is trapped or there
is not realtime hint.
In other words, it is unnecessary to check preemption if HLT is not
intercepted and guest has realtime hint

Signed-off-by: Li RongQing <lirongqing@baidu.com>
---
 arch/x86/kernel/kvm.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
index 1cceac5..1a2744d 100644
--- a/arch/x86/kernel/kvm.c
+++ b/arch/x86/kernel/kvm.c
@@ -820,8 +820,10 @@ static void __init kvm_guest_init(void)
 		has_steal_clock = 1;
 		static_call_update(pv_steal_clock, kvm_steal_clock);
 
-		pv_ops.lock.vcpu_is_preempted =
-			PV_CALLEE_SAVE(__kvm_vcpu_is_preempted);
+		if (kvm_para_has_feature(KVM_FEATURE_PV_UNHALT) ||
+		     !kvm_para_has_hint(KVM_HINTS_REALTIME))
+			pv_ops.lock.vcpu_is_preempted =
+				PV_CALLEE_SAVE(__kvm_vcpu_is_preempted);
 	}
 
 	if (kvm_para_has_feature(KVM_FEATURE_PV_EOI))
-- 
2.9.4

Re: [PATCH] x86/kvm: refine condition for checking vCPU preempted

[Sean Christopherson]

+Paolo, Wanpeng, and Vitaly

In the future, use get_maintainers.pl to build To: and Cc: so that the right folks
see the patch.  Not everyone habitually scours the KVM list. :-)

On Wed, Feb 15, 2023, lirongqing@baidu.com wrote:
> From: Li RongQing <lirongqing@baidu.com>
> 
> Check whether vcpu is preempted or not when HLT is trapped or there
> is not realtime hint.

Please explain _why_ there's no need to check for preemption in this setup.  What
may be obvious to you isn't necessarily obvious to reviewers or readers.

> In other words, it is unnecessary to check preemption if HLT is not
> intercepted and guest has realtime hint
> 
> Signed-off-by: Li RongQing <lirongqing@baidu.com>
> ---
>  arch/x86/kernel/kvm.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index 1cceac5..1a2744d 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
> @@ -820,8 +820,10 @@ static void __init kvm_guest_init(void)
>  		has_steal_clock = 1;
>  		static_call_update(pv_steal_clock, kvm_steal_clock);
>  
> -		pv_ops.lock.vcpu_is_preempted =
> -			PV_CALLEE_SAVE(__kvm_vcpu_is_preempted);
> +		if (kvm_para_has_feature(KVM_FEATURE_PV_UNHALT) ||

Rather than have the guest rely on host KVM behavior clearing PV_UNHALT when HLT
is passed through), would it make sense to add something like KVM_HINTS_HLT_PASSTHROUGH
to more explicitly tell the guest that HLT isn't intercepted?

> +		     !kvm_para_has_hint(KVM_HINTS_REALTIME))

Misaligned indentation (one too many spaces).

> +			pv_ops.lock.vcpu_is_preempted =
> +				PV_CALLEE_SAVE(__kvm_vcpu_is_preempted);
>  	}
>  
>  	if (kvm_para_has_feature(KVM_FEATURE_PV_EOI))
> -- 
> 2.9.4
> 

RE: [PATCH] x86/kvm: refine condition for checking vCPU preempted

[Li,Rongqing]

> -----Original Message-----
> From: Sean Christopherson <seanjc@google.com>
> Sent: Wednesday, March 15, 2023 8:16 AM
> To: Li,Rongqing <lirongqing@baidu.com>
> Cc: kvm@vger.kernel.org; x86@kernel.org; Paolo Bonzini
> <pbonzini@redhat.com>; Wanpeng Li <wanpengli@tencent.com>; Vitaly
> Kuznetsov <vkuznets@redhat.com>
> Subject: Re: [PATCH] x86/kvm: refine condition for checking vCPU preempted
> 
> +Paolo, Wanpeng, and Vitaly
> 
> In the future, use get_maintainers.pl to build To: and Cc: so that the right folks
> see the patch.  Not everyone habitually scours the KVM list. :-)
> 
Ok

> On Wed, Feb 15, 2023, lirongqing@baidu.com wrote:
> > From: Li RongQing <lirongqing@baidu.com>
> >
> > Check whether vcpu is preempted or not when HLT is trapped or there is
> > not realtime hint.
> 
> Please explain _why_ there's no need to check for preemption in this setup.
> What may be obvious to you isn't necessarily obvious to reviewers or readers.
> 

I will rewrite the changelog

> > In other words, it is unnecessary to check preemption if HLT is not
> > intercepted and guest has realtime hint
> >
> > Signed-off-by: Li RongQing <lirongqing@baidu.com>
> > ---
> >  arch/x86/kernel/kvm.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c index
> > 1cceac5..1a2744d 100644
> > --- a/arch/x86/kernel/kvm.c
> > +++ b/arch/x86/kernel/kvm.c
> > @@ -820,8 +820,10 @@ static void __init kvm_guest_init(void)
> >  		has_steal_clock = 1;
> >  		static_call_update(pv_steal_clock, kvm_steal_clock);
> >
> > -		pv_ops.lock.vcpu_is_preempted =
> > -			PV_CALLEE_SAVE(__kvm_vcpu_is_preempted);
> > +		if (kvm_para_has_feature(KVM_FEATURE_PV_UNHALT) ||
> 
> Rather than have the guest rely on host KVM behavior clearing PV_UNHALT
> when HLT is passed through), would it make sense to add something like
> KVM_HINTS_HLT_PASSTHROUGH to more explicitly tell the guest that HLT isn't
> intercepted?

KVM_HINTS_HLT_PASSTHROUGH is more obvious, but it need both kvm and guest support

Thanks

-Li

> 
> > +		     !kvm_para_has_hint(KVM_HINTS_REALTIME))
> 
> Misaligned indentation (one too many spaces).
> 
> > +			pv_ops.lock.vcpu_is_preempted =
> > +				PV_CALLEE_SAVE(__kvm_vcpu_is_preempted);
> >  	}
> >
> >  	if (kvm_para_has_feature(KVM_FEATURE_PV_EOI))
> > --
> > 2.9.4
> >

Re: [PATCH] x86/kvm: refine condition for checking vCPU preempted

[Sean Christopherson]

On Wed, Mar 15, 2023, Li,Rongqing wrote:
> > Rather than have the guest rely on host KVM behavior clearing PV_UNHALT
> > when HLT is passed through), would it make sense to add something like
> > KVM_HINTS_HLT_PASSTHROUGH to more explicitly tell the guest that HLT isn't
> > intercepted?
> 
> KVM_HINTS_HLT_PASSTHROUGH is more obvious, but it need both kvm and guest support

Yeah, that's the downside.  But modifying KVM and/or the userspace VMM shouldn't
be difficult, i.e the only meaningful cost is the rollout of a new kernel/VMM.

On the other hand, establishing the heuristic that !PV_UNHALT == HLT_PASSTHROUGH
could have to subtle issues in the future.  It safe-ish in the context of this
patch as userspace is unlikely to set KVM_HINTS_REALTIME, hide PV_UNHALT, and _not_
passthrough HLT.  But without the REALTIME side of things, !PV_UNHALT == HLT_PASSTHROUGH
is much less likely to hold true.

RE: [PATCH] x86/kvm: refine condition for checking vCPU preempted

[Li,Rongqing]

> -----Original Message-----
> From: Sean Christopherson <seanjc@google.com>
> Sent: Wednesday, March 15, 2023 11:27 PM
> To: Li,Rongqing <lirongqing@baidu.com>
> Cc: kvm@vger.kernel.org; x86@kernel.org; Paolo Bonzini
> <pbonzini@redhat.com>; Wanpeng Li <wanpengli@tencent.com>; Vitaly
> Kuznetsov <vkuznets@redhat.com>
> Subject: Re: [PATCH] x86/kvm: refine condition for checking vCPU preempted
> 
> On Wed, Mar 15, 2023, Li,Rongqing wrote:
> > > Rather than have the guest rely on host KVM behavior clearing
> > > PV_UNHALT when HLT is passed through), would it make sense to add
> > > something like KVM_HINTS_HLT_PASSTHROUGH to more explicitly tell the
> > > guest that HLT isn't intercepted?
> >
> > KVM_HINTS_HLT_PASSTHROUGH is more obvious, but it need both kvm and
> > guest support
> 
> Yeah, that's the downside.  But modifying KVM and/or the userspace VMM
> shouldn't be difficult, i.e the only meaningful cost is the rollout of a new
> kernel/VMM.
> 
> On the other hand, establishing the heuristic that !PV_UNHALT ==
> HLT_PASSTHROUGH could have to subtle issues in the future.  It safe-ish in the
> context of this patch as userspace is unlikely to set KVM_HINTS_REALTIME, hide
> PV_UNHALT, and _not_ passthrough HLT.  But without the REALTIME side of
> things, !PV_UNHALT == HLT_PASSTHROUGH is much less likely to hold true.

Ok, could you submit these codes

Thanks

-Li

Re: [PATCH] x86/kvm: refine condition for checking vCPU preempted

[Sean Christopherson]

On Thu, Mar 16, 2023, Li,Rongqing wrote:
> > From: Sean Christopherson <seanjc@google.com>
> > On Wed, Mar 15, 2023, Li,Rongqing wrote:
> > > > Rather than have the guest rely on host KVM behavior clearing
> > > > PV_UNHALT when HLT is passed through), would it make sense to add
> > > > something like KVM_HINTS_HLT_PASSTHROUGH to more explicitly tell the
> > > > guest that HLT isn't intercepted?
> > >
> > > KVM_HINTS_HLT_PASSTHROUGH is more obvious, but it need both kvm and
> > > guest support
> > 
> > Yeah, that's the downside.  But modifying KVM and/or the userspace VMM
> > shouldn't be difficult, i.e the only meaningful cost is the rollout of a new
> > kernel/VMM.
> > 
> > On the other hand, establishing the heuristic that !PV_UNHALT ==
> > HLT_PASSTHROUGH could have to subtle issues in the future.  It safe-ish in the
> > context of this patch as userspace is unlikely to set KVM_HINTS_REALTIME, hide
> > PV_UNHALT, and _not_ passthrough HLT.  But without the REALTIME side of
> > things, !PV_UNHALT == HLT_PASSTHROUGH is much less likely to hold true.
> 
> Ok, could you submit these codes

I'd like to hear from others first, especially Paolo and/or Wanpeng.

RE: [PATCH] x86/kvm: refine condition for checking vCPU preempted

[Li,Rongqing]

> -----Original Message-----
> From: Sean Christopherson <seanjc@google.com>
> Sent: Friday, March 17, 2023 1:01 AM
> To: Li,Rongqing <lirongqing@baidu.com>
> Cc: kvm@vger.kernel.org; x86@kernel.org; Paolo Bonzini
> <pbonzini@redhat.com>; Wanpeng Li <wanpengli@tencent.com>; Vitaly
> Kuznetsov <vkuznets@redhat.com>
> Subject: Re: [PATCH] x86/kvm: refine condition for checking vCPU preempted
> 
> On Thu, Mar 16, 2023, Li,Rongqing wrote:
> > > From: Sean Christopherson <seanjc@google.com> On Wed, Mar 15, 2023,
> > > Li,Rongqing wrote:
> > > > > Rather than have the guest rely on host KVM behavior clearing
> > > > > PV_UNHALT when HLT is passed through), would it make sense to
> > > > > add something like KVM_HINTS_HLT_PASSTHROUGH to more explicitly
> > > > > tell the guest that HLT isn't intercepted?
> > > >
> > > > KVM_HINTS_HLT_PASSTHROUGH is more obvious, but it need both kvm
> > > > and guest support
> > >
> > > Yeah, that's the downside.  But modifying KVM and/or the userspace
> > > VMM shouldn't be difficult, i.e the only meaningful cost is the
> > > rollout of a new kernel/VMM.
> > >
> > > On the other hand, establishing the heuristic that !PV_UNHALT ==
> > > HLT_PASSTHROUGH could have to subtle issues in the future.  It
> > > safe-ish in the context of this patch as userspace is unlikely to
> > > set KVM_HINTS_REALTIME, hide PV_UNHALT, and _not_ passthrough HLT.
> > > But without the REALTIME side of things, !PV_UNHALT ==
> HLT_PASSTHROUGH is much less likely to hold true.
> >
> > Ok, could you submit these codes
> 
> I'd like to hear from others first, especially Paolo and/or Wanpeng.

I see no progress
How about to adopt this patch at first, it can give small performance for existing KVM and setup
Then you continue to modify the kernel/VMM to give better support for KVM/guest

-Li

Re: [PATCH] x86/kvm: refine condition for checking vCPU preempted

[Sean Christopherson]

On Thu, Mar 30, 2023, Li,Rongqing wrote:
> > From: Sean Christopherson <seanjc@google.com>
> > On Thu, Mar 16, 2023, Li,Rongqing wrote:
> > > > From: Sean Christopherson <seanjc@google.com> On Wed, Mar 15, 2023,
> > > > Li,Rongqing wrote:
> > > > > > Rather than have the guest rely on host KVM behavior clearing
> > > > > > PV_UNHALT when HLT is passed through), would it make sense to
> > > > > > add something like KVM_HINTS_HLT_PASSTHROUGH to more explicitly
> > > > > > tell the guest that HLT isn't intercepted?
> > > > >
> > > > > KVM_HINTS_HLT_PASSTHROUGH is more obvious, but it need both kvm
> > > > > and guest support
> > > >
> > > > Yeah, that's the downside.  But modifying KVM and/or the userspace
> > > > VMM shouldn't be difficult, i.e the only meaningful cost is the
> > > > rollout of a new kernel/VMM.
> > > >
> > > > On the other hand, establishing the heuristic that !PV_UNHALT ==
> > > > HLT_PASSTHROUGH could have to subtle issues in the future.  It
> > > > safe-ish in the context of this patch as userspace is unlikely to
> > > > set KVM_HINTS_REALTIME, hide PV_UNHALT, and _not_ passthrough HLT.
> > > > But without the REALTIME side of things, !PV_UNHALT ==
> > HLT_PASSTHROUGH is much less likely to hold true.
> > >
> > > Ok, could you submit these codes
> > 
> > I'd like to hear from others first, especially Paolo and/or Wanpeng.
> 
> I see no progress
> How about to adopt this patch at first, it can give small performance for existing KVM and setup
> Then you continue to modify the kernel/VMM to give better support for KVM/guest

Guest-side KVM stuff is not my maintenance domain, i.e. this needs Paolo's
attention no matter what.

RE: [PATCH] x86/kvm: refine condition for checking vCPU preempted

[Li,Rongqing]

> -----Original Message-----
> From: Sean Christopherson <seanjc@google.com>
> Sent: Friday, March 31, 2023 3:26 AM
> To: Li,Rongqing <lirongqing@baidu.com>
> Cc: kvm@vger.kernel.org; x86@kernel.org; Paolo Bonzini
> <pbonzini@redhat.com>; Wanpeng Li <wanpengli@tencent.com>; Vitaly
> Kuznetsov <vkuznets@redhat.com>
> Subject: Re: [PATCH] x86/kvm: refine condition for checking vCPU preempted
> 


> Guest-side KVM stuff is not my maintenance domain, i.e. this needs Paolo's
> attention no matter what.


Ok, I will rewrite the changelog and modify the code indentation as your suggestion, and resend this patch

Thanks

-Li