[PATCH v3 0/2] KVM: x86: Check hypercall's exit to userspace generically

[PATCH v3 0/2] KVM: x86: Check hypercall's exit to userspace generically

[Binbin Wu]

Currently in kvm_emulate_hypercall, KVM_HC_MAP_GPA_RANGE is checked
specifically to decide whether a KVM hypercall needs to exit to userspace
or not.  Do the check based on the hypercall_exit_enabled field of
struct kvm_arch.

Also use the API user_exit_on_hypercall() to replace the opencode.

---
v3:
- Rename is_kvm_hc_exit_enabled() to user_exit_on_hypercall(). (Sean)
- Remove the WARN_ON_ONCE(). (Isaku, Sean)
- Use BIT(hc_nr) instead of (1 << nr) (Yuan)
- Added a comment to explain why check the !ret first. (Kai)
- Add Kai and Isaku's Reviewed-by.

v2:
- Check the return value of __kvm_emulate_hypercall() before checking
  hypercall_exit_enabled to avoid an invalid KVM hypercall nr.
  https://lore.kernel.org/kvm/184d90a8-14a0-494a-9112-365417245911@linux.intel.com/
- Add a warning if a hypercall nr out of the range of hypercall_exit_enabled
  can express.

Binbin Wu (2):
  KVM: x86: Check hypercall's exit to userspace generically
  KVM: x86: Use user_exit_on_hypercall() instead of opencode

 arch/x86/kvm/svm/sev.c | 4 ++--
 arch/x86/kvm/x86.c     | 7 ++++---
 arch/x86/kvm/x86.h     | 4 ++++
 3 files changed, 10 insertions(+), 5 deletions(-)


base-commit: a1206bc992c3cd3f758a9b46117dfc7e59e8c10f
-- 
2.46.0

[PATCH v3 1/2] KVM: x86: Check hypercall's exit to userspace generically

[Binbin Wu]

Check whether a KVM hypercall needs to exit to userspace or not based on
hypercall_exit_enabled field of struct kvm_arch.

Userspace can request a hypercall to exit to userspace for handling by
enable KVM_CAP_EXIT_HYPERCALL and the enabled hypercall will be set in
hypercall_exit_enabled.  Make the check code generic based on it.

Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com>
Reviewed-by: Kai Huang <kai.huang@intel.com>
---
 arch/x86/kvm/x86.c | 5 +++--
 arch/x86/kvm/x86.h | 4 ++++
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 966fb301d44b..e521f14ad2b2 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -10220,8 +10220,9 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
 	cpl = kvm_x86_call(get_cpl)(vcpu);
 
 	ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl);
-	if (nr == KVM_HC_MAP_GPA_RANGE && !ret)
-		/* MAP_GPA tosses the request to the user space. */
+	/* Check !ret first to make sure nr is a valid KVM hypercall. */
+	if (!ret && user_exit_on_hypercall(vcpu->kvm, nr))
+		/* The hypercall is requested to exit to userspace. */
 		return 0;
 
 	if (!op_64_bit)
diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index 6556a43f1915..bc1a9e080acb 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -561,4 +561,8 @@ int kvm_sev_es_string_io(struct kvm_vcpu *vcpu, unsigned int size,
 			 unsigned int port, void *data,  unsigned int count,
 			 int in);
 
+static inline bool user_exit_on_hypercall(struct kvm *kvm, unsigned long hc_nr)
+{
+	return kvm->arch.hypercall_exit_enabled & BIT(hc_nr);
+}
 #endif
-- 
2.46.0

Re: [PATCH v3 1/2] KVM: x86: Check hypercall's exit to userspace generically

[Xiaoyao Li]

On 8/26/2024 10:22 AM, Binbin Wu wrote:
> Check whether a KVM hypercall needs to exit to userspace or not based on
> hypercall_exit_enabled field of struct kvm_arch.
> 
> Userspace can request a hypercall to exit to userspace for handling by
> enable KVM_CAP_EXIT_HYPERCALL and the enabled hypercall will be set in
> hypercall_exit_enabled.  Make the check code generic based on it.
> 
> Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com>
> Reviewed-by: Kai Huang <kai.huang@intel.com>
> ---
>   arch/x86/kvm/x86.c | 5 +++--
>   arch/x86/kvm/x86.h | 4 ++++
>   2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 966fb301d44b..e521f14ad2b2 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -10220,8 +10220,9 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
>   	cpl = kvm_x86_call(get_cpl)(vcpu);
>   
>   	ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl);
> -	if (nr == KVM_HC_MAP_GPA_RANGE && !ret)
> -		/* MAP_GPA tosses the request to the user space. */
> +	/* Check !ret first to make sure nr is a valid KVM hypercall. */
> +	if (!ret && user_exit_on_hypercall(vcpu->kvm, nr))
> +		/* The hypercall is requested to exit to userspace. */

Nit: Above comment is unnecessary since the name of 
user_exit_on_hypercall() is self documenting.

Otherwise,

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

>   		return 0;
>   
>   	if (!op_64_bit)
> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
> index 6556a43f1915..bc1a9e080acb 100644
> --- a/arch/x86/kvm/x86.h
> +++ b/arch/x86/kvm/x86.h
> @@ -561,4 +561,8 @@ int kvm_sev_es_string_io(struct kvm_vcpu *vcpu, unsigned int size,
>   			 unsigned int port, void *data,  unsigned int count,
>   			 int in);
>   
> +static inline bool user_exit_on_hypercall(struct kvm *kvm, unsigned long hc_nr)
> +{
> +	return kvm->arch.hypercall_exit_enabled & BIT(hc_nr);
> +}
>   #endif

Re: [PATCH v3 1/2] KVM: x86: Check hypercall's exit to userspace generically

[Sean Christopherson]

On Mon, Aug 26, 2024, Binbin Wu wrote:
> Check whether a KVM hypercall needs to exit to userspace or not based on
> hypercall_exit_enabled field of struct kvm_arch.
> 
> Userspace can request a hypercall to exit to userspace for handling by
> enable KVM_CAP_EXIT_HYPERCALL and the enabled hypercall will be set in
> hypercall_exit_enabled.  Make the check code generic based on it.
> 
> Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com>
> Reviewed-by: Kai Huang <kai.huang@intel.com>
> ---
>  arch/x86/kvm/x86.c | 5 +++--
>  arch/x86/kvm/x86.h | 4 ++++
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 966fb301d44b..e521f14ad2b2 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -10220,8 +10220,9 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
>  	cpl = kvm_x86_call(get_cpl)(vcpu);
>  
>  	ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl);
> -	if (nr == KVM_HC_MAP_GPA_RANGE && !ret)
> -		/* MAP_GPA tosses the request to the user space. */
> +	/* Check !ret first to make sure nr is a valid KVM hypercall. */
> +	if (!ret && user_exit_on_hypercall(vcpu->kvm, nr))

I don't love that the caller has to re-check for user_exit_on_hypercall().  I
also don't love that there's a surprising number of checks lurking in
__kvm_emulate_hypercall(), e.g. that CPL==0, especially since the above comment
about "a valid KVM hypercall" can be intrepreted as meaning KVM is *only* checking
if the hypercall number is valid.

E.g. my initial reaction was that we could add a separate path for userspace
hypercalls, but that would be subtly wrong.  And my second reaction was to hoist
the common checks out of __kvm_emulate_hypercall(), but then I remembered that
the only reason __kvm_emulate_hypercall() is separate is to allow it to be called
by TDX with different source/destionation registers.

So, I'm strongly leaning towards dropping the above change, squashing the addition
of the helper with patch 2, and then landing this on top.

Thoughts?

--
Subject: [PATCH] KVM: x86: Use '0' in __kvm_emulate_hypercall()  to signal
 "exit to userspace"

Rework __kvm_emulate_hypercall() to use '0' to indicate an exit to
userspace instead of relying on the caller to manually check for success
*and* if user_exit_on_hypercall() is true.  Use '1' for "success" to
(mostly) align with KVM's de factor return codes, where '0' == exit to
userspace, '1' == resume guest, and -errno == failure.  Unfortunately,
some of the PV error codes returned to the guest are postive values, so
the pattern doesn't exactly match KVM's "standard", but it should be close
enough to be intuitive for KVM readers.

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

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index e09daa3b157c..5fdeb58221e2 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -10024,7 +10024,7 @@ unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
 
 	switch (nr) {
 	case KVM_HC_VAPIC_POLL_IRQ:
-		ret = 0;
+		ret = 1;
 		break;
 	case KVM_HC_KICK_CPU:
 		if (!guest_pv_has(vcpu, KVM_FEATURE_PV_UNHALT))
@@ -10032,7 +10032,7 @@ unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
 
 		kvm_pv_kick_cpu_op(vcpu->kvm, a1);
 		kvm_sched_yield(vcpu, a1);
-		ret = 0;
+		ret = 1;
 		break;
 #ifdef CONFIG_X86_64
 	case KVM_HC_CLOCK_PAIRING:
@@ -10050,7 +10050,7 @@ unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
 			break;
 
 		kvm_sched_yield(vcpu, a0);
-		ret = 0;
+		ret = 1;
 		break;
 	case KVM_HC_MAP_GPA_RANGE: {
 		u64 gpa = a0, npages = a1, attrs = a2;
@@ -10111,12 +10111,21 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
 	cpl = kvm_x86_call(get_cpl)(vcpu);
 
 	ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl);
-	if (nr == KVM_HC_MAP_GPA_RANGE && !ret)
-		/* MAP_GPA tosses the request to the user space. */
+	if (!ret)
 		return 0;
 
-	if (!op_64_bit)
+	/*
+	 * KVM's ABI with the guest is that '0' is success, and any other value
+	 * is an error code.  Internally, '0' == exit to userspace (see above)
+	 * and '1' == success, as KVM's de facto standard return codes are that
+	 * plus -errno == failure.  Explicitly check for '1' as some PV error
+	 * codes are positive values.
+	 */
+	if (ret == 1)
+		ret = 0;
+	else if (!op_64_bit)
 		ret = (u32)ret;
+
 	kvm_rax_write(vcpu, ret);
 
 	return kvm_skip_emulated_instruction(vcpu);

base-commit: 675248928970d33f7fc8ca9851a170c98f4f1c4f
-- 

Re: [PATCH v3 1/2] KVM: x86: Check hypercall's exit to userspace generically

[Huang, Kai]

> @@ -10024,7 +10024,7 @@ unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
>  
>  	switch (nr) {
>  	case KVM_HC_VAPIC_POLL_IRQ:
> -		ret = 0;
> +		ret = 1;
>  		break;
>  	case KVM_HC_KICK_CPU:
>  		if (!guest_pv_has(vcpu, KVM_FEATURE_PV_UNHALT))
> @@ -10032,7 +10032,7 @@ unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
>  
>  		kvm_pv_kick_cpu_op(vcpu->kvm, a1);
>  		kvm_sched_yield(vcpu, a1);
> -		ret = 0;
> +		ret = 1;
>  		break;
>  #ifdef CONFIG_X86_64
>  	case KVM_HC_CLOCK_PAIRING:
> @@ -10050,7 +10050,7 @@ unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
>  			break;
>  
>  		kvm_sched_yield(vcpu, a0);
> -		ret = 0;
> +		ret = 1;
>  		break;
>  	case KVM_HC_MAP_GPA_RANGE: {
>  		u64 gpa = a0, npages = a1, attrs = a2;
> @@ -10111,12 +10111,21 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
>  	cpl = kvm_x86_call(get_cpl)(vcpu);
>  
>  	ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl);
> -	if (nr == KVM_HC_MAP_GPA_RANGE && !ret)
> -		/* MAP_GPA tosses the request to the user space. */
> +	if (!ret)
>  		return 0;
>  
> -	if (!op_64_bit)
> +	/*
> +	 * KVM's ABI with the guest is that '0' is success, and any other value
> +	 * is an error code.  Internally, '0' == exit to userspace (see above)
> +	 * and '1' == success, as KVM's de facto standard return codes are that
> +	 * plus -errno == failure.  Explicitly check for '1' as some PV error
> +	 * codes are positive values.
> +	 */
> +	if (ret == 1)
> +		ret = 0;
> +	else if (!op_64_bit)
>  		ret = (u32)ret;
> +
>  	kvm_rax_write(vcpu, ret);
>  
>  	return kvm_skip_emulated_instruction(vcpu);
> 

It appears below two cases are not covered correctly?

#ifdef CONFIG_X86_64    
        case KVM_HC_CLOCK_PAIRING:
                ret = kvm_pv_clock_pairing(vcpu, a0, a1);
                break;
#endif
        case KVM_HC_SEND_IPI:
                if (!guest_pv_has(vcpu, KVM_FEATURE_PV_SEND_IPI))
                        break;

                ret = kvm_pv_send_ipi(vcpu->kvm, a0, a1, a2, a3, op_64_bit);
                break; 

Looking at the code, seems at least for KVM_HC_CLOCK_PAIRING,
kvm_pv_clock_pairing() returns 0 on success, and the upstream behaviour is not
routing to userspace but writing 0 to vcpu's RAX and returning to guest.  With
the change above, it immediately returns to userspace w/o writing 0 to RAX.

For KVM_HC_SEND_IPI, seems the upstream behaviour is the return value of
kvm_pv_send_ipi() is always written to vcpu's RAX reg, and we always just go
back to guest.  With your change, the behaviour will be changed to:

  1) when ret == 0, exit to userspace w/o writing  0 to vcpu's RAX,
  2) when ret == 1, it is converted to 0 and then written to RAX.

This doesn't look like safe.

Re: [PATCH v3 1/2] KVM: x86: Check hypercall's exit to userspace generically

[Sean Christopherson]

On Thu, Oct 31, 2024, Kai Huang wrote:
> > @@ -10111,12 +10111,21 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
> >  	cpl = kvm_x86_call(get_cpl)(vcpu);
> >  
> >  	ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl);
> > -	if (nr == KVM_HC_MAP_GPA_RANGE && !ret)
> > -		/* MAP_GPA tosses the request to the user space. */
> > +	if (!ret)
> >  		return 0;
> >  
> > -	if (!op_64_bit)
> > +	/*
> > +	 * KVM's ABI with the guest is that '0' is success, and any other value
> > +	 * is an error code.  Internally, '0' == exit to userspace (see above)
> > +	 * and '1' == success, as KVM's de facto standard return codes are that
> > +	 * plus -errno == failure.  Explicitly check for '1' as some PV error
> > +	 * codes are positive values.
> > +	 */
> > +	if (ret == 1)
> > +		ret = 0;
> > +	else if (!op_64_bit)
> >  		ret = (u32)ret;
> > +
> >  	kvm_rax_write(vcpu, ret);
> >  
> >  	return kvm_skip_emulated_instruction(vcpu);
> > 
> 
> It appears below two cases are not covered correctly?
> 
> #ifdef CONFIG_X86_64    
>         case KVM_HC_CLOCK_PAIRING:
>                 ret = kvm_pv_clock_pairing(vcpu, a0, a1);
>                 break;
> #endif
>         case KVM_HC_SEND_IPI:
>                 if (!guest_pv_has(vcpu, KVM_FEATURE_PV_SEND_IPI))
>                         break;
> 
>                 ret = kvm_pv_send_ipi(vcpu->kvm, a0, a1, a2, a3, op_64_bit);
>                 break; 
> 
> Looking at the code, seems at least for KVM_HC_CLOCK_PAIRING,
> kvm_pv_clock_pairing() returns 0 on success, and the upstream behaviour is not
> routing to userspace but writing 0 to vcpu's RAX and returning to guest.  With
> the change above, it immediately returns to userspace w/o writing 0 to RAX.
> 
> For KVM_HC_SEND_IPI, seems the upstream behaviour is the return value of
> kvm_pv_send_ipi() is always written to vcpu's RAX reg, and we always just go
> back to guest.  With your change, the behaviour will be changed to:
> 
>   1) when ret == 0, exit to userspace w/o writing  0 to vcpu's RAX,
>   2) when ret == 1, it is converted to 0 and then written to RAX.
> 
> This doesn't look like safe.

