KVM: x86: Prep KVM hypercall handling for TDX

[PATCH v4 4/6] KVM: x86: Bump hypercall stat prior to fully completing hypercall

Posted by Sean Christopherson 1 year, 2 months ago

Increment the "hypercalls" stat for KVM hypercalls as soon as KVM knows
it will skip the guest instruction, i.e. once KVM is committed to emulating
the hypercall.  Waiting until completion adds no known value, and creates a
discrepancy where the stat will be bumped if KVM exits to userspace as a
result of trying to skip the instruction, but not if the hypercall itself
exits.

Handling the stat in common code will also avoid the need for another
helper to dedup code when TDX comes along (TDX needs a separate completion
path due to GPR usage differences).

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

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 13fe5d6eb8f3..11434752b467 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -9979,7 +9979,6 @@ static int complete_hypercall_exit(struct kvm_vcpu *vcpu)
 	if (!is_64_bit_hypercall(vcpu))
 		ret = (u32)ret;
 	kvm_rax_write(vcpu, ret);
-	++vcpu->stat.hypercalls;
 	return kvm_skip_emulated_instruction(vcpu);
 }
 
@@ -9990,6 +9989,8 @@ unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
 {
 	unsigned long ret;
 
+	++vcpu->stat.hypercalls;
+
 	trace_kvm_hypercall(nr, a0, a1, a2, a3);
 
 	if (!op_64_bit) {
@@ -10070,7 +10071,6 @@ unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
 	}
 
 out:
-	++vcpu->stat.hypercalls;
 	return ret;
 }
 EXPORT_SYMBOL_GPL(__kvm_emulate_hypercall);
-- 
2.47.0.338.g60cca15819-goog

Re: [PATCH v4 4/6] KVM: x86: Bump hypercall stat prior to fully completing hypercall

Posted by Binbin Wu 1 year, 2 months ago



On 11/28/2024 8:43 AM, Sean Christopherson wrote:
> Increment the "hypercalls" stat for KVM hypercalls as soon as KVM knows
> it will skip the guest instruction, i.e. once KVM is committed to emulating
> the hypercall.  Waiting until completion adds no known value, and creates a
> discrepancy where the stat will be bumped if KVM exits to userspace as a
> result of trying to skip the instruction, but not if the hypercall itself
> exits.
>
> Handling the stat in common code will also avoid the need for another
> helper to dedup code when TDX comes along (TDX needs a separate completion
> path due to GPR usage differences).
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
Reviewed-by: Binbin Wu <binbin.wu@linux.intel.com>

> ---
>   arch/x86/kvm/x86.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 13fe5d6eb8f3..11434752b467 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -9979,7 +9979,6 @@ static int complete_hypercall_exit(struct kvm_vcpu *vcpu)
>   	if (!is_64_bit_hypercall(vcpu))
>   		ret = (u32)ret;
>   	kvm_rax_write(vcpu, ret);
> -	++vcpu->stat.hypercalls;
>   	return kvm_skip_emulated_instruction(vcpu);
>   }
>   
> @@ -9990,6 +9989,8 @@ unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
>   {
>   	unsigned long ret;
>   
> +	++vcpu->stat.hypercalls;
> +
>   	trace_kvm_hypercall(nr, a0, a1, a2, a3);
>   
>   	if (!op_64_bit) {
> @@ -10070,7 +10071,6 @@ unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
>   	}
>   
>   out:
> -	++vcpu->stat.hypercalls;
>   	return ret;
>   }
>   EXPORT_SYMBOL_GPL(__kvm_emulate_hypercall);

Re: [PATCH v4 4/6] KVM: x86: Bump hypercall stat prior to fully completing hypercall

Posted by Tom Lendacky 1 year, 2 months ago

On 11/27/24 18:43, Sean Christopherson wrote:
> Increment the "hypercalls" stat for KVM hypercalls as soon as KVM knows
> it will skip the guest instruction, i.e. once KVM is committed to emulating
> the hypercall.  Waiting until completion adds no known value, and creates a
> discrepancy where the stat will be bumped if KVM exits to userspace as a
> result of trying to skip the instruction, but not if the hypercall itself
> exits.
> 
> Handling the stat in common code will also avoid the need for another
> helper to dedup code when TDX comes along (TDX needs a separate completion
> path due to GPR usage differences).
> 
> Signed-off-by: Sean Christopherson <seanjc@google.com>

There's a comment in the KVM_HC_MAP_GPA_RANGE case that reads:

	/* stat is incremented on completion. */

that should probably be deleted, but otherwise:

Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>

Also, if you want, you could get rid of the 'out' label, too, by doing:

	if (cpl)
		return -KVM_EPERM;

