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;
}