Drat, I managed to space on the cases that didn't explicit set '0'.  Hrm.

My other idea was have an out-param to separate the return code intended for KVM
from the return code intended for the guest.  I generally dislike out-params, but
trying to juggle a return value that multiplexes guest and host values seems like
an even worse idea.

Also completely untested...

---
 arch/x86/include/asm/kvm_host.h |  8 +++----
 arch/x86/kvm/x86.c              | 41 +++++++++++++++------------------
 2 files changed, 23 insertions(+), 26 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 6d9f763a7bb9..226df5c56811 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -2179,10 +2179,10 @@ static inline void kvm_clear_apicv_inhibit(struct kvm *kvm,
 	kvm_set_or_clear_apicv_inhibit(kvm, reason, false);
 }
 
-unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
-				      unsigned long a0, unsigned long a1,
-				      unsigned long a2, unsigned long a3,
-				      int op_64_bit, int cpl);
+int __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
+			    unsigned long a0, unsigned long a1,
+			    unsigned long a2, unsigned long a3,
+			    int op_64_bit, int cpl, unsigned long *ret);
 int kvm_emulate_hypercall(struct kvm_vcpu *vcpu);
 
 int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, u64 error_code,
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index e09daa3b157c..e9ae09f1b45b 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -9998,13 +9998,11 @@ static int complete_hypercall_exit(struct kvm_vcpu *vcpu)
 	return kvm_skip_emulated_instruction(vcpu);
 }
 
-unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
-				      unsigned long a0, unsigned long a1,
-				      unsigned long a2, unsigned long a3,
-				      int op_64_bit, int cpl)
+int __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
+			    unsigned long a0, unsigned long a1,
+			    unsigned long a2, unsigned long a3,
+			    int op_64_bit, int cpl, unsigned long *ret)
 {
-	unsigned long ret;
-
 	trace_kvm_hypercall(nr, a0, a1, a2, a3);
 
 	if (!op_64_bit) {
@@ -10016,15 +10014,15 @@ unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
 	}
 
 	if (cpl) {
-		ret = -KVM_EPERM;
+		*ret = -KVM_EPERM;
 		goto out;
 	}
 
-	ret = -KVM_ENOSYS;
+	*ret = -KVM_ENOSYS;
 
 	switch (nr) {
 	case KVM_HC_VAPIC_POLL_IRQ:
-		ret = 0;
+		*ret = 0;
 		break;
 	case KVM_HC_KICK_CPU:
 		if (!guest_pv_has(vcpu, KVM_FEATURE_PV_UNHALT))
@@ -10032,36 +10030,36 @@ unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
 
 		kvm_pv_kick_cpu_op(vcpu->kvm, a1);
 		kvm_sched_yield(vcpu, a1);
-		ret = 0;
+		*ret = 0;
 		break;
 #ifdef CONFIG_X86_64
 	case KVM_HC_CLOCK_PAIRING:
-		ret = kvm_pv_clock_pairing(vcpu, a0, a1);
+		*ret = kvm_pv_clock_pairing(vcpu, a0, a1);
 		break;
 #endif
 	case KVM_HC_SEND_IPI:
 		if (!guest_pv_has(vcpu, KVM_FEATURE_PV_SEND_IPI))
 			break;
 
-		ret = kvm_pv_send_ipi(vcpu->kvm, a0, a1, a2, a3, op_64_bit);
+		*ret = kvm_pv_send_ipi(vcpu->kvm, a0, a1, a2, a3, op_64_bit);
 		break;
 	case KVM_HC_SCHED_YIELD:
 		if (!guest_pv_has(vcpu, KVM_FEATURE_PV_SCHED_YIELD))
 			break;
 
 		kvm_sched_yield(vcpu, a0);
-		ret = 0;
+		*ret = 0;
 		break;
 	case KVM_HC_MAP_GPA_RANGE: {
 		u64 gpa = a0, npages = a1, attrs = a2;
 
-		ret = -KVM_ENOSYS;
+		*ret = -KVM_ENOSYS;
 		if (!user_exit_on_hypercall(vcpu->kvm, KVM_HC_MAP_GPA_RANGE))
 			break;
 
 		if (!PAGE_ALIGNED(gpa) || !npages ||
 		    gpa_to_gfn(gpa) + npages <= gpa_to_gfn(gpa)) {
-			ret = -KVM_EINVAL;
+			*ret = -KVM_EINVAL;
 			break;
 		}
 
@@ -10080,13 +10078,13 @@ unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
 		return 0;
 	}
 	default:
-		ret = -KVM_ENOSYS;
+		*ret = -KVM_ENOSYS;
 		break;
 	}
 
 out:
 	++vcpu->stat.hypercalls;
-	return ret;
+	return 1;
 }
 EXPORT_SYMBOL_GPL(__kvm_emulate_hypercall);
 
@@ -10094,7 +10092,7 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
 {
 	unsigned long nr, a0, a1, a2, a3, ret;
 	int op_64_bit;
-	int cpl;
+	int cpl, r;
 
 	if (kvm_xen_hypercall_enabled(vcpu->kvm))
 		return kvm_xen_hypercall(vcpu);
@@ -10110,10 +10108,9 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
 	op_64_bit = is_64_bit_hypercall(vcpu);
 	cpl = kvm_x86_call(get_cpl)(vcpu);
 
-	ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl);
-	if (nr == KVM_HC_MAP_GPA_RANGE && !ret)
-		/* MAP_GPA tosses the request to the user space. */
-		return 0;
+	r = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl, &ret);
+	if (r <= r)
+		return r;
 
 	if (!op_64_bit)
 		ret = (u32)ret;

base-commit: 675248928970d33f7fc8ca9851a170c98f4f1c4f
-- 

Re: [PATCH v3 1/2] KVM: x86: Check hypercall's exit to userspace generically

[Binbin Wu]

On 10/31/2024 10:54 PM, Sean Christopherson wrote:
> My other idea was have an out-param to separate the return code intended for KVM
> from the return code intended for the guest.  I generally dislike out-params, but
> trying to juggle a return value that multiplexes guest and host values seems like
> an even worse idea.
>
> Also completely untested...
>
> ---
>   arch/x86/include/asm/kvm_host.h |  8 +++----
>   arch/x86/kvm/x86.c              | 41 +++++++++++++++------------------
>   2 files changed, 23 insertions(+), 26 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 6d9f763a7bb9..226df5c56811 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -2179,10 +2179,10 @@ static inline void kvm_clear_apicv_inhibit(struct kvm *kvm,
>   	kvm_set_or_clear_apicv_inhibit(kvm, reason, false);
>   }
>   
> -unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
> -				      unsigned long a0, unsigned long a1,
> -				      unsigned long a2, unsigned long a3,
> -				      int op_64_bit, int cpl);
> +int __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
> +			    unsigned long a0, unsigned long a1,
> +			    unsigned long a2, unsigned long a3,
> +			    int op_64_bit, int cpl, unsigned long *ret);
>   int kvm_emulate_hypercall(struct kvm_vcpu *vcpu);
>   
>   int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, u64 error_code,
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index e09daa3b157c..e9ae09f1b45b 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -9998,13 +9998,11 @@ static int complete_hypercall_exit(struct kvm_vcpu *vcpu)
>   	return kvm_skip_emulated_instruction(vcpu);
>   }
>   
> -unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
> -				      unsigned long a0, unsigned long a1,
> -				      unsigned long a2, unsigned long a3,
> -				      int op_64_bit, int cpl)
> +int __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
> +			    unsigned long a0, unsigned long a1,
> +			    unsigned long a2, unsigned long a3,
> +			    int op_64_bit, int cpl, unsigned long *ret)
>   {
> -	unsigned long ret;
> -
>   	trace_kvm_hypercall(nr, a0, a1, a2, a3);
>   
>   	if (!op_64_bit) {
> @@ -10016,15 +10014,15 @@ unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
>   	}
>   
>   	if (cpl) {
> -		ret = -KVM_EPERM;
> +		*ret = -KVM_EPERM;
>   		goto out;
>   	}
>   
> -	ret = -KVM_ENOSYS;
> +	*ret = -KVM_ENOSYS;
>   
>   	switch (nr) {
>   	case KVM_HC_VAPIC_POLL_IRQ:
> -		ret = 0;
> +		*ret = 0;
>   		break;
>   	case KVM_HC_KICK_CPU:
>   		if (!guest_pv_has(vcpu, KVM_FEATURE_PV_UNHALT))
> @@ -10032,36 +10030,36 @@ unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
>   
>   		kvm_pv_kick_cpu_op(vcpu->kvm, a1);
>   		kvm_sched_yield(vcpu, a1);
> -		ret = 0;
> +		*ret = 0;
>   		break;
>   #ifdef CONFIG_X86_64
>   	case KVM_HC_CLOCK_PAIRING:
> -		ret = kvm_pv_clock_pairing(vcpu, a0, a1);
> +		*ret = kvm_pv_clock_pairing(vcpu, a0, a1);
>   		break;
>   #endif
>   	case KVM_HC_SEND_IPI:
>   		if (!guest_pv_has(vcpu, KVM_FEATURE_PV_SEND_IPI))
>   			break;
>   
> -		ret = kvm_pv_send_ipi(vcpu->kvm, a0, a1, a2, a3, op_64_bit);
> +		*ret = kvm_pv_send_ipi(vcpu->kvm, a0, a1, a2, a3, op_64_bit);
>   		break;
>   	case KVM_HC_SCHED_YIELD:
>   		if (!guest_pv_has(vcpu, KVM_FEATURE_PV_SCHED_YIELD))
>   			break;
>   
>   		kvm_sched_yield(vcpu, a0);
> -		ret = 0;
> +		*ret = 0;
>   		break;
>   	case KVM_HC_MAP_GPA_RANGE: {
>   		u64 gpa = a0, npages = a1, attrs = a2;
>   
> -		ret = -KVM_ENOSYS;
> +		*ret = -KVM_ENOSYS;
>   		if (!user_exit_on_hypercall(vcpu->kvm, KVM_HC_MAP_GPA_RANGE))
>   			break;
>   
>   		if (!PAGE_ALIGNED(gpa) || !npages ||
>   		    gpa_to_gfn(gpa) + npages <= gpa_to_gfn(gpa)) {
> -			ret = -KVM_EINVAL;
> +			*ret = -KVM_EINVAL;
>   			break;
>   		}

*ret needs to be set to 0 for this case before returning 0 to caller?

>   
> @@ -10080,13 +10078,13 @@ unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
>   		return 0;
>   	}
>   	default:
> -		ret = -KVM_ENOSYS;
> +		*ret = -KVM_ENOSYS;
>   		break;
>   	}
>   
>   out:
>   	++vcpu->stat.hypercalls;
> -	return ret;
> +	return 1;
>   }
>   EXPORT_SYMBOL_GPL(__kvm_emulate_hypercall);
>   
> @@ -10094,7 +10092,7 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
>   {
>   	unsigned long nr, a0, a1, a2, a3, ret;
>   	int op_64_bit;
> -	int cpl;
> +	int cpl, r;
>   
>   	if (kvm_xen_hypercall_enabled(vcpu->kvm))
>   		return kvm_xen_hypercall(vcpu);
> @@ -10110,10 +10108,9 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
>   	op_64_bit = is_64_bit_hypercall(vcpu);
>   	cpl = kvm_x86_call(get_cpl)(vcpu);
>   
> -	ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl);
> -	if (nr == KVM_HC_MAP_GPA_RANGE && !ret)
> -		/* MAP_GPA tosses the request to the user space. */
> -		return 0;
> +	r = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl, &ret);
> +	if (r <= r)
A typo here.
I guess it meant to be "if (r <= ret)" ?

So the combinations will be
----------------------------------------------------------------------------
    |  r  |    ret    | r <= ret |
---|-----|-----------|----------|-------------------------------------------
  1 |  0  |     0     |   true   |  return r, which is 0, exit to userspace
---|-----|-----------|----------|-------------------------------------------
  2 |  1  |     0     |   false  |  set vcpu's RAX and return back to guest
---|-----|-----------|----------|-------------------------------------------
  3 |  1  | -KVM_Exxx |   false  |  set vcpu's RAX and return back to guest
---|-----|-----------|----------|-------------------------------------------
  4 |  1  |  Positive |   true   |  return r, which is 1,
    |     |     N     |          |  back to guest without setting vcpu's RAX
----------------------------------------------------------------------------

KVM_HC_SEND_IPI, which calls kvm_pv_send_ipi() can hit case 4, which will
return back to guest without setting RAX. It is different from the current behavior.

r can be 0 only if there is no other error detected during pre-checks.
I think it can just check whether r is 0 or not.
I.e.,

@@ -10094,7 +10092,7 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
  {
      unsigned long nr, a0, a1, a2, a3, ret;
      int op_64_bit;
-    int cpl;
+    int cpl, r;

      if (kvm_xen_hypercall_enabled(vcpu->kvm))
          return kvm_xen_hypercall(vcpu);
@@ -10110,10 +10108,9 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
      op_64_bit = is_64_bit_hypercall(vcpu);
      cpl = kvm_x86_call(get_cpl)(vcpu);

-    ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl);
-    if (nr == KVM_HC_MAP_GPA_RANGE && !ret)
-        /* MAP_GPA tosses the request to the user space. */
-        return 0;
+    r = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl, &ret);
+    if (!r)
+        return 0;

      if (!op_64_bit)
          ret = (u32)ret;


> +		return r;
>   
>   	if (!op_64_bit)
>   		ret = (u32)ret;
>
> base-commit: 675248928970d33f7fc8ca9851a170c98f4f1c4f

Re: [PATCH v3 1/2] KVM: x86: Check hypercall's exit to userspace generically

[Sean Christopherson]

On Fri, Nov 01, 2024, Binbin Wu wrote:
> On 10/31/2024 10:54 PM, Sean Christopherson wrote:
> > My other idea was have an out-param to separate the return code intended for KVM
> > from the return code intended for the guest.  I generally dislike out-params, but
> > trying to juggle a return value that multiplexes guest and host values seems like
> > an even worse idea.
> > 
> > Also completely untested...

...

