[PATCH][v2] x86/kvm: Don't check vCPU preempted if vCPU has dedicated pCPU and non-trap HLT

[PATCH][v2] x86/kvm: Don't check vCPU preempted if vCPU has dedicated pCPU and non-trap HLT

[lirongqing]

From: Li RongQing <lirongqing@baidu.com>

Check whether vCPU is preempted or not only when HLT is trapped or
there is not realtime hint. In other words, it is unnecessary to check
preemption when vCPU has realtime hint (which means vCPU has dedicated
pCP) and has not PV_UNHALT (which means unintercepted HLT), because
vCPU should not to be marked as preempted in this setup.

Signed-off-by: Li RongQing <lirongqing@baidu.com>
---
diff with v1: rewrite changelog and indentation

 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..25398d2 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][v2] x86/kvm: Don't check vCPU preempted if vCPU has dedicated pCPU and non-trap HLT

[Peter Zijlstra]

On Thu, Apr 06, 2023 at 03:33:13PM +0800, lirongqing@baidu.com wrote:
> From: Li RongQing <lirongqing@baidu.com>
> 
> Check whether vCPU is preempted or not only when HLT is trapped or
> there is not realtime hint. In other words, it is unnecessary to check
> preemption when vCPU has realtime hint (which means vCPU has dedicated
> pCP) and has not PV_UNHALT (which means unintercepted HLT), because
> vCPU should not to be marked as preempted in this setup.

To what benefit?

> Signed-off-by: Li RongQing <lirongqing@baidu.com>
> ---
> diff with v1: rewrite changelog and indentation
> 
>  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..25398d2 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))

This is atrocious coding style, please align on the (.

> +			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][v2] x86/kvm: Don't check vCPU preempted if vCPU has dedicated pCPU and non-trap HLT

[Sean Christopherson]

On Thu, Apr 06, 2023, Peter Zijlstra wrote:
> On Thu, Apr 06, 2023 at 03:33:13PM +0800, lirongqing@baidu.com wrote:
> > From: Li RongQing <lirongqing@baidu.com>
> > 
> > Check whether vCPU is preempted or not only when HLT is trapped or
> > there is not realtime hint. In other words, it is unnecessary to check
> > preemption when vCPU has realtime hint (which means vCPU has dedicated
> > pCP) and has not PV_UNHALT (which means unintercepted HLT), because
> > vCPU should not to be marked as preempted in this setup.
> 
> To what benefit?
> 
> > Signed-off-by: Li RongQing <lirongqing@baidu.com>
> > ---
> > diff with v1: rewrite changelog and indentation

This also fails to mention my objection to querying PV_UNHALT[*].  When I said
"this needs Paolo's attention no matter what", I did not mean "post a v2 and hope
Paolo applies it", I meant we need Paolo (and others) to weigh in on the ongoing
discussion.

[*] https://lore.kernel.org/all/ZBEOK6ws9wGqof3O@google.com

Re: [PATCH][v2] x86/kvm: Don't check vCPU preempted if vCPU has dedicated pCPU and non-trap HLT

[Paolo Bonzini]

On 4/6/23 18:08, Sean Christopherson wrote:
>> Signed-off-by: Li RongQing<lirongqing@baidu.com>
>> ---
>> diff with v1: rewrite changelog and indentation
>
> This also fails to mention my objection to querying PV_UNHALT[*].  When I said
> "this needs Paolo's attention no matter what", I did not mean "post a v2 and hope
> Paolo applies it", I meant we need Paolo (and others) to weigh in on the ongoing
> discussion.

Quoting v1:

>> +		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?

Yes, I agree with adding KVM_HINTS_HLT_PASSTHROUGH or 
KVM_HINTS_GUEST_CSTATE (i.e. host can remain in guest mode even when 
running in C1 aka hlt or possibly deeper states).

Lack of PV_UNHALT does not indicate anything about whether HLT will be 
handled in host or guest.  In fact the same KVM_HINTS_* value could also 
be used to disable PV_UNHALT.

Paolo