> ---
>  arch/x86/kvm/x86.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 13fe5d6eb8f3..11434752b467 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -9979,7 +9979,6 @@ static int complete_hypercall_exit(struct kvm_vcpu *vcpu)
>  	if (!is_64_bit_hypercall(vcpu))
>  		ret = (u32)ret;
>  	kvm_rax_write(vcpu, ret);
> -	++vcpu->stat.hypercalls;
>  	return kvm_skip_emulated_instruction(vcpu);
>  }
>  
> @@ -9990,6 +9989,8 @@ unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
>  {
>  	unsigned long ret;
>  
> +	++vcpu->stat.hypercalls;
> +
>  	trace_kvm_hypercall(nr, a0, a1, a2, a3);
>  
>  	if (!op_64_bit) {
> @@ -10070,7 +10071,6 @@ unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
>  	}
>  
>  out:
> -	++vcpu->stat.hypercalls;
>  	return ret;
>  }
>  EXPORT_SYMBOL_GPL(__kvm_emulate_hypercall);

Re: [PATCH v4 4/6] KVM: x86: Bump hypercall stat prior to fully completing hypercall

Posted by Tom Lendacky 1 year, 2 months ago

On 12/2/24 14:13, Tom Lendacky wrote:
> On 11/27/24 18:43, Sean Christopherson wrote:
>> Increment the "hypercalls" stat for KVM hypercalls as soon as KVM knows
>> it will skip the guest instruction, i.e. once KVM is committed to emulating
>> the hypercall.  Waiting until completion adds no known value, and creates a
>> discrepancy where the stat will be bumped if KVM exits to userspace as a
>> result of trying to skip the instruction, but not if the hypercall itself
>> exits.
>>
>> Handling the stat in common code will also avoid the need for another
>> helper to dedup code when TDX comes along (TDX needs a separate completion
>> path due to GPR usage differences).
>>
>> Signed-off-by: Sean Christopherson <seanjc@google.com>
> 
> There's a comment in the KVM_HC_MAP_GPA_RANGE case that reads:
> 
> 	/* stat is incremented on completion. */
> 
> that should probably be deleted, but otherwise:
> 
> Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
> 
> Also, if you want, you could get rid of the 'out' label, too, by doing:
> 
> 	if (cpl)
> 		return -KVM_EPERM;

Until I saw the next patch...  nevermind.

Thanks,
Tom

> 
>> ---
>>  arch/x86/kvm/x86.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 13fe5d6eb8f3..11434752b467 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -9979,7 +9979,6 @@ static int complete_hypercall_exit(struct kvm_vcpu *vcpu)
>>  	if (!is_64_bit_hypercall(vcpu))
>>  		ret = (u32)ret;
>>  	kvm_rax_write(vcpu, ret);
>> -	++vcpu->stat.hypercalls;
>>  	return kvm_skip_emulated_instruction(vcpu);
>>  }
>>  
>> @@ -9990,6 +9989,8 @@ unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
>>  {
>>  	unsigned long ret;
>>  
>> +	++vcpu->stat.hypercalls;
>> +
>>  	trace_kvm_hypercall(nr, a0, a1, a2, a3);
>>  
>>  	if (!op_64_bit) {
>> @@ -10070,7 +10071,6 @@ unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
>>  	}
>>  
>>  out:
>> -	++vcpu->stat.hypercalls;
>>  	return ret;
>>  }
>>  EXPORT_SYMBOL_GPL(__kvm_emulate_hypercall);

Re: [PATCH v4 4/6] KVM: x86: Bump hypercall stat prior to fully completing hypercall

Posted by Xiaoyao Li 1 year, 2 months ago

On 11/28/2024 8:43 AM, Sean Christopherson wrote:
> Increment the "hypercalls" stat for KVM hypercalls as soon as KVM knows
> it will skip the guest instruction, i.e. once KVM is committed to emulating
> the hypercall.  Waiting until completion adds no known value, and creates a
> discrepancy where the stat will be bumped if KVM exits to userspace as a
> result of trying to skip the instruction, but not if the hypercall itself
> exits.
> 
> Handling the stat in common code will also avoid the need for another
> helper to dedup code when TDX comes along (TDX needs a separate completion
> path due to GPR usage differences).

Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>

> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>   arch/x86/kvm/x86.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 13fe5d6eb8f3..11434752b467 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -9979,7 +9979,6 @@ static int complete_hypercall_exit(struct kvm_vcpu *vcpu)
>   	if (!is_64_bit_hypercall(vcpu))
>   		ret = (u32)ret;
>   	kvm_rax_write(vcpu, ret);
> -	++vcpu->stat.hypercalls;
>   	return kvm_skip_emulated_instruction(vcpu);
>   }
>   
> @@ -9990,6 +9989,8 @@ unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
>   {
>   	unsigned long ret;
>   
> +	++vcpu->stat.hypercalls;
> +
>   	trace_kvm_hypercall(nr, a0, a1, a2, a3);
>   
>   	if (!op_64_bit) {
> @@ -10070,7 +10071,6 @@ unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
>   	}
>   
>   out:
> -	++vcpu->stat.hypercalls;
>   	return ret;
>   }
>   EXPORT_SYMBOL_GPL(__kvm_emulate_hypercall);