> >   	case KVM_HC_MAP_GPA_RANGE: {
> >   		u64 gpa = a0, npages = a1, attrs = a2;
> > -		ret = -KVM_ENOSYS;
> > +		*ret = -KVM_ENOSYS;
> >   		if (!user_exit_on_hypercall(vcpu->kvm, KVM_HC_MAP_GPA_RANGE))
> >   			break;
> >   		if (!PAGE_ALIGNED(gpa) || !npages ||
> >   		    gpa_to_gfn(gpa) + npages <= gpa_to_gfn(gpa)) {
> > -			ret = -KVM_EINVAL;
> > +			*ret = -KVM_EINVAL;
> >   			break;
> >   		}
> 
> *ret needs to be set to 0 for this case before returning 0 to caller?

No, because the caller should consume *ret if and only if the function return value
is '1', i.e. iff KVM should resume the guest.  And I think we actually want to
intentionally not touch *ret, because a sufficient smart compiler (or static
analysis tool) should be able to detect that incorrect usage of *ret is consuming
uninitialized data.

> > @@ -10080,13 +10078,13 @@ unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
> >   		return 0;
> >   	}
> >   	default:
> > -		ret = -KVM_ENOSYS;
> > +		*ret = -KVM_ENOSYS;
> >   		break;
> >   	}
> >   out:
> >   	++vcpu->stat.hypercalls;
> > -	return ret;
> > +	return 1;
> >   }
> >   EXPORT_SYMBOL_GPL(__kvm_emulate_hypercall);
> > @@ -10094,7 +10092,7 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
> >   {
> >   	unsigned long nr, a0, a1, a2, a3, ret;
> >   	int op_64_bit;
> > -	int cpl;
> > +	int cpl, r;
> >   	if (kvm_xen_hypercall_enabled(vcpu->kvm))
> >   		return kvm_xen_hypercall(vcpu);
> > @@ -10110,10 +10108,9 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
> >   	op_64_bit = is_64_bit_hypercall(vcpu);
> >   	cpl = kvm_x86_call(get_cpl)(vcpu);
> > -	ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl);
> > -	if (nr == KVM_HC_MAP_GPA_RANGE && !ret)
> > -		/* MAP_GPA tosses the request to the user space. */
> > -		return 0;
> > +	r = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl, &ret);
> > +	if (r <= r)
> A typo here.
> I guess it meant to be "if (r <= ret)" ?

No, "if (r <= 0)", i.e. exit to userspace on 0 or -errno.

> So the combinations will be
> ----------------------------------------------------------------------------
>    |  r  |    ret    | r <= ret |
> ---|-----|-----------|----------|-------------------------------------------
>  1 |  0  |     0     |   true   |  return r, which is 0, exit to userspace
> ---|-----|-----------|----------|-------------------------------------------
>  2 |  1  |     0     |   false  |  set vcpu's RAX and return back to guest
> ---|-----|-----------|----------|-------------------------------------------
>  3 |  1  | -KVM_Exxx |   false  |  set vcpu's RAX and return back to guest
> ---|-----|-----------|----------|-------------------------------------------
>  4 |  1  |  Positive |   true   |  return r, which is 1,
>    |     |     N     |          |  back to guest without setting vcpu's RAX
> ----------------------------------------------------------------------------
> 
> KVM_HC_SEND_IPI, which calls kvm_pv_send_ipi() can hit case 4, which will
> return back to guest without setting RAX. It is different from the current behavior.
> 
> r can be 0 only if there is no other error detected during pre-checks.
> I think it can just check whether r is 0 or not.

Yeah, I just fat fingered the code (and didn't even compile test).

Re: [PATCH v3 1/2] KVM: x86: Check hypercall's exit to userspace generically

[Huang, Kai]

On Thu, 2024-10-31 at 07:54 -0700, Sean Christopherson wrote:
> On Thu, Oct 31, 2024, Kai Huang wrote:
> > > @@ -10111,12 +10111,21 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
> > >  	cpl = kvm_x86_call(get_cpl)(vcpu);
> > >  
> > >  	ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl);
> > > -	if (nr == KVM_HC_MAP_GPA_RANGE && !ret)
> > > -		/* MAP_GPA tosses the request to the user space. */
> > > +	if (!ret)
> > >  		return 0;
> > >  
> > > -	if (!op_64_bit)
> > > +	/*
> > > +	 * KVM's ABI with the guest is that '0' is success, and any other value
> > > +	 * is an error code.  Internally, '0' == exit to userspace (see above)
> > > +	 * and '1' == success, as KVM's de facto standard return codes are that
> > > +	 * plus -errno == failure.  Explicitly check for '1' as some PV error
> > > +	 * codes are positive values.
> > > +	 */
> > > +	if (ret == 1)
> > > +		ret = 0;
> > > +	else if (!op_64_bit)
> > >  		ret = (u32)ret;
> > > +
> > >  	kvm_rax_write(vcpu, ret);
> > >  
> > >  	return kvm_skip_emulated_instruction(vcpu);
> > > 
> > 
> > It appears below two cases are not covered correctly?
> > 
> > #ifdef CONFIG_X86_64    
> >         case KVM_HC_CLOCK_PAIRING:
> >                 ret = kvm_pv_clock_pairing(vcpu, a0, a1);
> >                 break;
> > #endif
> >         case KVM_HC_SEND_IPI:
> >                 if (!guest_pv_has(vcpu, KVM_FEATURE_PV_SEND_IPI))
> >                         break;
> > 
> >                 ret = kvm_pv_send_ipi(vcpu->kvm, a0, a1, a2, a3, op_64_bit);
> >                 break; 
> > 
> > Looking at the code, seems at least for KVM_HC_CLOCK_PAIRING,
> > kvm_pv_clock_pairing() returns 0 on success, and the upstream behaviour is not
> > routing to userspace but writing 0 to vcpu's RAX and returning to guest.  With
> > the change above, it immediately returns to userspace w/o writing 0 to RAX.
> > 
> > For KVM_HC_SEND_IPI, seems the upstream behaviour is the return value of
> > kvm_pv_send_ipi() is always written to vcpu's RAX reg, and we always just go
> > back to guest.  With your change, the behaviour will be changed to:
> > 
> >   1) when ret == 0, exit to userspace w/o writing  0 to vcpu's RAX,
> >   2) when ret == 1, it is converted to 0 and then written to RAX.
> > 
> > This doesn't look like safe.
> 
> Drat, I managed to space on the cases that didn't explicit set '0'.  Hrm.
> 
> My other idea was have an out-param to separate the return code intended for KVM
> from the return code intended for the guest.  I generally dislike out-params, but
> trying to juggle a return value that multiplexes guest and host values seems like
> an even worse idea.

Yeah this looks fine to me, one comment below ...


[...]

> -	ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl);
> -	if (nr == KVM_HC_MAP_GPA_RANGE && !ret)
> -		/* MAP_GPA tosses the request to the user space. */
> -		return 0;
> +	r = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl, &ret);
> +	if (r <= r)
> +		return r;

... should be:

	if (r <= 0)
		return r;

?

Another option might be we move "set hypercall return value" code inside
__kvm_emulate_hypercall().  So IIUC the reason to split
__kvm_emulate_hypercall() out is for TDX, and while non-TDX uses RAX to carry
the hypercall return value, TDX uses R10.

We can additionally pass a "kvm_hypercall_set_ret_func" function pointer to
__kvm_emulate_hypercall(), and invoke it inside.  Then we can change
__kvm_emulate_hypercall() to return: 
    < 0 error, 
    ==0 return to userspace, 
    > 0 go back to guest.

I made some attempt below, build test only:

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 6d9f763a7bb9..c48feb62a5d7 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -2179,10 +2179,14 @@ static inline void kvm_clear_apicv_inhibit(struct kvm
*kvm,
        kvm_set_or_clear_apicv_inhibit(kvm, reason, false);
 }
 
-unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
-                                     unsigned long a0, unsigned long a1,
-                                     unsigned long a2, unsigned long a3,
-                                     int op_64_bit, int cpl);
+typedef void (*kvm_hypercall_set_ret_func_t)(struct kvm_vcpu *vcpu,
+               unsigned long val);
+
+int __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
+                           unsigned long a0, unsigned long a1,
+                           unsigned long a2, unsigned long a3,
+                           int op_64_bit, int cpl,
+                           kvm_hypercall_set_ret_func_t set_ret_func);
 int kvm_emulate_hypercall(struct kvm_vcpu *vcpu);
 
 int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, u64 error_code,
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 83fe0a78146f..01bdf01cfc79 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -9998,10 +9998,11 @@ static int complete_hypercall_exit(struct kvm_vcpu
*vcpu)
        return kvm_skip_emulated_instruction(vcpu);
 }
 
-unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
-                                     unsigned long a0, unsigned long a1,
-                                     unsigned long a2, unsigned long a3,
-                                     int op_64_bit, int cpl)
+int __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
+                           unsigned long a0, unsigned long a1,
+                           unsigned long a2, unsigned long a3,
+                           int op_64_bit, int cpl,
+                           kvm_hypercall_set_ret_func_t set_ret_func)
 {
        unsigned long ret;
 
@@ -10076,6 +10077,7 @@ unsigned long __kvm_emulate_hypercall(struct kvm_vcpu
*vcpu, unsigned long nr,
 
                WARN_ON_ONCE(vcpu->run->hypercall.flags &
KVM_EXIT_HYPERCALL_MBZ);
                vcpu->arch.complete_userspace_io = complete_hypercall_exit;
+               /* MAP_GPA tosses the request to the user space. */
                /* stat is incremented on completion. */
                return 0;
        }
@@ -10085,16 +10087,26 @@ unsigned long __kvm_emulate_hypercall(struct kvm_vcpu
*vcpu, unsigned long nr,
        }
 
 out:
+       if (!op_64_bit)
+               ret = (u32)ret;
+       (*set_ret_func)(vcpu, ret);
+
        ++vcpu->stat.hypercalls;
-       return ret;
+       return 1;
 }
 EXPORT_SYMBOL_GPL(__kvm_emulate_hypercall);
 
+static void kvm_hypercall_set_ret_default(struct kvm_vcpu *vcpu,
+                                         unsigned long val)
+{
+       kvm_rax_write(vcpu, val);
+}
+
 int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
 {
-       unsigned long nr, a0, a1, a2, a3, ret;
+       unsigned long nr, a0, a1, a2, a3;
        int op_64_bit;
-       int cpl;
+       int cpl, ret;
 
        if (kvm_xen_hypercall_enabled(vcpu->kvm))
                return kvm_xen_hypercall(vcpu);
@@ -10110,14 +10122,10 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
        op_64_bit = is_64_bit_hypercall(vcpu);
        cpl = kvm_x86_call(get_cpl)(vcpu);
 
-       ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl);
-       if (nr == KVM_HC_MAP_GPA_RANGE && !ret)
-               /* MAP_GPA tosses the request to the user space. */
-               return 0;
-
-       if (!op_64_bit)
-               ret = (u32)ret;
-       kvm_rax_write(vcpu, ret);
+       ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl,
+                       kvm_hypercall_set_ret_default);
+       if (ret <= 0)
+               return ret;
 
        return kvm_skip_emulated_instruction(vcpu);
 }

Re: [PATCH v3 1/2] KVM: x86: Check hypercall's exit to userspace generically

[Sean Christopherson]

On Fri, Nov 01, 2024, Kai Huang wrote:
> On Thu, 2024-10-31 at 07:54 -0700, Sean Christopherson wrote:
> > On Thu, Oct 31, 2024, Kai Huang wrote:
> > -	ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl);
> > -	if (nr == KVM_HC_MAP_GPA_RANGE && !ret)
> > -		/* MAP_GPA tosses the request to the user space. */
> > -		return 0;
> > +	r = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl, &ret);
> > +	if (r <= r)
> > +		return r;
> 
> ... should be:
> 
> 	if (r <= 0)
> 		return r;
> 
> ?
> 
> Another option might be we move "set hypercall return value" code inside
> __kvm_emulate_hypercall().  So IIUC the reason to split
> __kvm_emulate_hypercall() out is for TDX, and while non-TDX uses RAX to carry
> the hypercall return value, TDX uses R10.
> 
> We can additionally pass a "kvm_hypercall_set_ret_func" function pointer to
> __kvm_emulate_hypercall(), and invoke it inside.  Then we can change
> __kvm_emulate_hypercall() to return: 
>     < 0 error, 
>     ==0 return to userspace, 
>     > 0 go back to guest.

Hmm, and the caller can still handle kvm_skip_emulated_instruction(), because the
return value is KVM's normal pattern.

I like it!

But, there's no need to pass a function pointer, KVM can write (and read) arbitrary
GPRs, it's just avoided in most cases so that the sanity checks and available/dirty
updates are elided.  For this code though, it's easy enough to keep kvm_rxx_read()
for getting values, and eating the overhead of a single GPR write is a perfectly
fine tradeoff for eliminating the return multiplexing.

Lightly tested.  Assuming this works for TDX and passes testing, I'll post a
mini-series next week.

--
From: Sean Christopherson <seanjc@google.com>
Date: Fri, 1 Nov 2024 09:04:00 -0700
Subject: [PATCH] KVM: x86: Refactor __kvm_emulate_hypercall() to accept reg
 names, not values

Rework __kvm_emulate_hypercall() to take the names of input and output
(guest return value) registers, as opposed to taking the input values and
returning the output value.  As part of the refactor, change the actual
return value from __kvm_emulate_hypercall() to be KVM's de facto standard
of '0' == exit to userspace, '1' == resume guest, and -errno == failure.

Using the return value for KVM's control flow eliminates the multiplexed
return value, where '0' for KVM_HC_MAP_GPA_RANGE (and only that hypercall)
means "exit to userspace".

Use the direct GPR accessors to read values to avoid the pointless marking
of the registers as available, but use kvm_register_write_raw() for the
guest return value so that the innermost helper doesn't need to multiplex
its return value.  Using the generic kvm_register_write_raw() adds very
minimal overhead, so as a one-off in a relatively slow path it's well
worth the code simplification.

Suggested-by: Kai Huang <kai.huang@intel.com>
Signed-off-by: Sean Christopherson <seanjc@google.com>
---
 arch/x86/include/asm/kvm_host.h | 15 +++++++++----
 arch/x86/kvm/x86.c              | 40 +++++++++++++--------------------
 2 files changed, 27 insertions(+), 28 deletions(-)

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 6d9f763a7bb9..9e66fde1c4e4 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -2179,10 +2179,17 @@ static inline void kvm_clear_apicv_inhibit(struct kvm *kvm,
 	kvm_set_or_clear_apicv_inhibit(kvm, reason, false);
 }
 
-unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
-				      unsigned long a0, unsigned long a1,
-				      unsigned long a2, unsigned long a3,
-				      int op_64_bit, int cpl);
+int ____kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
+			      unsigned long a0, unsigned long a1,
+			      unsigned long a2, unsigned long a3,
+			      int op_64_bit, int cpl, int ret_reg);
+
+#define __kvm_emulate_hypercall(_vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl, ret)	\
+	____kvm_emulate_hypercall(vcpu,						\
+				  kvm_##nr##_read(vcpu), kvm_##a0##_read(vcpu),	\
+				  kvm_##a1##_read(vcpu), kvm_##a2##_read(vcpu),	\
+				  kvm_##a3##_read(vcpu), op_64_bit, cpl, VCPU_REGS_##ret)
+
 int kvm_emulate_hypercall(struct kvm_vcpu *vcpu);
 
 int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, u64 error_code,
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index e09daa3b157c..425a301911a6 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -9998,10 +9998,10 @@ static int complete_hypercall_exit(struct kvm_vcpu *vcpu)
 	return kvm_skip_emulated_instruction(vcpu);
 }
 
-unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
-				      unsigned long a0, unsigned long a1,
-				      unsigned long a2, unsigned long a3,
-				      int op_64_bit, int cpl)
+int ____kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
+			      unsigned long a0, unsigned long a1,
+			      unsigned long a2, unsigned long a3,
+			      int op_64_bit, int cpl, int ret_reg)
 {
 	unsigned long ret;
 
@@ -10086,15 +10086,18 @@ unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
 
 out:
 	++vcpu->stat.hypercalls;
-	return ret;
+
+	if (!op_64_bit)
+		ret = (u32)ret;
+
+	kvm_register_write_raw(vcpu, ret_reg, ret);
+	return 1;
 }
