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

[Binbin Wu <binbin.wu@linux.intel.com>]

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