arch/x86/include/asm/kvm_host.h | 6 ---- arch/x86/kvm/svm/sev.c | 4 +-- arch/x86/kvm/x86.c | 50 +++++++++++---------------------- arch/x86/kvm/x86.h | 28 ++++++++++++++++++ 4 files changed, 47 insertions(+), 41 deletions(-)
Effectively v4 of Binbin's series to handle hypercall exits to userspace in
a generic manner, so that TDX
Binbin and Kai, this is fairly different that what we last discussed. While
sorting through Binbin's latest patch, I stumbled on what I think/hope is an
approach that will make life easier for TDX. Rather than have common code
set the return value, _and_ have TDX implement a callback to do the same for
user return MSRs, just use the callback for all paths.
As for abusing vcpu->run->hypercall.ret... It's obviously a bit gross, but
I think it's a lesser evil than having multiple a one-line wrappers just to
trampoline in the return code.
v4:
- Fix an SEV-* bug where KVM trips the WARN in is_64_bit_mode().
- Add a pile of reworks to (hopefully) avoid as much duplicate code when
TDX comes along.
v3: https://lore.kernel.org/all/20240826022255.361406-1-binbin.wu@linux.intel.com
Binbin Wu (1):
KVM: x86: Add a helper to check for user interception of KVM
hypercalls
Sean Christopherson (5):
KVM: x86: Play nice with protected guests in complete_hypercall_exit()
KVM: x86: Move "emulate hypercall" function declarations to x86.h
KVM: x86: Bump hypercall stat prior to fully completing hypercall
KVM: x86: Always complete hypercall via function callback
KVM: x86: Refactor __kvm_emulate_hypercall() into a macro
arch/x86/include/asm/kvm_host.h | 6 ----
arch/x86/kvm/svm/sev.c | 4 +--
arch/x86/kvm/x86.c | 50 +++++++++++----------------------
arch/x86/kvm/x86.h | 28 ++++++++++++++++++
4 files changed, 47 insertions(+), 41 deletions(-)
base-commit: 4d911c7abee56771b0219a9fbf0120d06bdc9c14
--
2.47.0.338.g60cca15819-goog
On Wed, 2024-11-27 at 16:43 -0800, Sean Christopherson wrote: > Effectively v4 of Binbin's series to handle hypercall exits to userspace in > a generic manner, so that TDX > > Binbin and Kai, this is fairly different that what we last discussed. While > sorting through Binbin's latest patch, I stumbled on what I think/hope is an > approach that will make life easier for TDX. Rather than have common code > set the return value, _and_ have TDX implement a callback to do the same for > user return MSRs, just use the callback for all paths. > > As for abusing vcpu->run->hypercall.ret... It's obviously a bit gross, but > I think it's a lesser evil than having multiple a one-line wrappers just to > trampoline in the return code. Doesn't seem to be "gross" to me, and AFAICT now for TDX we just need to play with __kvm_emulate_hypercall() with a TDX-specific completion callback. Which is nice. Thanks! For this series: Reviewed-by: Kai Huang <kai.huang@intel.com>
On Wed, 27 Nov 2024 16:43:38 -0800, Sean Christopherson wrote:
> Effectively v4 of Binbin's series to handle hypercall exits to userspace in
> a generic manner, so that TDX
>
> Binbin and Kai, this is fairly different that what we last discussed. While
> sorting through Binbin's latest patch, I stumbled on what I think/hope is an
> approach that will make life easier for TDX. Rather than have common code
> set the return value, _and_ have TDX implement a callback to do the same for
> user return MSRs, just use the callback for all paths.
>
> [...]
Applied patch 1 to kvm-x86 fixes. I'm going to hold off on the rest until the
dust settles on the SEAMCALL interfaces, e.g. in case TDX ends up marshalling
state into the "normal" GPRs.
[1/6] KVM: x86: Play nice with protected guests in complete_hypercall_exit()
https://github.com/kvm-x86/linux/commit/a317794eefd0
[2/6] KVM: x86: Add a helper to check for user interception of KVM hypercalls
(no commit info)
[3/6] KVM: x86: Move "emulate hypercall" function declarations to x86.h
(no commit info)
[4/6] KVM: x86: Bump hypercall stat prior to fully completing hypercall
(no commit info)
[5/6] KVM: x86: Always complete hypercall via function callback
(no commit info)
[6/6] KVM: x86: Refactor __kvm_emulate_hypercall() into a macro
(no commit info)
--
https://github.com/kvm-x86/linux/tree/next
On 12/19/2024 10:40 AM, Sean Christopherson wrote:
> On Wed, 27 Nov 2024 16:43:38 -0800, Sean Christopherson wrote:
>> Effectively v4 of Binbin's series to handle hypercall exits to userspace in
>> a generic manner, so that TDX
>>
>> Binbin and Kai, this is fairly different that what we last discussed. While
>> sorting through Binbin's latest patch, I stumbled on what I think/hope is an
>> approach that will make life easier for TDX. Rather than have common code
>> set the return value, _and_ have TDX implement a callback to do the same for
>> user return MSRs, just use the callback for all paths.
>>
>> [...]
> Applied patch 1 to kvm-x86 fixes. I'm going to hold off on the rest until the
> dust settles on the SEAMCALL interfaces, e.g. in case TDX ends up marshalling
> state into the "normal" GPRs.
Hi Sean, Based on your suggestions in the link https://lore.kernel.org/kvm/Z1suNzg2Or743a7e@google.com, the v2 of "KVM: TDX: TDX hypercalls may exit to userspace" is planned to morph the TDG.VP.VMCALL with KVM hypercall to EXIT_REASON_VMCALL and marshall r10~r14 from vp_enter_args in struct vcpu_tdx to the appropriate x86 registers for KVM hypercall handling.
diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index ef66985ddc91..d5aaf66af835 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -935,6 +935,23 @@ fastpath_t tdx_vcpu_run(struct kvm_vcpu *vcpu, bool force_immediate_exit)
return tdx_exit_handlers_fastpath(vcpu);
}
+static int complete_hypercall_exit(struct kvm_vcpu *vcpu)
+{
+ tdvmcall_set_return_code(vcpu, vcpu->run->hypercall.ret);
+ return 1;
+}
+
+static int tdx_emulate_vmcall(struct kvm_vcpu *vcpu)
+{
+ kvm_rax_write(vcpu, to_tdx(vcpu)->vp_enter_args.r10);
+ kvm_rbx_write(vcpu, to_tdx(vcpu)->vp_enter_args.r11);
+ kvm_rcx_write(vcpu, to_tdx(vcpu)->vp_enter_args.r12);
+ kvm_rdx_write(vcpu, to_tdx(vcpu)->vp_enter_args.r13);
+ kvm_rsi_write(vcpu, to_tdx(vcpu)->vp_enter_args.r14);
+
+ return __kvm_emulate_hypercall(vcpu, 0, complete_hypercall_exit);
+}
+
static int handle_tdvmcall(struct kvm_vcpu *vcpu)
{
switch (tdvmcall_leaf(vcpu)) {
@@ -1286,6 +1303,8 @@ int tdx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t fastpath)
return 0;
case EXIT_REASON_TDCALL:
return handle_tdvmcall(vcpu);
+ case EXIT_REASON_VMCALL:
+ return tdx_emulate_vmcall(vcpu);
default:
break;
}
To test TDX, I made some modifications to your patch
"KVM: x86: Refactor __kvm_emulate_hypercall() into a macro"
Are the following changes make sense to you?
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index a2198807290b..2c5df57ad799 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -10088,9 +10088,7 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
if (kvm_hv_hypercall_enabled(vcpu))
return kvm_hv_hypercall(vcpu);
- return __kvm_emulate_hypercall(vcpu, rax, rbx, rcx, rdx, rsi,
- is_64_bit_hypercall(vcpu),
- kvm_x86_call(get_cpl)(vcpu),
+ return __kvm_emulate_hypercall(vcpu, kvm_x86_call(get_cpl)(vcpu),
complete_hypercall_exit);
}
EXPORT_SYMBOL_GPL(kvm_emulate_hypercall);
diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
index b00ecbfef000..989bed5b48b0 100644
--- a/arch/x86/kvm/x86.h
+++ b/arch/x86/kvm/x86.h
@@ -623,19 +623,18 @@ int ____kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
int op_64_bit, int cpl,
int (*complete_hypercall)(struct kvm_vcpu *));
-#define __kvm_emulate_hypercall(_vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl, complete_hypercall) \
-({ \
- int __ret; \
- \
- __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, \
- complete_hypercall); \
- \
- if (__ret > 0) \
- __ret = complete_hypercall(_vcpu); \
- __ret; \
+#define __kvm_emulate_hypercall(_vcpu, cpl, complete_hypercall) \
+({ \
+ int __ret; \
+ __ret = ____kvm_emulate_hypercall(_vcpu, kvm_rax_read(_vcpu), \
+ kvm_rbx_read(_vcpu), kvm_rcx_read(_vcpu), \
+ kvm_rdx_read(_vcpu), kvm_rsi_read(_vcpu), \
+ is_64_bit_hypercall(_vcpu), cpl, \
+ complete_hypercall); \
+ \
+ if (__ret > 0) \
+ __ret = complete_hypercall(_vcpu); \
+ __ret; \
})
int kvm_emulate_hypercall(struct kvm_vcpu *vcpu);
On Wed, Jan 15, 2025, Binbin Wu wrote:
> On 12/19/2024 10:40 AM, Sean Christopherson wrote:
> > On Wed, 27 Nov 2024 16:43:38 -0800, Sean Christopherson wrote:
> > > Effectively v4 of Binbin's series to handle hypercall exits to userspace in
> > > a generic manner, so that TDX
> > >
> > > Binbin and Kai, this is fairly different that what we last discussed. While
> > > sorting through Binbin's latest patch, I stumbled on what I think/hope is an
> > > approach that will make life easier for TDX. Rather than have common code
> > > set the return value, _and_ have TDX implement a callback to do the same for
> > > user return MSRs, just use the callback for all paths.
> > >
> > > [...]
> > Applied patch 1 to kvm-x86 fixes. I'm going to hold off on the rest until the
> > dust settles on the SEAMCALL interfaces, e.g. in case TDX ends up marshalling
> > state into the "normal" GPRs.
> Hi Sean, Based on your suggestions in the link
> https://lore.kernel.org/kvm/Z1suNzg2Or743a7e@google.com, the v2 of "KVM: TDX:
> TDX hypercalls may exit to userspace" is planned to morph the TDG.VP.VMCALL
> with KVM hypercall to EXIT_REASON_VMCALL and marshall r10~r14 from
> vp_enter_args in struct vcpu_tdx to the appropriate x86 registers for KVM
> hypercall handling.
...
> To test TDX, I made some modifications to your patch
> "KVM: x86: Refactor __kvm_emulate_hypercall() into a macro"
> Are the following changes make sense to you?
Yes, but I think we can go a step further and effectively revert the bulk of commit
e913ef159fad ("KVM: x86: Split core of hypercall emulation to helper function"),
i.e. have ____kvm_emulate_hypercall() read the GPRs instead of passing them in
via the macro.
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index a2198807290b..2c5df57ad799 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -10088,9 +10088,7 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
> if (kvm_hv_hypercall_enabled(vcpu))
> return kvm_hv_hypercall(vcpu);
> - return __kvm_emulate_hypercall(vcpu, rax, rbx, rcx, rdx, rsi,
> - is_64_bit_hypercall(vcpu),
> - kvm_x86_call(get_cpl)(vcpu),
> + return __kvm_emulate_hypercall(vcpu, kvm_x86_call(get_cpl)(vcpu),
> complete_hypercall_exit);
> }
> EXPORT_SYMBOL_GPL(kvm_emulate_hypercall);
> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
> index b00ecbfef000..989bed5b48b0 100644
> --- a/arch/x86/kvm/x86.h
> +++ b/arch/x86/kvm/x86.h
> @@ -623,19 +623,18 @@ int ____kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
> int op_64_bit, int cpl,
> int (*complete_hypercall)(struct kvm_vcpu *));
> -#define __kvm_emulate_hypercall(_vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl, complete_hypercall) \
> -({ \
> - int __ret; \
> - \
> - __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, \
> - complete_hypercall); \
> - \
> - if (__ret > 0) \
> - __ret = complete_hypercall(_vcpu); \
> - __ret; \
> +#define __kvm_emulate_hypercall(_vcpu, cpl, complete_hypercall) \
> +({ \
> + int __ret; \
> + __ret = ____kvm_emulate_hypercall(_vcpu, kvm_rax_read(_vcpu), \
> + kvm_rbx_read(_vcpu), kvm_rcx_read(_vcpu), \
> + kvm_rdx_read(_vcpu), kvm_rsi_read(_vcpu), \
> + is_64_bit_hypercall(_vcpu), cpl, \
> + complete_hypercall); \
> + \
> + if (__ret > 0) \
> + __ret = complete_hypercall(_vcpu); \
> + __ret; \
> })
> int kvm_emulate_hypercall(struct kvm_vcpu *vcpu);
>
>
On 1/18/2025 3:31 AM, Sean Christopherson wrote:
> On Wed, Jan 15, 2025, Binbin Wu wrote:
>> On 12/19/2024 10:40 AM, Sean Christopherson wrote:
>>> On Wed, 27 Nov 2024 16:43:38 -0800, Sean Christopherson wrote:
>>>> Effectively v4 of Binbin's series to handle hypercall exits to userspace in
>>>> a generic manner, so that TDX
>>>>
>>>> Binbin and Kai, this is fairly different that what we last discussed. While
>>>> sorting through Binbin's latest patch, I stumbled on what I think/hope is an
>>>> approach that will make life easier for TDX. Rather than have common code
>>>> set the return value, _and_ have TDX implement a callback to do the same for
>>>> user return MSRs, just use the callback for all paths.
>>>>
>>>> [...]
>>> Applied patch 1 to kvm-x86 fixes. I'm going to hold off on the rest until the
>>> dust settles on the SEAMCALL interfaces, e.g. in case TDX ends up marshalling
>>> state into the "normal" GPRs.
>> Hi Sean, Based on your suggestions in the link
>> https://lore.kernel.org/kvm/Z1suNzg2Or743a7e@google.com, the v2 of "KVM: TDX:
>> TDX hypercalls may exit to userspace" is planned to morph the TDG.VP.VMCALL
>> with KVM hypercall to EXIT_REASON_VMCALL and marshall r10~r14 from
>> vp_enter_args in struct vcpu_tdx to the appropriate x86 registers for KVM
>> hypercall handling.
> ...
>
>> To test TDX, I made some modifications to your patch
>> "KVM: x86: Refactor __kvm_emulate_hypercall() into a macro"
>> Are the following changes make sense to you?
> Yes, but I think we can go a step further and effectively revert the bulk of commit
> e913ef159fad ("KVM: x86: Split core of hypercall emulation to helper function"),
> i.e. have ____kvm_emulate_hypercall() read the GPRs instead of passing them in
> via the macro.
Sure.
Are you OK if I sent the change (as a prep patch) along with v2 of
"TDX hypercalls may exit to userspace"?
>
>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
>> index a2198807290b..2c5df57ad799 100644
>> --- a/arch/x86/kvm/x86.c
>> +++ b/arch/x86/kvm/x86.c
>> @@ -10088,9 +10088,7 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
>> if (kvm_hv_hypercall_enabled(vcpu))
>> return kvm_hv_hypercall(vcpu);
>> - return __kvm_emulate_hypercall(vcpu, rax, rbx, rcx, rdx, rsi,
>> - is_64_bit_hypercall(vcpu),
>> - kvm_x86_call(get_cpl)(vcpu),
>> + return __kvm_emulate_hypercall(vcpu, kvm_x86_call(get_cpl)(vcpu),
>> complete_hypercall_exit);
>> }
>> EXPORT_SYMBOL_GPL(kvm_emulate_hypercall);
>> diff --git a/arch/x86/kvm/x86.h b/arch/x86/kvm/x86.h
>> index b00ecbfef000..989bed5b48b0 100644
>> --- a/arch/x86/kvm/x86.h
>> +++ b/arch/x86/kvm/x86.h
>> @@ -623,19 +623,18 @@ int ____kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
>> int op_64_bit, int cpl,
>> int (*complete_hypercall)(struct kvm_vcpu *));
>> -#define __kvm_emulate_hypercall(_vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl, complete_hypercall) \
>> -({ \
>> - int __ret; \
>> - \
>> - __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, \
>> - complete_hypercall); \
>> - \
>> - if (__ret > 0) \
>> - __ret = complete_hypercall(_vcpu); \
>> - __ret; \
>> +#define __kvm_emulate_hypercall(_vcpu, cpl, complete_hypercall) \
>> +({ \
>> + int __ret; \
>> + __ret = ____kvm_emulate_hypercall(_vcpu, kvm_rax_read(_vcpu), \
>> + kvm_rbx_read(_vcpu), kvm_rcx_read(_vcpu), \
>> + kvm_rdx_read(_vcpu), kvm_rsi_read(_vcpu), \
>> + is_64_bit_hypercall(_vcpu), cpl, \
>> + complete_hypercall); \
>> + \
>> + if (__ret > 0) \
>> + __ret = complete_hypercall(_vcpu); \
>> + __ret; \
>> })
>> int kvm_emulate_hypercall(struct kvm_vcpu *vcpu);
>>
>>
On Mon, Jan 20, 2025, Binbin Wu wrote:
> On 1/18/2025 3:31 AM, Sean Christopherson wrote:
> > On Wed, Jan 15, 2025, Binbin Wu wrote:
> > > On 12/19/2024 10:40 AM, Sean Christopherson wrote:
> > > > On Wed, 27 Nov 2024 16:43:38 -0800, Sean Christopherson wrote:
> > > > > Effectively v4 of Binbin's series to handle hypercall exits to userspace in
> > > > > a generic manner, so that TDX
> > > > >
> > > > > Binbin and Kai, this is fairly different that what we last discussed. While
> > > > > sorting through Binbin's latest patch, I stumbled on what I think/hope is an
> > > > > approach that will make life easier for TDX. Rather than have common code
> > > > > set the return value, _and_ have TDX implement a callback to do the same for
> > > > > user return MSRs, just use the callback for all paths.
> > > > >
> > > > > [...]
> > > > Applied patch 1 to kvm-x86 fixes. I'm going to hold off on the rest until the
> > > > dust settles on the SEAMCALL interfaces, e.g. in case TDX ends up marshalling
> > > > state into the "normal" GPRs.
> > > Hi Sean, Based on your suggestions in the link
> > > https://lore.kernel.org/kvm/Z1suNzg2Or743a7e@google.com, the v2 of "KVM: TDX:
> > > TDX hypercalls may exit to userspace" is planned to morph the TDG.VP.VMCALL
> > > with KVM hypercall to EXIT_REASON_VMCALL and marshall r10~r14 from
> > > vp_enter_args in struct vcpu_tdx to the appropriate x86 registers for KVM
> > > hypercall handling.
> > ...
> >
> > > To test TDX, I made some modifications to your patch
> > > "KVM: x86: Refactor __kvm_emulate_hypercall() into a macro"
> > > Are the following changes make sense to you?
> > Yes, but I think we can go a step further and effectively revert the bulk of commit
> > e913ef159fad ("KVM: x86: Split core of hypercall emulation to helper function"),
> > i.e. have ____kvm_emulate_hypercall() read the GPRs instead of passing them in
> > via the macro.
>
> Sure.
>
> Are you OK if I sent the change (as a prep patch) along with v2 of
> "TDX hypercalls may exit to userspace"?
Ya, go for it.
© 2016 - 2026 Red Hat, Inc.