-EXPORT_SYMBOL_GPL(__kvm_emulate_hypercall);
+EXPORT_SYMBOL_GPL(____kvm_emulate_hypercall);
 
 int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
 {
-	unsigned long nr, a0, a1, a2, a3, ret;
-	int op_64_bit;
-	int cpl;
+	int r;
 
 	if (kvm_xen_hypercall_enabled(vcpu->kvm))
 		return kvm_xen_hypercall(vcpu);
@@ -10102,23 +10105,12 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
 	if (kvm_hv_hypercall_enabled(vcpu))
 		return kvm_hv_hypercall(vcpu);
 
-	nr = kvm_rax_read(vcpu);
-	a0 = kvm_rbx_read(vcpu);
-	a1 = kvm_rcx_read(vcpu);
-	a2 = kvm_rdx_read(vcpu);
-	a3 = kvm_rsi_read(vcpu);
-	op_64_bit = is_64_bit_hypercall(vcpu);
-	cpl = kvm_x86_call(get_cpl)(vcpu);
-
-	ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl);
-	if (nr == KVM_HC_MAP_GPA_RANGE && !ret)
-		/* MAP_GPA tosses the request to the user space. */
+	r = __kvm_emulate_hypercall(vcpu, rax, rbx, rcx, rdx, rsi,
+				    is_64_bit_hypercall(vcpu),
+				    kvm_x86_call(get_cpl)(vcpu), RAX);
+	if (r <= 0)
 		return 0;
 
-	if (!op_64_bit)
-		ret = (u32)ret;
-	kvm_rax_write(vcpu, ret);
-
 	return kvm_skip_emulated_instruction(vcpu);
 }
 EXPORT_SYMBOL_GPL(kvm_emulate_hypercall);

base-commit: 911785b796e325dec83b32050f294e278a306211
-- 

Re: [PATCH v3 1/2] KVM: x86: Check hypercall's exit to userspace generically

[Huang, Kai]

On Fri, 2024-11-01 at 09:39 -0700, Sean Christopherson wrote:
> On Fri, Nov 01, 2024, Kai Huang wrote:
> > On Thu, 2024-10-31 at 07:54 -0700, Sean Christopherson wrote:
> > > On Thu, Oct 31, 2024, Kai Huang wrote:
> > > -	ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl);
> > > -	if (nr == KVM_HC_MAP_GPA_RANGE && !ret)
> > > -		/* MAP_GPA tosses the request to the user space. */
> > > -		return 0;
> > > +	r = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl, &ret);
> > > +	if (r <= r)
> > > +		return r;
> > 
> > ... should be:
> > 
> > 	if (r <= 0)
> > 		return r;
> > 
> > ?
> > 
> > Another option might be we move "set hypercall return value" code inside
> > __kvm_emulate_hypercall().  So IIUC the reason to split
> > __kvm_emulate_hypercall() out is for TDX, and while non-TDX uses RAX to carry
> > the hypercall return value, TDX uses R10.
> > 
> > We can additionally pass a "kvm_hypercall_set_ret_func" function pointer to
> > __kvm_emulate_hypercall(), and invoke it inside.  Then we can change
> > __kvm_emulate_hypercall() to return: 
> >     < 0 error, 
> >     ==0 return to userspace, 
> >     > 0 go back to guest.
> 
> Hmm, and the caller can still handle kvm_skip_emulated_instruction(), because the
> return value is KVM's normal pattern.
> 
> I like it!
> 
> But, there's no need to pass a function pointer, KVM can write (and read) arbitrary
> GPRs, it's just avoided in most cases so that the sanity checks and available/dirty
> updates are elided.  For this code though, it's easy enough to keep kvm_rxx_read()
> for getting values, and eating the overhead of a single GPR write is a perfectly
> fine tradeoff for eliminating the return multiplexing.
> 
> Lightly tested.  Assuming this works for TDX and passes testing, I'll post a
> mini-series next week.
> 
> --
> From: Sean Christopherson <seanjc@google.com>
> Date: Fri, 1 Nov 2024 09:04:00 -0700
> Subject: [PATCH] KVM: x86: Refactor __kvm_emulate_hypercall() to accept reg
>  names, not values
> 
> Rework __kvm_emulate_hypercall() to take the names of input and output
> (guest return value) registers, as opposed to taking the input values and
> returning the output value.  As part of the refactor, change the actual
> return value from __kvm_emulate_hypercall() to be KVM's de facto standard
> of '0' == exit to userspace, '1' == resume guest, and -errno == failure.
> 
> Using the return value for KVM's control flow eliminates the multiplexed
> return value, where '0' for KVM_HC_MAP_GPA_RANGE (and only that hypercall)
> means "exit to userspace".
> 
> Use the direct GPR accessors to read values to avoid the pointless marking
> of the registers as available, but use kvm_register_write_raw() for the
> guest return value so that the innermost helper doesn't need to multiplex
> its return value.  Using the generic kvm_register_write_raw() adds very
> minimal overhead, so as a one-off in a relatively slow path it's well
> worth the code simplification.

Ah right :-)

> 
> Suggested-by: Kai Huang <kai.huang@intel.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---

I think Binbin can help to test on TDX, and assuming it works,

Reviewed-by: Kai Huang <kai.huang@intel.com>

Re: [PATCH v3 1/2] KVM: x86: Check hypercall's exit to userspace generically

[Binbin Wu]

On 11/2/2024 5:13 AM, Huang, Kai wrote:
> On Fri, 2024-11-01 at 09:39 -0700, Sean Christopherson wrote:
>> On Fri, Nov 01, 2024, Kai Huang wrote:
>>> On Thu, 2024-10-31 at 07:54 -0700, Sean Christopherson wrote:
>>>> On Thu, Oct 31, 2024, Kai Huang wrote:
>>>> -	ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl);
>>>> -	if (nr == KVM_HC_MAP_GPA_RANGE && !ret)
>>>> -		/* MAP_GPA tosses the request to the user space. */
>>>> -		return 0;
>>>> +	r = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl, &ret);
>>>> +	if (r <= r)
>>>> +		return r;
>>> ... should be:
>>>
>>> 	if (r <= 0)
>>> 		return r;
>>>
>>> ?
>>>
>>> Another option might be we move "set hypercall return value" code inside
>>> __kvm_emulate_hypercall().  So IIUC the reason to split
>>> __kvm_emulate_hypercall() out is for TDX, and while non-TDX uses RAX to carry
>>> the hypercall return value, TDX uses R10.
>>>
>>> We can additionally pass a "kvm_hypercall_set_ret_func" function pointer to
>>> __kvm_emulate_hypercall(), and invoke it inside.  Then we can change
>>> __kvm_emulate_hypercall() to return:
>>>      < 0 error,
>>>      ==0 return to userspace,
>>>      > 0 go back to guest.
>> Hmm, and the caller can still handle kvm_skip_emulated_instruction(), because the
>> return value is KVM's normal pattern.
>>
>> I like it!
>>
>> But, there's no need to pass a function pointer, KVM can write (and read) arbitrary
>> GPRs, it's just avoided in most cases so that the sanity checks and available/dirty
>> updates are elided.  For this code though, it's easy enough to keep kvm_rxx_read()
>> for getting values, and eating the overhead of a single GPR write is a perfectly
>> fine tradeoff for eliminating the return multiplexing.
>>
>> Lightly tested.  Assuming this works for TDX and passes testing, I'll post a
>> mini-series next week.
>>
>> --
>> From: Sean Christopherson <seanjc@google.com>
>> Date: Fri, 1 Nov 2024 09:04:00 -0700
>> Subject: [PATCH] KVM: x86: Refactor __kvm_emulate_hypercall() to accept reg
>>   names, not values
>>
>> Rework __kvm_emulate_hypercall() to take the names of input and output
>> (guest return value) registers, as opposed to taking the input values and
>> returning the output value.  As part of the refactor, change the actual
>> return value from __kvm_emulate_hypercall() to be KVM's de facto standard
>> of '0' == exit to userspace, '1' == resume guest, and -errno == failure.
>>
>> Using the return value for KVM's control flow eliminates the multiplexed
>> return value, where '0' for KVM_HC_MAP_GPA_RANGE (and only that hypercall)
>> means "exit to userspace".
>>
>> Use the direct GPR accessors to read values to avoid the pointless marking
>> of the registers as available, but use kvm_register_write_raw() for the
>> guest return value so that the innermost helper doesn't need to multiplex
>> its return value.  Using the generic kvm_register_write_raw() adds very
>> minimal overhead, so as a one-off in a relatively slow path it's well
>> worth the code simplification.
> Ah right :-)
>
>> Suggested-by: Kai Huang <kai.huang@intel.com>
>> Signed-off-by: Sean Christopherson <seanjc@google.com>
>> ---
> I think Binbin can help to test on TDX, and assuming it works,
I tried to add a selftest case to do memory conversion via kvm hypercall
directly for TDX.  And found TDX code didn't handle the return value for
the hypercall properly.

I tried to add a parameter to pass the cui callback as mentioned in
https://lore.kernel.org/lkml/f95cd8c6-af5c-4d8f-99a8-16d0ec56d9a4@linux.intel.com/
And then, made the following change in TDX code to make it work.

diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index cd27ebd3d7d1..efa434c6547d 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -1072,6 +1072,15 @@ static int tdx_handle_triple_fault(struct kvm_vcpu *vcpu)
         return 0;
  }

+static int complete_hypercall_exit(struct kvm_vcpu *vcpu)
+{
+       u64 ret = vcpu->run->hypercall.ret;
+
+       kvm_r10_write(vcpu, ret);
+       ++vcpu->stat.hypercalls;
+
+       return 1;
+}
+
  static int tdx_emulate_vmcall(struct kvm_vcpu *vcpu)
  {
         int r;
@@ -1087,7 +1096,7 @@ static int tdx_emulate_vmcall(struct kvm_vcpu *vcpu)
          * R10: KVM hypercall number
          * arguments: R11, R12, R13, R14.
          */
-       r = __kvm_emulate_hypercall(vcpu, r10, r11, r12, r13, r14, true, 0, R10);
+       r = __kvm_emulate_hypercall(vcpu, r10, r11, r12, r13, r14, true, 0, R10, complete_hypercall_exit);

         return r > 0;
  }


>
> Reviewed-by: Kai Huang <kai.huang@intel.com>
>

Re: [PATCH v3 1/2] KVM: x86: Check hypercall's exit to userspace generically

[Binbin Wu]

On 11/2/2024 12:39 AM, Sean Christopherson wrote:
> On Fri, Nov 01, 2024, Kai Huang wrote:
>> On Thu, 2024-10-31 at 07:54 -0700, Sean Christopherson wrote:
>>> On Thu, Oct 31, 2024, Kai Huang wrote:
>>> -	ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl);
>>> -	if (nr == KVM_HC_MAP_GPA_RANGE && !ret)
>>> -		/* MAP_GPA tosses the request to the user space. */
>>> -		return 0;
>>> +	r = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl, &ret);
>>> +	if (r <= r)
>>> +		return r;
>> ... should be:
>>
>> 	if (r <= 0)
>> 		return r;
>>
>> ?
>>
>> Another option might be we move "set hypercall return value" code inside
>> __kvm_emulate_hypercall().  So IIUC the reason to split
>> __kvm_emulate_hypercall() out is for TDX, and while non-TDX uses RAX to carry
>> the hypercall return value, TDX uses R10.
>>
>> We can additionally pass a "kvm_hypercall_set_ret_func" function pointer to
>> __kvm_emulate_hypercall(), and invoke it inside.  Then we can change
>> __kvm_emulate_hypercall() to return:
>>      < 0 error,
>>      ==0 return to userspace,
>>      > 0 go back to guest.
> Hmm, and the caller can still handle kvm_skip_emulated_instruction(), because the
> return value is KVM's normal pattern.
>
> I like it!
>
> But, there's no need to pass a function pointer, KVM can write (and read) arbitrary
> GPRs, it's just avoided in most cases so that the sanity checks and available/dirty
> updates are elided.  For this code though, it's easy enough to keep kvm_rxx_read()
> for getting values, and eating the overhead of a single GPR write is a perfectly
> fine tradeoff for eliminating the return multiplexing.
>
> Lightly tested.  Assuming this works for TDX and passes testing, I'll post a
> mini-series next week.
>
> --
> From: Sean Christopherson <seanjc@google.com>
> Date: Fri, 1 Nov 2024 09:04:00 -0700
> Subject: [PATCH] KVM: x86: Refactor __kvm_emulate_hypercall() to accept reg
>   names, not values
>
> Rework __kvm_emulate_hypercall() to take the names of input and output
> (guest return value) registers, as opposed to taking the input values and
> returning the output value.  As part of the refactor, change the actual
> return value from __kvm_emulate_hypercall() to be KVM's de facto standard
> of '0' == exit to userspace, '1' == resume guest, and -errno == failure.
>
> Using the return value for KVM's control flow eliminates the multiplexed
> return value, where '0' for KVM_HC_MAP_GPA_RANGE (and only that hypercall)
> means "exit to userspace".
>
> Use the direct GPR accessors to read values to avoid the pointless marking
> of the registers as available, but use kvm_register_write_raw() for the
> guest return value so that the innermost helper doesn't need to multiplex
> its return value.  Using the generic kvm_register_write_raw() adds very
> minimal overhead, so as a one-off in a relatively slow path it's well
> worth the code simplification.
>
> Suggested-by: Kai Huang <kai.huang@intel.com>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>   arch/x86/include/asm/kvm_host.h | 15 +++++++++----
>   arch/x86/kvm/x86.c              | 40 +++++++++++++--------------------
>   2 files changed, 27 insertions(+), 28 deletions(-)
>
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 6d9f763a7bb9..9e66fde1c4e4 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -2179,10 +2179,17 @@ static inline void kvm_clear_apicv_inhibit(struct kvm *kvm,
>   	kvm_set_or_clear_apicv_inhibit(kvm, reason, false);
>   }
>   
> -unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
> -				      unsigned long a0, unsigned long a1,
> -				      unsigned long a2, unsigned long a3,
> -				      int op_64_bit, int cpl);
> +int ____kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
> +			      unsigned long a0, unsigned long a1,
> +			      unsigned long a2, unsigned long a3,
> +			      int op_64_bit, int cpl, int ret_reg);
> +
> +#define __kvm_emulate_hypercall(_vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl, ret)	\
> +	____kvm_emulate_hypercall(vcpu,						\
> +				  kvm_##nr##_read(vcpu), kvm_##a0##_read(vcpu),	\
> +				  kvm_##a1##_read(vcpu), kvm_##a2##_read(vcpu),	\
> +				  kvm_##a3##_read(vcpu), op_64_bit, cpl, VCPU_REGS_##ret)
> +
>   int kvm_emulate_hypercall(struct kvm_vcpu *vcpu);
>   
>   int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, u64 error_code,
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index e09daa3b157c..425a301911a6 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -9998,10 +9998,10 @@ static int complete_hypercall_exit(struct kvm_vcpu *vcpu)
>   	return kvm_skip_emulated_instruction(vcpu);
>   }
>   
> -unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
> -				      unsigned long a0, unsigned long a1,
> -				      unsigned long a2, unsigned long a3,
> -				      int op_64_bit, int cpl)
> +int ____kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
> +			      unsigned long a0, unsigned long a1,
> +			      unsigned long a2, unsigned long a3,
> +			      int op_64_bit, int cpl, int ret_reg)
>   {
>   	unsigned long ret;
>   
> @@ -10086,15 +10086,18 @@ unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
>   
>   out:
>   	++vcpu->stat.hypercalls;
> -	return ret;
> +
> +	if (!op_64_bit)
> +		ret = (u32)ret;
> +
> +	kvm_register_write_raw(vcpu, ret_reg, ret);
> +	return 1;
>   }
> -EXPORT_SYMBOL_GPL(__kvm_emulate_hypercall);
> +EXPORT_SYMBOL_GPL(____kvm_emulate_hypercall);
>   
>   int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
>   {
> -	unsigned long nr, a0, a1, a2, a3, ret;
> -	int op_64_bit;
> -	int cpl;
> +	int r;
>   
>   	if (kvm_xen_hypercall_enabled(vcpu->kvm))
>   		return kvm_xen_hypercall(vcpu);
> @@ -10102,23 +10105,12 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
>   	if (kvm_hv_hypercall_enabled(vcpu))
>   		return kvm_hv_hypercall(vcpu);
>   
> -	nr = kvm_rax_read(vcpu);
> -	a0 = kvm_rbx_read(vcpu);
> -	a1 = kvm_rcx_read(vcpu);
> -	a2 = kvm_rdx_read(vcpu);
> -	a3 = kvm_rsi_read(vcpu);
> -	op_64_bit = is_64_bit_hypercall(vcpu);
> -	cpl = kvm_x86_call(get_cpl)(vcpu);
> -
> -	ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl);
> -	if (nr == KVM_HC_MAP_GPA_RANGE && !ret)
> -		/* MAP_GPA tosses the request to the user space. */
> +	r = __kvm_emulate_hypercall(vcpu, rax, rbx, rcx, rdx, rsi,
> +				    is_64_bit_hypercall(vcpu),
> +				    kvm_x86_call(get_cpl)(vcpu), RAX);
Now, the register for return code of the hypercall can be specified.
But in  ____kvm_emulate_hypercall(), the complete_userspace_io callback
is hardcoded to complete_hypercall_exit(), which always set return code
to RAX.

We can allow the caller to pass in the cui callback, or assign different
version according to the input 'ret_reg'.  So that different callers can use
different cui callbacks.  E.g., TDX needs to set return code to R10 in cui
callback.

How about:

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index dba78f22ab27..0fba98685f42 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -2226,13 +2226,15 @@ static inline void kvm_clear_apicv_inhibit(struct kvm *kvm,
  int ____kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
                               unsigned long a0, unsigned long a1,
                               unsigned long a2, unsigned long a3,
-                             int op_64_bit, int cpl, int ret_reg);
+                             int op_64_bit, int cpl, int ret_reg,
+                             int (*cui)(struct kvm_vcpu *vcpu));

-#define __kvm_emulate_hypercall(_vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl, ret)        \
+#define __kvm_emulate_hypercall(_vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl, ret, cui)   \
____kvm_emulate_hypercall(vcpu, \
                                   kvm_##nr##_read(vcpu), kvm_##a0##_read(vcpu), \
                                   kvm_##a1##_read(vcpu), kvm_##a2##_read(vcpu), \
-                                 kvm_##a3##_read(vcpu), op_64_bit, cpl, VCPU_REGS_##ret)
+                                 kvm_##a3##_read(vcpu), op_64_bit, cpl, VCPU_REGS_##ret, \
+                                 cui)

  int kvm_emulate_hypercall(struct kvm_vcpu *vcpu);

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 6e0a518aec4a..b68690c4a4c0 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -10019,7 +10019,8 @@ static int complete_hypercall_exit(struct kvm_vcpu *vcpu)
  int ____kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
                               unsigned long a0, unsigned long a1,
                               unsigned long a2, unsigned long a3,
-                             int op_64_bit, int cpl, int ret_reg)
+                             int op_64_bit, int cpl, int ret_reg,
+                             int (*cui)(struct kvm_vcpu *vcpu))
  {
         unsigned long ret;

@@ -10093,7 +10094,7 @@ int ____kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
                         vcpu->run->hypercall.flags |= KVM_EXIT_HYPERCALL_LONG_MODE;

                 WARN_ON_ONCE(vcpu->run->hypercall.flags & KVM_EXIT_HYPERCALL_MBZ);
-               vcpu->arch.complete_userspace_io = complete_hypercall_exit;
+               vcpu->arch.complete_userspace_io = cui;
                 /* stat is incremented on completion. */
                 return 0;
         }
@@ -10125,7 +10126,7 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)

         r = __kvm_emulate_hypercall(vcpu, rax, rbx, rcx, rdx, rsi,
                                     is_64_bit_hypercall(vcpu),
-                                   kvm_x86_call(get_cpl)(vcpu), RAX);
+                                   kvm_x86_call(get_cpl)(vcpu), RAX, complete_hypercall_exit);
         if (r <= 0)
                 return 0;


> +	if (r <= 0)
>   		return 0;
>   
> -	if (!op_64_bit)
> -		ret = (u32)ret;
> -	kvm_rax_write(vcpu, ret);
> -
>   	return kvm_skip_emulated_instruction(vcpu);
>   }
>   EXPORT_SYMBOL_GPL(kvm_emulate_hypercall);
>
> base-commit: 911785b796e325dec83b32050f294e278a306211

Re: [PATCH v3 1/2] KVM: x86: Check hypercall's exit to userspace generically

[Huang, Kai]

On Mon, 2024-11-04 at 16:49 +0800, Binbin Wu wrote:
> 
> 
> On 11/2/2024 12:39 AM, Sean Christopherson wrote:
> > On Fri, Nov 01, 2024, Kai Huang wrote:
> > > On Thu, 2024-10-31 at 07:54 -0700, Sean Christopherson wrote:
> > > > On Thu, Oct 31, 2024, Kai Huang wrote:
> > > > -	ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl);
> > > > -	if (nr == KVM_HC_MAP_GPA_RANGE && !ret)
> > > > -		/* MAP_GPA tosses the request to the user space. */
> > > > -		return 0;
> > > > +	r = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl, &ret);
> > > > +	if (r <= r)
> > > > +		return r;
> > > ... should be:
> > > 
> > > 	if (r <= 0)
> > > 		return r;
> > > 
> > > ?
> > > 
> > > Another option might be we move "set hypercall return value" code inside
> > > __kvm_emulate_hypercall().  So IIUC the reason to split
> > > __kvm_emulate_hypercall() out is for TDX, and while non-TDX uses RAX to carry
> > > the hypercall return value, TDX uses R10.
> > > 
> > > We can additionally pass a "kvm_hypercall_set_ret_func" function pointer to
> > > __kvm_emulate_hypercall(), and invoke it inside.  Then we can change
> > > __kvm_emulate_hypercall() to return:
> > >      < 0 error,
> > >      ==0 return to userspace,
> > >      > 0 go back to guest.
> > Hmm, and the caller can still handle kvm_skip_emulated_instruction(), because the
> > return value is KVM's normal pattern.
> > 
> > I like it!
> > 
> > But, there's no need to pass a function pointer, KVM can write (and read) arbitrary
> > GPRs, it's just avoided in most cases so that the sanity checks and available/dirty
> > updates are elided.  For this code though, it's easy enough to keep kvm_rxx_read()
> > for getting values, and eating the overhead of a single GPR write is a perfectly
> > fine tradeoff for eliminating the return multiplexing.
> > 
> > Lightly tested.  Assuming this works for TDX and passes testing, I'll post a
> > mini-series next week.
> > 
> > --
> > From: Sean Christopherson <seanjc@google.com>
> > Date: Fri, 1 Nov 2024 09:04:00 -0700
> > Subject: [PATCH] KVM: x86: Refactor __kvm_emulate_hypercall() to accept reg
> >   names, not values
> > 
> > Rework __kvm_emulate_hypercall() to take the names of input and output
> > (guest return value) registers, as opposed to taking the input values and
> > returning the output value.  As part of the refactor, change the actual
> > return value from __kvm_emulate_hypercall() to be KVM's de facto standard
> > of '0' == exit to userspace, '1' == resume guest, and -errno == failure.
> > 
> > Using the return value for KVM's control flow eliminates the multiplexed
> > return value, where '0' for KVM_HC_MAP_GPA_RANGE (and only that hypercall)
> > means "exit to userspace".
> > 
> > Use the direct GPR accessors to read values to avoid the pointless marking
> > of the registers as available, but use kvm_register_write_raw() for the
> > guest return value so that the innermost helper doesn't need to multiplex
> > its return value.  Using the generic kvm_register_write_raw() adds very
> > minimal overhead, so as a one-off in a relatively slow path it's well
> > worth the code simplification.
> > 
> > Suggested-by: Kai Huang <kai.huang@intel.com>
> > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > ---
> >   arch/x86/include/asm/kvm_host.h | 15 +++++++++----
> >   arch/x86/kvm/x86.c              | 40 +++++++++++++--------------------
> >   2 files changed, 27 insertions(+), 28 deletions(-)
> > 
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > index 6d9f763a7bb9..9e66fde1c4e4 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -2179,10 +2179,17 @@ static inline void kvm_clear_apicv_inhibit(struct kvm *kvm,
> >   	kvm_set_or_clear_apicv_inhibit(kvm, reason, false);
> >   }
> >   
> > -unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
> > -				      unsigned long a0, unsigned long a1,
> > -				      unsigned long a2, unsigned long a3,
> > -				      int op_64_bit, int cpl);
> > +int ____kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
> > +			      unsigned long a0, unsigned long a1,
> > +			      unsigned long a2, unsigned long a3,
> > +			      int op_64_bit, int cpl, int ret_reg);
> > +
> > +#define __kvm_emulate_hypercall(_vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl, ret)	\
> > +	____kvm_emulate_hypercall(vcpu,						\
> > +				  kvm_##nr##_read(vcpu), kvm_##a0##_read(vcpu),	\
> > +				  kvm_##a1##_read(vcpu), kvm_##a2##_read(vcpu),	\
> > +				  kvm_##a3##_read(vcpu), op_64_bit, cpl, VCPU_REGS_##ret)
> > +
> >   int kvm_emulate_hypercall(struct kvm_vcpu *vcpu);
> >   
> >   int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, u64 error_code,
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index e09daa3b157c..425a301911a6 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -9998,10 +9998,10 @@ static int complete_hypercall_exit(struct kvm_vcpu *vcpu)
> >   	return kvm_skip_emulated_instruction(vcpu);
> >   }
> >   
> > -unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
> > -				      unsigned long a0, unsigned long a1,
> > -				      unsigned long a2, unsigned long a3,
> > -				      int op_64_bit, int cpl)
> > +int ____kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
> > +			      unsigned long a0, unsigned long a1,
> > +			      unsigned long a2, unsigned long a3,
> > +			      int op_64_bit, int cpl, int ret_reg)
> >   {
> >   	unsigned long ret;
> >   
> > @@ -10086,15 +10086,18 @@ unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
> >   
> >   out:
> >   	++vcpu->stat.hypercalls;
> > -	return ret;
> > +
> > +	if (!op_64_bit)
> > +		ret = (u32)ret;
> > +
> > +	kvm_register_write_raw(vcpu, ret_reg, ret);
> > +	return 1;
> >   }
> > -EXPORT_SYMBOL_GPL(__kvm_emulate_hypercall);
> > +EXPORT_SYMBOL_GPL(____kvm_emulate_hypercall);
> >   
> >   int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
> >   {
> > -	unsigned long nr, a0, a1, a2, a3, ret;
> > -	int op_64_bit;
> > -	int cpl;
> > +	int r;
> >   
> >   	if (kvm_xen_hypercall_enabled(vcpu->kvm))
> >   		return kvm_xen_hypercall(vcpu);
> > @@ -10102,23 +10105,12 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
> >   	if (kvm_hv_hypercall_enabled(vcpu))
> >   		return kvm_hv_hypercall(vcpu);
> >   
> > -	nr = kvm_rax_read(vcpu);
> > -	a0 = kvm_rbx_read(vcpu);
> > -	a1 = kvm_rcx_read(vcpu);
> > -	a2 = kvm_rdx_read(vcpu);
> > -	a3 = kvm_rsi_read(vcpu);
> > -	op_64_bit = is_64_bit_hypercall(vcpu);
> > -	cpl = kvm_x86_call(get_cpl)(vcpu);
> > -
> > -	ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl);
> > -	if (nr == KVM_HC_MAP_GPA_RANGE && !ret)
> > -		/* MAP_GPA tosses the request to the user space. */
> > +	r = __kvm_emulate_hypercall(vcpu, rax, rbx, rcx, rdx, rsi,
> > +				    is_64_bit_hypercall(vcpu),
> > +				    kvm_x86_call(get_cpl)(vcpu), RAX);
> Now, the register for return code of the hypercall can be specified.
> But in  ____kvm_emulate_hypercall(), the complete_userspace_io callback
> is hardcoded to complete_hypercall_exit(), which always set return code
> to RAX.
> 
> We can allow the caller to pass in the cui callback, or assign different
> version according to the input 'ret_reg'.  So that different callers can use
> different cui callbacks.  E.g., TDX needs to set return code to R10 in cui
> callback.
> 
> How about:
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index dba78f22ab27..0fba98685f42 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -2226,13 +2226,15 @@ static inline void kvm_clear_apicv_inhibit(struct kvm *kvm,
>   int ____kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
>                                unsigned long a0, unsigned long a1,
>                                unsigned long a2, unsigned long a3,
> -                             int op_64_bit, int cpl, int ret_reg);
> +                             int op_64_bit, int cpl, int ret_reg,
> +                             int (*cui)(struct kvm_vcpu *vcpu));
> 

Does below (incremental diff based on Sean's) work?

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 734dac079453..5131af97968d 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -10075,7 +10075,6 @@ int ____kvm_emulate_hypercall(struct kvm_vcpu *vcpu,
unsigned long nr,
                        vcpu->run->hypercall.flags |=
KVM_EXIT_HYPERCALL_LONG_MODE;
 
                WARN_ON_ONCE(vcpu->run->hypercall.flags &
KVM_EXIT_HYPERCALL_MBZ);
-               vcpu->arch.complete_userspace_io = complete_hypercall_exit;
                /* stat is incremented on completion. */
                return 0;
        }
@@ -10108,8 +10107,11 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
        r = __kvm_emulate_hypercall(vcpu, rax, rbx, rcx, rdx, rsi,
                                    is_64_bit_hypercall(vcpu),
                                    kvm_x86_call(get_cpl)(vcpu), RAX);
-       if (r <= 0)
+       if (r <= 0) {
+               if (!r)
+                       vcpu->arch.complete_userspace_io =
complete_hypercall_exit;
                return 0;
+       }
 
        return kvm_skip_emulated_instruction(vcpu);
 }

Re: [PATCH v3 1/2] KVM: x86: Check hypercall's exit to userspace generically

[Binbin Wu]

On 11/4/2024 5:55 PM, Huang, Kai wrote:
[...]
>>>    int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
>>>    {
>>> -	unsigned long nr, a0, a1, a2, a3, ret;
>>> -	int op_64_bit;
>>> -	int cpl;
>>> +	int r;
>>>    
>>>    	if (kvm_xen_hypercall_enabled(vcpu->kvm))
>>>    		return kvm_xen_hypercall(vcpu);
>>> @@ -10102,23 +10105,12 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
>>>    	if (kvm_hv_hypercall_enabled(vcpu))
>>>    		return kvm_hv_hypercall(vcpu);
>>>    
>>> -	nr = kvm_rax_read(vcpu);
>>> -	a0 = kvm_rbx_read(vcpu);
>>> -	a1 = kvm_rcx_read(vcpu);
>>> -	a2 = kvm_rdx_read(vcpu);
>>> -	a3 = kvm_rsi_read(vcpu);
>>> -	op_64_bit = is_64_bit_hypercall(vcpu);
>>> -	cpl = kvm_x86_call(get_cpl)(vcpu);
>>> -
>>> -	ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl);
>>> -	if (nr == KVM_HC_MAP_GPA_RANGE && !ret)
>>> -		/* MAP_GPA tosses the request to the user space. */
>>> +	r = __kvm_emulate_hypercall(vcpu, rax, rbx, rcx, rdx, rsi,
>>> +				    is_64_bit_hypercall(vcpu),
>>> +				    kvm_x86_call(get_cpl)(vcpu), RAX);
>> Now, the register for return code of the hypercall can be specified.
>> But in  ____kvm_emulate_hypercall(), the complete_userspace_io callback
>> is hardcoded to complete_hypercall_exit(), which always set return code
>> to RAX.
>>
>> We can allow the caller to pass in the cui callback, or assign different
>> version according to the input 'ret_reg'.  So that different callers can use
>> different cui callbacks.  E.g., TDX needs to set return code to R10 in cui
>> callback.
>>
>> How about:
>>
>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>> index dba78f22ab27..0fba98685f42 100644
>> --- a/arch/x86/include/asm/kvm_host.h
>> +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -2226,13 +2226,15 @@ static inline void kvm_clear_apicv_inhibit(struct kvm *kvm,
>>    int ____kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
>>                                 unsigned long a0, unsigned long a1,
>>                                 unsigned long a2, unsigned long a3,
>> -                             int op_64_bit, int cpl, int ret_reg);
>> +                             int op_64_bit, int cpl, int ret_reg,
>> +                             int (*cui)(struct kvm_vcpu *vcpu));
>>
> Does below (incremental diff based on Sean's) work?

Yes, it can work.

>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 734dac079453..5131af97968d 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -10075,7 +10075,6 @@ int ____kvm_emulate_hypercall(struct kvm_vcpu *vcpu,
> unsigned long nr,
>                          vcpu->run->hypercall.flags |=
> KVM_EXIT_HYPERCALL_LONG_MODE;
>   
>                  WARN_ON_ONCE(vcpu->run->hypercall.flags &
> KVM_EXIT_HYPERCALL_MBZ);
> -               vcpu->arch.complete_userspace_io = complete_hypercall_exit;
>                  /* stat is incremented on completion. */
>                  return 0;
>          }
> @@ -10108,8 +10107,11 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
>          r = __kvm_emulate_hypercall(vcpu, rax, rbx, rcx, rdx, rsi,
>                                      is_64_bit_hypercall(vcpu),
>                                      kvm_x86_call(get_cpl)(vcpu), RAX);
> -       if (r <= 0)
> +       if (r <= 0) {
> +               if (!r)
> +                       vcpu->arch.complete_userspace_io =
> complete_hypercall_exit;
>                  return 0;
> +       }
>   
>          return kvm_skip_emulated_instruction(vcpu);
>   }
>

Re: [PATCH v3 1/2] KVM: x86: Check hypercall's exit to userspace generically

[Sean Christopherson]

On Mon, Nov 04, 2024, Kai Huang wrote:
> On Mon, 2024-11-04 at 16:49 +0800, Binbin Wu wrote:
> > On 11/2/2024 12:39 AM, Sean Christopherson wrote:
> > > On Fri, Nov 01, 2024, Kai Huang wrote:
> > > > On Thu, 2024-10-31 at 07:54 -0700, Sean Christopherson wrote:

...

> > > Lightly tested.  Assuming this works for TDX and passes testing, I'll post a
> > > mini-series next week.
> > > 
> > > --
> > > From: Sean Christopherson <seanjc@google.com>
> > > Date: Fri, 1 Nov 2024 09:04:00 -0700
> > > Subject: [PATCH] KVM: x86: Refactor __kvm_emulate_hypercall() to accept reg
> > >   names, not values
> > > 
> > > Rework __kvm_emulate_hypercall() to take the names of input and output
> > > (guest return value) registers, as opposed to taking the input values and
> > > returning the output value.  As part of the refactor, change the actual
> > > return value from __kvm_emulate_hypercall() to be KVM's de facto standard
> > > of '0' == exit to userspace, '1' == resume guest, and -errno == failure.
> > > 
> > > Using the return value for KVM's control flow eliminates the multiplexed
> > > return value, where '0' for KVM_HC_MAP_GPA_RANGE (and only that hypercall)
> > > means "exit to userspace".
> > > 
> > > Use the direct GPR accessors to read values to avoid the pointless marking
> > > of the registers as available, but use kvm_register_write_raw() for the
> > > guest return value so that the innermost helper doesn't need to multiplex
> > > its return value.  Using the generic kvm_register_write_raw() adds very
> > > minimal overhead, so as a one-off in a relatively slow path it's well
> > > worth the code simplification.
> > > 
> > > Suggested-by: Kai Huang <kai.huang@intel.com>
> > > Signed-off-by: Sean Christopherson <seanjc@google.com>
> > > ---

...

> > > -	ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl);
> > > -	if (nr == KVM_HC_MAP_GPA_RANGE && !ret)
> > > -		/* MAP_GPA tosses the request to the user space. */
> > > +	r = __kvm_emulate_hypercall(vcpu, rax, rbx, rcx, rdx, rsi,
> > > +				    is_64_bit_hypercall(vcpu),
> > > +				    kvm_x86_call(get_cpl)(vcpu), RAX);
> > Now, the register for return code of the hypercall can be specified.
> > But in  ____kvm_emulate_hypercall(), the complete_userspace_io callback
> > is hardcoded to complete_hypercall_exit(), which always set return code
> > to RAX.
> > 
> > We can allow the caller to pass in the cui callback, or assign different
> > version according to the input 'ret_reg'.  So that different callers can use
> > different cui callbacks.  E.g., TDX needs to set return code to R10 in cui
> > callback.
> > 
> > How about:
> > 
> > diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> > index dba78f22ab27..0fba98685f42 100644
> > --- a/arch/x86/include/asm/kvm_host.h
> > +++ b/arch/x86/include/asm/kvm_host.h
> > @@ -2226,13 +2226,15 @@ static inline void kvm_clear_apicv_inhibit(struct kvm *kvm,
> >   int ____kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
> >                                unsigned long a0, unsigned long a1,
> >                                unsigned long a2, unsigned long a3,
> > -                             int op_64_bit, int cpl, int ret_reg);
> > +                             int op_64_bit, int cpl, int ret_reg,
> > +                             int (*cui)(struct kvm_vcpu *vcpu));
> > 
> 
> Does below (incremental diff based on Sean's) work?
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 734dac079453..5131af97968d 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -10075,7 +10075,6 @@ int ____kvm_emulate_hypercall(struct kvm_vcpu *vcpu,
> unsigned long nr,
>                         vcpu->run->hypercall.flags |=
> KVM_EXIT_HYPERCALL_LONG_MODE;
>  
>                 WARN_ON_ONCE(vcpu->run->hypercall.flags &
> KVM_EXIT_HYPERCALL_MBZ);
> -               vcpu->arch.complete_userspace_io = complete_hypercall_exit;
>                 /* stat is incremented on completion. */
>                 return 0;
>         }
> @@ -10108,8 +10107,11 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
>         r = __kvm_emulate_hypercall(vcpu, rax, rbx, rcx, rdx, rsi,
>                                     is_64_bit_hypercall(vcpu),
>                                     kvm_x86_call(get_cpl)(vcpu), RAX);
> -       if (r <= 0)
> +       if (r <= 0) {
> +               if (!r)
> +                       vcpu->arch.complete_userspace_io =
> complete_hypercall_exit;
>                 return 0;
> +       }

I think I prefer Binbin's version, as it forces the caller to provide cui(), i.e.
makes it harder KVM to fail to handle the backend of the hypercall.

Side topic, unless I'm missing something, all of the instruction VM-Exits that are
piped through #VMGEXIT to svm_invoke_exit_handler() will make a bogus call to
kvm_skip_emulated_instruction().  It "works", but only because nothing ever looks
at the modified data.

At first, I thought it was just VMMCALL, and so was wondering if maybe we could
fix that wart too.  But it's *every* instruction, so it's probably out of scope.

The one thing I don't love about providing a separate cui() is that it means
duplicating the guts of the completion helper.  Ha!  But we can avoid that by
adding another macro (untested).

More macros/helpers is a bit ugly too, but I like the symmetry, and it will
definitely be easier to maintain.  E.g. if the completion phase needs to pivot
on the exact hypercall, then we can update common code and don't need to remember
to go update TDX too.

If no one objects and/or has a better idea, I'll splice together Binbin's patch
with this blob, and post a series tomorrow.

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index 8e8ca6dab2b2..0b0fa9174000 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -2179,6 +2179,16 @@ static inline void kvm_clear_apicv_inhibit(struct kvm *kvm,
        kvm_set_or_clear_apicv_inhibit(kvm, reason, false);
 }
 
+#define kvm_complete_hypercall_exit(vcpu, ret_reg)                             \
+do {                                                                           \
+       u64 ret = (vcpu)->run->hypercall.ret;                                   \
+                                                                               \
+       if (!is_64_bit_mode(vcpu))                                              \
+               ret = (u32)ret;                                                 \
+       kvm_##ret_reg##_write(vcpu, ret);                                       \
+       ++(vcpu)->stat.hypercalls;                                              \
+} while (0)
+
 int ____kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
                              unsigned long a0, unsigned long a1,
                              unsigned long a2, unsigned long a3,
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 425a301911a6..aec79e132d3b 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -9989,12 +9989,8 @@ static void kvm_sched_yield(struct kvm_vcpu *vcpu, unsigned long dest_id)
 
 static int complete_hypercall_exit(struct kvm_vcpu *vcpu)
 {
-       u64 ret = vcpu->run->hypercall.ret;
+       kvm_complete_hypercall_exit(vcpu, rax);
 
-       if (!is_64_bit_mode(vcpu))
-               ret = (u32)ret;
-       kvm_rax_write(vcpu, ret);
-       ++vcpu->stat.hypercalls;
        return kvm_skip_emulated_instruction(vcpu);
 }
 

Re: [PATCH v3 1/2] KVM: x86: Check hypercall's exit to userspace generically

[Huang, Kai]

> > 
> 
> I think I prefer Binbin's version, as it forces the caller to provide cui(), i.e.
> makes it harder KVM to fail to handle the backend of the hypercall.

Fine to me.

[...]

> 
> The one thing I don't love about providing a separate cui() is that it means
> duplicating the guts of the completion helper.  Ha!  But we can avoid that by
> adding another macro (untested).
> 
> More macros/helpers is a bit ugly too, but I like the symmetry, and it will
> definitely be easier to maintain.  E.g. if the completion phase needs to pivot
> on the exact hypercall, then we can update common code and don't need to remember
> to go update TDX too.
> 
> If no one objects and/or has a better idea, I'll splice together Binbin's patch
> with this blob, and post a series tomorrow.
> 
> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
> index 8e8ca6dab2b2..0b0fa9174000 100644
> --- a/arch/x86/include/asm/kvm_host.h
> +++ b/arch/x86/include/asm/kvm_host.h
> @@ -2179,6 +2179,16 @@ static inline void kvm_clear_apicv_inhibit(struct kvm *kvm,
>         kvm_set_or_clear_apicv_inhibit(kvm, reason, false);
>  }
>  
> +#define kvm_complete_hypercall_exit(vcpu, ret_reg)                             \
> +do {                                                                           \
> +       u64 ret = (vcpu)->run->hypercall.ret;                                   \
> +                                                                               \
> +       if (!is_64_bit_mode(vcpu))                                              \
> +               ret = (u32)ret;                                                 \
> +       kvm_##ret_reg##_write(vcpu, ret);                                       \
> +       ++(vcpu)->stat.hypercalls;                                              \
> +} while (0)
> +
>  int ____kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
>                               unsigned long a0, unsigned long a1,
>                               unsigned long a2, unsigned long a3,
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 425a301911a6..aec79e132d3b 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -9989,12 +9989,8 @@ static void kvm_sched_yield(struct kvm_vcpu *vcpu, unsigned long dest_id)
>  
>  static int complete_hypercall_exit(struct kvm_vcpu *vcpu)
>  {
> -       u64 ret = vcpu->run->hypercall.ret;
> +       kvm_complete_hypercall_exit(vcpu, rax);
>  
> -       if (!is_64_bit_mode(vcpu))
> -               ret = (u32)ret;
> -       kvm_rax_write(vcpu, ret);
> -       ++vcpu->stat.hypercalls;
>         return kvm_skip_emulated_instruction(vcpu);
>  }
>  

I think there's one issue here:

I assume macro kvm_complete_hypercall_exit(vcpu, ret_reg) will also be used by
TDX.  The issue is it calls !is_64_bit_mode(vcpu), which has below WARN():

        WARN_ON_ONCE(vcpu->arch.guest_state_protected);     

So IIUC TDX will hit this.

Btw, we have below (kinda) duplicated code in ____kvm_emulate_hypercall() too:

	++vcpu->stat.hypercalls;                                                      
                                                                                                                                                               
        if (!op_64_bit)                                                        
                ret = (u32)ret;                                                
                                                                                                                                                               
        kvm_register_write_raw(vcpu, ret_reg, ret);      

If we add a helper to do above, e.g.,

static void kvm_complete_hypercall_exit(struct kvm_vcpu *vcpu, int ret_reg, 
				        unsigned long ret, bool op_64_bit)
{
	if (!op_64_bit)
		ret = (u32)ret;
	kvm_register_write_raw(vcpu, ret_reg, ret);
	++vcpu->stat.hypercalls;
}

Then we can have

static int complete_hypercall_exit(struct kvm_vcpu *vcpu)
{
	kvm_complete_hypercall_exit(vcpu, VCPU_REGS_RAX,
		vcpu->run->hypercall.ret, is_64_bit_mode(vcpu));

	return kvm_skip_emulated_instruction(vcpu);
}

TDX version can use:

	kvm_complete_hypercall_exit(vcpu, VCPU_REGS_R10,
		vcpu->run->hypercall.ret, true);

And ____kvm_emulate_hypercall() can be:

static int ____kvm_emulate_hypercall(vcpu, ...)
{
	...
out:
	kvm_complete_hypercall_exit(vcpu, ret_reg, ret, op_64_bit);
	return 1;
}

Re: [PATCH v3 1/2] KVM: x86: Check hypercall's exit to userspace generically

[Binbin Wu]

On 11/5/2024 5:20 PM, Huang, Kai wrote:
>> I think I prefer Binbin's version, as it forces the caller to provide cui(), i.e.
>> makes it harder KVM to fail to handle the backend of the hypercall.
> Fine to me.
>
> [...]
>
>> The one thing I don't love about providing a separate cui() is that it means
>> duplicating the guts of the completion helper.  Ha!  But we can avoid that by
>> adding another macro (untested).
>>
>> More macros/helpers is a bit ugly too, but I like the symmetry, and it will
>> definitely be easier to maintain.  E.g. if the completion phase needs to pivot
>> on the exact hypercall, then we can update common code and don't need to remember
>> to go update TDX too.
>>
>> If no one objects and/or has a better idea, I'll splice together Binbin's patch
>> with this blob, and post a series tomorrow.
>>
>> diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
>> index 8e8ca6dab2b2..0b0fa9174000 100644
>> --- a/arch/x86/include/asm/kvm_host.h
>> +++ b/arch/x86/include/asm/kvm_host.h
>> @@ -2179,6 +2179,16 @@ static inline void kvm_clear_apicv_inhibit(struct kvm *kvm,
>>          kvm_set_or_clear_apicv_inhibit(kvm, reason, false);
>>   }
>>   
>> +#define kvm_complete_hypercall_exit(vcpu, ret_reg)                             \
>> +do {                                                                           \
>> +       u64 ret = (vcpu)->run->hypercall.ret;                                   \
>> +                                                                               \
>> +       if (!is_64_bit_mode(vcpu))                                              \
>> +               ret = (u32)ret;                                                 \
>> +       kvm_##ret_reg##_write(vcpu, ret);                                       \
>> +       ++(vcpu)->stat.hypercalls;                                              \
>> +} while (0)
>> +
>>   int ____kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
>>                                unsigned long a0, unsigned long a1,
>>                                unsigned long a2, unsigned long a3,
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 425a301911a6..aec79e132d3b 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -9989,12 +9989,8 @@ static void kvm_sched_yield(struct kvm_vcpu *vcpu, unsigned long dest_id)
>>   
>>   static int complete_hypercall_exit(struct kvm_vcpu *vcpu)
>>   {
>> -       u64 ret = vcpu->run->hypercall.ret;
>> +       kvm_complete_hypercall_exit(vcpu, rax);
>>   
>> -       if (!is_64_bit_mode(vcpu))
>> -               ret = (u32)ret;
>> -       kvm_rax_write(vcpu, ret);
>> -       ++vcpu->stat.hypercalls;
>>          return kvm_skip_emulated_instruction(vcpu);
>>   }
>>   
> I think there's one issue here:
>
> I assume macro kvm_complete_hypercall_exit(vcpu, ret_reg) will also be used by
> TDX.  The issue is it calls !is_64_bit_mode(vcpu), which has below WARN():
>
>          WARN_ON_ONCE(vcpu->arch.guest_state_protected);
>
> So IIUC TDX will hit this.
>
> Btw, we have below (kinda) duplicated code in ____kvm_emulate_hypercall() too:
>
> 	++vcpu->stat.hypercalls;
>                                                                                                                                                                 
>          if (!op_64_bit)
>                  ret = (u32)ret;
>                                                                                                                                                                 
>          kvm_register_write_raw(vcpu, ret_reg, ret);
>
> If we add a helper to do above, e.g.,
>
> static void kvm_complete_hypercall_exit(struct kvm_vcpu *vcpu, int ret_reg,
> 				        unsigned long ret, bool op_64_bit)
> {
> 	if (!op_64_bit)
> 		ret = (u32)ret;
> 	kvm_register_write_raw(vcpu, ret_reg, ret);
> 	++vcpu->stat.hypercalls;
> }
If this is going to be the final version, it would be better to make it
public, and export the symbol, so that TDX code can reuse it.


>
> Then we can have
>
> static int complete_hypercall_exit(struct kvm_vcpu *vcpu)
> {
> 	kvm_complete_hypercall_exit(vcpu, VCPU_REGS_RAX,
> 		vcpu->run->hypercall.ret, is_64_bit_mode(vcpu));
>
> 	return kvm_skip_emulated_instruction(vcpu);
> }
>
> TDX version can use:
>
> 	kvm_complete_hypercall_exit(vcpu, VCPU_REGS_R10,
> 		vcpu->run->hypercall.ret, true);
>
> And ____kvm_emulate_hypercall() can be:
>
> static int ____kvm_emulate_hypercall(vcpu, ...)
> {
> 	...
> out:
> 	kvm_complete_hypercall_exit(vcpu, ret_reg, ret, op_64_bit);
> 	return 1;
> }
>

Re: [PATCH v3 1/2] KVM: x86: Check hypercall's exit to userspace generically

[Huang, Kai]

On Wed, 2024-11-06 at 16:32 +0800, Binbin Wu wrote:
> > static void kvm_complete_hypercall_exit(struct kvm_vcpu *vcpu, int ret_reg,
> >  				        unsigned long ret, bool op_64_bit)
> > {
> >  	if (!op_64_bit)
> >  		ret = (u32)ret;
> >  	kvm_register_write_raw(vcpu, ret_reg, ret);
> >  	++vcpu->stat.hypercalls;
> > }
> If this is going to be the final version, it would be better to make it
> public, and export the symbol, so that TDX code can reuse it.

Does making it 'static inline' and moving to kvm_host.h work?

kvm_register_write_raw(), and kvm_register_mark_dirty() which is called by
kvm_register_write_raw()), are both 'static inline'.  Seems we can get rid of
export by making it 'static inline'.

Re: [PATCH v3 1/2] KVM: x86: Check hypercall's exit to userspace generically

[Binbin Wu]

On 11/6/2024 4:54 PM, Huang, Kai wrote:
> On Wed, 2024-11-06 at 16:32 +0800, Binbin Wu wrote:
>>> static void kvm_complete_hypercall_exit(struct kvm_vcpu *vcpu, int ret_reg,
>>>   				        unsigned long ret, bool op_64_bit)
>>> {
>>>   	if (!op_64_bit)
>>>   		ret = (u32)ret;
>>>   	kvm_register_write_raw(vcpu, ret_reg, ret);
>>>   	++vcpu->stat.hypercalls;
>>> }
>> If this is going to be the final version, it would be better to make it
>> public, and export the symbol, so that TDX code can reuse it.
> Does making it 'static inline' and moving to kvm_host.h work?
It doesn't have a complete definition of struct kvm_vcpu in
arch/x86/include/asm/kvm_host.h, and the code is dereferencing
struct kvm_vcpu.
Also, the definition of kvm_register_write_raw() is in
arch/x86/kvm/kvm_cache_regs.h, which make it difficult to be called
there.

>
> kvm_register_write_raw(), and kvm_register_mark_dirty() which is called by
> kvm_register_write_raw()), are both 'static inline'.  Seems we can get rid of
> export by making it 'static inline'.

Re: [PATCH v3 1/2] KVM: x86: Check hypercall's exit to userspace generically

[Sean Christopherson]

On Wed, Nov 06, 2024, Binbin Wu wrote:
> On 11/6/2024 4:54 PM, Huang, Kai wrote:
> > On Wed, 2024-11-06 at 16:32 +0800, Binbin Wu wrote:
> > > > static void kvm_complete_hypercall_exit(struct kvm_vcpu *vcpu, int ret_reg,
> > > >   				        unsigned long ret, bool op_64_bit)
> > > > {
> > > >   	if (!op_64_bit)
> > > >   		ret = (u32)ret;
> > > >   	kvm_register_write_raw(vcpu, ret_reg, ret);
> > > >   	++vcpu->stat.hypercalls;
> > > > }
> > > If this is going to be the final version, it would be better to make it
> > > public, and export the symbol, so that TDX code can reuse it.
> > Does making it 'static inline' and moving to kvm_host.h work?
> It doesn't have a complete definition of struct kvm_vcpu in
> arch/x86/include/asm/kvm_host.h, and the code is dereferencing
> struct kvm_vcpu.
> Also, the definition of kvm_register_write_raw() is in
> arch/x86/kvm/kvm_cache_regs.h, which make it difficult to be called
> there.

A way around that would be to move the declarations from asm/kvm_host.h to x86.h,
and then kvm_complete_hypercall_exit() can be inlined (or not), without having to
deal with the kvm_host.h ordering issues.

IMO, KVM x86 would ideally put as much as possible in x86.h.  The vast majority
of KVM x86's exports are intended only for the vendor modules.  Declaring those
exports in kvm_host.h is effectively bleeding KVM internals to the broader kernel.

I'll go that route for the series, assuming it works as I intend :-)

Re: [PATCH v3 1/2] KVM: x86: Check hypercall's exit to userspace generically

[Binbin Wu]

On 11/6/2024 11:30 PM, Sean Christopherson wrote:
> On Wed, Nov 06, 2024, Binbin Wu wrote:
>> On 11/6/2024 4:54 PM, Huang, Kai wrote:
>>> On Wed, 2024-11-06 at 16:32 +0800, Binbin Wu wrote:
>>>>> static void kvm_complete_hypercall_exit(struct kvm_vcpu *vcpu, int ret_reg,
>>>>>    				        unsigned long ret, bool op_64_bit)
>>>>> {
>>>>>    	if (!op_64_bit)
>>>>>    		ret = (u32)ret;
>>>>>    	kvm_register_write_raw(vcpu, ret_reg, ret);
>>>>>    	++vcpu->stat.hypercalls;
>>>>> }
>>>> If this is going to be the final version, it would be better to make it
>>>> public, and export the symbol, so that TDX code can reuse it.
>>> Does making it 'static inline' and moving to kvm_host.h work?
>> It doesn't have a complete definition of struct kvm_vcpu in
>> arch/x86/include/asm/kvm_host.h, and the code is dereferencing
>> struct kvm_vcpu.
>> Also, the definition of kvm_register_write_raw() is in
>> arch/x86/kvm/kvm_cache_regs.h, which make it difficult to be called
>> there.
> A way around that would be to move the declarations from asm/kvm_host.h to x86.h,
> and then kvm_complete_hypercall_exit() can be inlined (or not), without having to
> deal with the kvm_host.h ordering issues.
>
> IMO, KVM x86 would ideally put as much as possible in x86.h.  The vast majority
> of KVM x86's exports are intended only for the vendor modules.  Declaring those
> exports in kvm_host.h is effectively bleeding KVM internals to the broader kernel.
>
> I'll go that route for the series, assuming it works as I intend :-)
>
Based on the previous discussions, is the below code expected?
If so, I am gonna use __kvm_emulate_hypercall() and kvm_complete_hypercall_exit()
in TDX code for handling KVM hypercalls.

diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index e51a95aba824..356620f74bb0 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -2205,10 +2205,6 @@ static inline void kvm_clear_apicv_inhibit(struct kvm *kvm,
         kvm_set_or_clear_apicv_inhibit(kvm, reason, false);
  }

-unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
-                                     unsigned long a0, unsigned long a1,
-                                     unsigned long a2, unsigned long a3,
-                                     int op_64_bit, int cpl);
  int kvm_emulate_hypercall(struct kvm_vcpu *vcpu);

  int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, u64 error_code,
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 2b5b0ae3dd7e..05191c4af156 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -10008,19 +10008,17 @@ static void kvm_sched_yield(struct kvm_vcpu *vcpu, unsigned long dest_id)

  static int complete_hypercall_exit(struct kvm_vcpu *vcpu)
  {
-       u64 ret = vcpu->run->hypercall.ret;
+       kvm_complete_hypercall_exit(vcpu, VCPU_REGS_RAX,
+               vcpu->run->hypercall.ret, is_64_bit_mode(vcpu));

-       if (!is_64_bit_mode(vcpu))
-               ret = (u32)ret;
-       kvm_rax_write(vcpu, ret);
-       ++vcpu->stat.hypercalls;
         return kvm_skip_emulated_instruction(vcpu);
  }

-unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
-                                     unsigned long a0, unsigned long a1,
-                                     unsigned long a2, unsigned long a3,
-                                     int op_64_bit, int cpl)
+int ____kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
+                             unsigned long a0, unsigned long a1,
+                             unsigned long a2, unsigned long a3,
+                             int op_64_bit, int cpl, int ret_reg,
+                             int (*cui)(struct kvm_vcpu *vcpu))
  {
         unsigned long ret;

@@ -10104,16 +10102,15 @@ unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
         }

  out:
-       ++vcpu->stat.hypercalls;
-       return ret;
+       kvm_complete_hypercall_exit(vcpu, ret_reg, ret, op_64_bit);
+
+       return 1;
  }
-EXPORT_SYMBOL_GPL(__kvm_emulate_hypercall);
+EXPORT_SYMBOL_GPL(____kvm_emulate_hypercall);

  int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
  {
-       unsigned long nr, a0, a1, a2, a3, ret;
-       int op_64_bit;
-       int cpl;
+       int r;

         if (kvm_xen_hypercall_enabled(vcpu->kvm))
                 return kvm_xen_hypercall(vcpu);
@@ -10121,23 +10118,13 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
         if (kvm_hv_hypercall_enabled(vcpu))
                 return kvm_hv_hypercall(vcpu);
                 return kvm_hv_hypercall(vcpu);

-       nr = kvm_rax_read(vcpu);
-       a0 = kvm_rbx_read(vcpu);
-       a1 = kvm_rcx_read(vcpu);
-       a2 = kvm_rdx_read(vcpu);
-       a3 = kvm_rsi_read(vcpu);
-       op_64_bit = is_64_bit_hypercall(vcpu);
-       cpl = kvm_x86_call(get_cpl)(vcpu);
-
-       ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl);
-       if (nr == KVM_HC_MAP_GPA_RANGE && !ret)
-               /* MAP_GPA tosses the request to the user space. */
+       r = __kvm_emulate_hypercall(vcpu, rax, rbx, rcx, rdx, rsi,
+                                   is_64_bit_hypercall(vcpu),
+                                   kvm_x86_call(get_cpl)(vcpu), RAX,
+                                   complete_hypercall_exit);
+       if (r <= 0)
                 return 0;

-       if (!op_64_bit)
-               ret = (u32)ret;
-       kvm_rax_write(vcpu, ret);
-
         return kvm_skip_emulated_instruction(vcpu);
  }
  EXPORT_SYMBOL_GPL(kvm_emulate_hypercall);
diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index a84c48ef5278..5667d029e7f0 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -568,4 +568,25 @@ int kvm_sev_es_string_io(struct kvm_vcpu *vcpu, unsigned int size,
                          unsigned int port, void *data,  unsigned int count,
                          int in);

+int ____kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
+                             unsigned long a0, unsigned long a1,
+                             unsigned long a2, unsigned long a3,
+                             int op_64_bit, int cpl, int ret_reg,
+                             int (*cui)(struct kvm_vcpu *vcpu));
+
+#define __kvm_emulate_hypercall(_vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl, ret, cui)           \
+ ____kvm_emulate_hypercall(vcpu, \
+                                 kvm_##nr##_read(vcpu), kvm_##a0##_read(vcpu),                 \
+                                 kvm_##a1##_read(vcpu), kvm_##a2##_read(vcpu),                 \
+                                 kvm_##a3##_read(vcpu), op_64_bit, cpl, VCPU_REGS_##ret,       \
+                                 cui)
+
+static inline void kvm_complete_hypercall_exit(struct kvm_vcpu *vcpu, int ret_reg,
+                                              unsigned long ret, bool op_64_bit)
+{
+       if (!op_64_bit)
+               ret = (u32)ret;
+       kvm_register_write_raw(vcpu, ret_reg, ret);
+       ++vcpu->stat.hypercalls;
+}
+
  #endif

Re: [PATCH v3 1/2] KVM: x86: Check hypercall's exit to userspace generically

[Huang, Kai]

> > 
> Based on the previous discussions, is the below code expected?
> If so, I am gonna use __kvm_emulate_hypercall() and kvm_complete_hypercall_exit()
> in TDX code for handling KVM hypercalls.
> 

LGTM.

Re: [PATCH v3 1/2] KVM: x86: Check hypercall's exit to userspace generically

[Binbin Wu]

On 10/31/2024 4:49 AM, Sean Christopherson wrote:
> On Mon, Aug 26, 2024, Binbin Wu wrote:
>> Check whether a KVM hypercall needs to exit to userspace or not based on
>> hypercall_exit_enabled field of struct kvm_arch.
>>
>> Userspace can request a hypercall to exit to userspace for handling by
>> enable KVM_CAP_EXIT_HYPERCALL and the enabled hypercall will be set in
>> hypercall_exit_enabled.  Make the check code generic based on it.
>>
>> Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com>
>> Reviewed-by: Kai Huang <kai.huang@intel.com>
>> ---
>>   arch/x86/kvm/x86.c | 5 +++--
>>   arch/x86/kvm/x86.h | 4 ++++
>>   2 files changed, 7 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index 966fb301d44b..e521f14ad2b2 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -10220,8 +10220,9 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
>>   	cpl = kvm_x86_call(get_cpl)(vcpu);
>>   
>>   	ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl);
>> -	if (nr == KVM_HC_MAP_GPA_RANGE && !ret)
>> -		/* MAP_GPA tosses the request to the user space. */
>> +	/* Check !ret first to make sure nr is a valid KVM hypercall. */
>> +	if (!ret && user_exit_on_hypercall(vcpu->kvm, nr))
> I don't love that the caller has to re-check for user_exit_on_hypercall().
Agree, it is not ideal.

But if __kvm_emulate_hypercall() returns 0 to indicate user exit and 1 to
indicate success, then the callers have to convert the return code to set
return value for guest.  E.g., TDX code also needs to do the conversion.

> I also don't love that there's a surprising number of checks lurking in
> __kvm_emulate_hypercall(), e.g. that CPL==0, especially since the above comment
> about "a valid KVM hypercall" can be intrepreted as meaning KVM is *only* checking
> if the hypercall number is valid.
>
> E.g. my initial reaction was that we could add a separate path for userspace
> hypercalls, but that would be subtly wrong.  And my second reaction was to hoist
> the common checks out of __kvm_emulate_hypercall(), but then I remembered that
> the only reason __kvm_emulate_hypercall() is separate is to allow it to be called
> by TDX with different source/destionation registers.
>
> So, I'm strongly leaning towards dropping the above change, squashing the addition
> of the helper with patch 2, and then landing this on top.
>
> Thoughts?
I have no strong preference and OK with the proposal below.

Just some cases, which don't get the return value right as pointed by Kai
in another thread.
https://lore.kernel.org/kvm/3f158732a66829faaeb527a94b8df78d6173befa.camel@intel.com/


>
> --
> Subject: [PATCH] KVM: x86: Use '0' in __kvm_emulate_hypercall()  to signal
>   "exit to userspace"
>
> Rework __kvm_emulate_hypercall() to use '0' to indicate an exit to
> userspace instead of relying on the caller to manually check for success
> *and* if user_exit_on_hypercall() is true.  Use '1' for "success" to
> (mostly) align with KVM's de factor return codes, where '0' == exit to
> userspace, '1' == resume guest, and -errno == failure.  Unfortunately,
> some of the PV error codes returned to the guest are postive values, so
> the pattern doesn't exactly match KVM's "standard", but it should be close
> enough to be intuitive for KVM readers.
>
> Signed-off-by: Sean Christopherson <seanjc@google.com>
> ---
>   arch/x86/kvm/x86.c | 21 +++++++++++++++------
>   1 file changed, 15 insertions(+), 6 deletions(-)
>
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index e09daa3b157c..5fdeb58221e2 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -10024,7 +10024,7 @@ unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
>   
>   	switch (nr) {
>   	case KVM_HC_VAPIC_POLL_IRQ:
> -		ret = 0;
> +		ret = 1;
>   		break;
>   	case KVM_HC_KICK_CPU:
>   		if (!guest_pv_has(vcpu, KVM_FEATURE_PV_UNHALT))
> @@ -10032,7 +10032,7 @@ unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
>   
>   		kvm_pv_kick_cpu_op(vcpu->kvm, a1);
>   		kvm_sched_yield(vcpu, a1);
> -		ret = 0;
> +		ret = 1;
>   		break;
>   #ifdef CONFIG_X86_64
>   	case KVM_HC_CLOCK_PAIRING:
> @@ -10050,7 +10050,7 @@ unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
>   			break;
>   
>   		kvm_sched_yield(vcpu, a0);
> -		ret = 0;
> +		ret = 1;
>   		break;
>   	case KVM_HC_MAP_GPA_RANGE: {
>   		u64 gpa = a0, npages = a1, attrs = a2;
> @@ -10111,12 +10111,21 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
>   	cpl = kvm_x86_call(get_cpl)(vcpu);
>   
>   	ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl);
> -	if (nr == KVM_HC_MAP_GPA_RANGE && !ret)
> -		/* MAP_GPA tosses the request to the user space. */
> +	if (!ret)
>   		return 0;
>   
> -	if (!op_64_bit)
> +	/*
> +	 * KVM's ABI with the guest is that '0' is success, and any other value
> +	 * is an error code.  Internally, '0' == exit to userspace (see above)
> +	 * and '1' == success, as KVM's de facto standard return codes are that
> +	 * plus -errno == failure.  Explicitly check for '1' as some PV error
> +	 * codes are positive values.
> +	 */
I didn't understand the last sentence:
"Explicitly check for '1' as some PV error codes are positive values."

The functions called in __kvm_emulate_hypercall() for PV features return
-KVM_EXXX for error code.
Did you mean the functions like kvm_pv_enable_async_pf(), which return
1 for error, would be called in __kvm_emulate_hypercall() in the future?
If this is the concern, then we cannot simply convert 1 to 0 then.

> +	if (ret == 1)
> +		ret = 0;
> +	else if (!op_64_bit)
>   		ret = (u32)ret;
> +
>   	kvm_rax_write(vcpu, ret);
>   
>   	return kvm_skip_emulated_instruction(vcpu);
>
> base-commit: 675248928970d33f7fc8ca9851a170c98f4f1c4f

Re: [PATCH v3 1/2] KVM: x86: Check hypercall's exit to userspace generically

[Sean Christopherson]

On Thu, Oct 31, 2024, Binbin Wu wrote:
> On 10/31/2024 4:49 AM, Sean Christopherson wrote:
> > On Mon, Aug 26, 2024, Binbin Wu wrote:
> > > Check whether a KVM hypercall needs to exit to userspace or not based on
> > > hypercall_exit_enabled field of struct kvm_arch.
> > > 
> > > Userspace can request a hypercall to exit to userspace for handling by
> > > enable KVM_CAP_EXIT_HYPERCALL and the enabled hypercall will be set in
> > > hypercall_exit_enabled.  Make the check code generic based on it.
> > > 
> > > Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com>
> > > Reviewed-by: Kai Huang <kai.huang@intel.com>
> > > ---
> > >   arch/x86/kvm/x86.c | 5 +++--
> > >   arch/x86/kvm/x86.h | 4 ++++
> > >   2 files changed, 7 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > > index 966fb301d44b..e521f14ad2b2 100644
> > > --- a/arch/x86/kvm/x86.c
> > > +++ b/arch/x86/kvm/x86.c
> > > @@ -10220,8 +10220,9 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
> > >   	cpl = kvm_x86_call(get_cpl)(vcpu);
> > >   	ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl);
> > > -	if (nr == KVM_HC_MAP_GPA_RANGE && !ret)
> > > -		/* MAP_GPA tosses the request to the user space. */
> > > +	/* Check !ret first to make sure nr is a valid KVM hypercall. */
> > > +	if (!ret && user_exit_on_hypercall(vcpu->kvm, nr))
> > I don't love that the caller has to re-check for user_exit_on_hypercall().
> Agree, it is not ideal.
> 
> But if __kvm_emulate_hypercall() returns 0 to indicate user exit and 1 to
> indicate success, then the callers have to convert the return code to set
> return value for guest.  E.g., TDX code also needs to do the conversion.

Yeah, it's ugly.

> > @@ -10111,12 +10111,21 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
> >   	cpl = kvm_x86_call(get_cpl)(vcpu);
> >   	ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl);
> > -	if (nr == KVM_HC_MAP_GPA_RANGE && !ret)
> > -		/* MAP_GPA tosses the request to the user space. */
> > +	if (!ret)
> >   		return 0;
> > -	if (!op_64_bit)
> > +	/*
> > +	 * KVM's ABI with the guest is that '0' is success, and any other value
> > +	 * is an error code.  Internally, '0' == exit to userspace (see above)
> > +	 * and '1' == success, as KVM's de facto standard return codes are that
> > +	 * plus -errno == failure.  Explicitly check for '1' as some PV error
> > +	 * codes are positive values.
> > +	 */
> I didn't understand the last sentence:
> "Explicitly check for '1' as some PV error codes are positive values."
> 
> The functions called in __kvm_emulate_hypercall() for PV features return
> -KVM_EXXX for error code.
> Did you mean the functions like kvm_pv_enable_async_pf(), which return
> 1 for error, would be called in __kvm_emulate_hypercall() in the future?
> If this is the concern, then we cannot simply convert 1 to 0 then.

No, it was a brain fart on my end.  I was thinking of these #defines, and managed
to forget that KVM always returns -KVM_Exxx.  /facepalm

  #define KVM_ENOSYS		1000
  #define KVM_EFAULT		EFAULT
  #define KVM_EINVAL		EINVAL
  #define KVM_E2BIG		E2BIG
  #define KVM_EPERM		EPERM
  #define KVM_EOPNOTSUPP		95

> 
> > +	if (ret == 1)
> > +		ret = 0;
> > +	else if (!op_64_bit)
> >   		ret = (u32)ret;
> > +
> >   	kvm_rax_write(vcpu, ret);
> >   	return kvm_skip_emulated_instruction(vcpu);
> > 
> > base-commit: 675248928970d33f7fc8ca9851a170c98f4f1c4f
> 

[PATCH v3 2/2] KVM: x86: Use user_exit_on_hypercall() instead of opencode

[Binbin Wu]

Use user_exit_on_hypercall() instead of opencode.

No functional change intended.

Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com>
Reviewed-by: Isaku Yamahata <isaku.yamahata@intel.com>
Reviewed-by: Kai Huang <kai.huang@intel.com>
---
 arch/x86/kvm/svm/sev.c | 4 ++--
 arch/x86/kvm/x86.c     | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
index 714c517dd4b7..9b3d55474922 100644
--- a/arch/x86/kvm/svm/sev.c
+++ b/arch/x86/kvm/svm/sev.c
@@ -3637,7 +3637,7 @@ static int snp_begin_psc_msr(struct vcpu_svm *svm, u64 ghcb_msr)
 		return 1; /* resume guest */
 	}
 
-	if (!(vcpu->kvm->arch.hypercall_exit_enabled & (1 << KVM_HC_MAP_GPA_RANGE))) {
+	if (!user_exit_on_hypercall(vcpu->kvm, KVM_HC_MAP_GPA_RANGE)) {
 		set_ghcb_msr(svm, GHCB_MSR_PSC_RESP_ERROR);
 		return 1; /* resume guest */
 	}
@@ -3720,7 +3720,7 @@ static int snp_begin_psc(struct vcpu_svm *svm, struct psc_buffer *psc)
 	bool huge;
 	u64 gfn;
 
-	if (!(vcpu->kvm->arch.hypercall_exit_enabled & (1 << KVM_HC_MAP_GPA_RANGE))) {
+	if (!user_exit_on_hypercall(vcpu->kvm, KVM_HC_MAP_GPA_RANGE)) {
 		snp_complete_psc(svm, VMGEXIT_PSC_ERROR_GENERIC);
 		return 1;
 	}
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index e521f14ad2b2..c41ba387cc8c 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -10165,7 +10165,7 @@ unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
 		u64 gpa = a0, npages = a1, attrs = a2;
 
 		ret = -KVM_ENOSYS;
-		if (!(vcpu->kvm->arch.hypercall_exit_enabled & (1 << KVM_HC_MAP_GPA_RANGE)))
+		if (!user_exit_on_hypercall(vcpu->kvm, KVM_HC_MAP_GPA_RANGE))
 			break;
 
 		if (!PAGE_ALIGNED(gpa) || !npages ||
-- 
2.46.0

Re: [PATCH v3 2/2] KVM: x86: Use user_exit_on_hypercall() instead of opencode

[Xiaoyao Li]

On 8/26/2024 10:22 AM, Binbin Wu wrote:
> Use user_exit_on_hypercall() instead of opencode.
> 
> No functional change intended.
> 
> Signed-off-by: Binbin Wu <binbin.wu@linux.intel.com>
> Reviewed-by: Isaku Yamahata <isaku.yamahata@intel.com>
> Reviewed-by: Kai Huang <kai.huang@intel.com>

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

> ---
>   arch/x86/kvm/svm/sev.c | 4 ++--
>   arch/x86/kvm/x86.c     | 2 +-
>   2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c
> index 714c517dd4b7..9b3d55474922 100644
> --- a/arch/x86/kvm/svm/sev.c
> +++ b/arch/x86/kvm/svm/sev.c
> @@ -3637,7 +3637,7 @@ static int snp_begin_psc_msr(struct vcpu_svm *svm, u64 ghcb_msr)
>   		return 1; /* resume guest */
>   	}
>   
> -	if (!(vcpu->kvm->arch.hypercall_exit_enabled & (1 << KVM_HC_MAP_GPA_RANGE))) {
> +	if (!user_exit_on_hypercall(vcpu->kvm, KVM_HC_MAP_GPA_RANGE)) {
>   		set_ghcb_msr(svm, GHCB_MSR_PSC_RESP_ERROR);
>   		return 1; /* resume guest */
>   	}
> @@ -3720,7 +3720,7 @@ static int snp_begin_psc(struct vcpu_svm *svm, struct psc_buffer *psc)
>   	bool huge;
>   	u64 gfn;
>   
> -	if (!(vcpu->kvm->arch.hypercall_exit_enabled & (1 << KVM_HC_MAP_GPA_RANGE))) {
> +	if (!user_exit_on_hypercall(vcpu->kvm, KVM_HC_MAP_GPA_RANGE)) {
>   		snp_complete_psc(svm, VMGEXIT_PSC_ERROR_GENERIC);
>   		return 1;
>   	}
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index e521f14ad2b2..c41ba387cc8c 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -10165,7 +10165,7 @@ unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
>   		u64 gpa = a0, npages = a1, attrs = a2;
>   
>   		ret = -KVM_ENOSYS;
> -		if (!(vcpu->kvm->arch.hypercall_exit_enabled & (1 << KVM_HC_MAP_GPA_RANGE)))
> +		if (!user_exit_on_hypercall(vcpu->kvm, KVM_HC_MAP_GPA_RANGE))
>   			break;
>   
>   		if (!PAGE_ALIGNED(gpa) || !npages ||

Re: [PATCH v3 0/2] KVM: x86: Check hypercall's exit to userspace generically

[Binbin Wu]

Gentle ping...


On 8/26/2024 10:22 AM, Binbin Wu wrote:
> Currently in kvm_emulate_hypercall, KVM_HC_MAP_GPA_RANGE is checked
> specifically to decide whether a KVM hypercall needs to exit to userspace
> or not.  Do the check based on the hypercall_exit_enabled field of
> struct kvm_arch.
>
> Also use the API user_exit_on_hypercall() to replace the opencode.
>
> ---
> v3:
> - Rename is_kvm_hc_exit_enabled() to user_exit_on_hypercall(). (Sean)
> - Remove the WARN_ON_ONCE(). (Isaku, Sean)
> - Use BIT(hc_nr) instead of (1 << nr) (Yuan)
> - Added a comment to explain why check the !ret first. (Kai)
> - Add Kai and Isaku's Reviewed-by.
>
> v2:
> - Check the return value of __kvm_emulate_hypercall() before checking
>    hypercall_exit_enabled to avoid an invalid KVM hypercall nr.
>    https://lore.kernel.org/kvm/184d90a8-14a0-494a-9112-365417245911@linux.intel.com/
> - Add a warning if a hypercall nr out of the range of hypercall_exit_enabled
>    can express.
>
> Binbin Wu (2):
>    KVM: x86: Check hypercall's exit to userspace generically
>    KVM: x86: Use user_exit_on_hypercall() instead of opencode
>
>   arch/x86/kvm/svm/sev.c | 4 ++--
>   arch/x86/kvm/x86.c     | 7 ++++---
>   arch/x86/kvm/x86.h     | 4 ++++
>   3 files changed, 10 insertions(+), 5 deletions(-)
>
>
> base-commit: a1206bc992c3cd3f758a9b46117dfc7e59e8c10f