If compiled with CONFIG_FRAME_POINTER=y, objtool in not happy that
__tdx_hypercall() messes up RBP.
objtool: __tdx_hypercall+0x7f: return with modified stack frame
Rework the function to store TDX_HCALL_ flags on stack instead of RBP.
Signed-off-by: Kirill A. Shutemov <[email protected]>
Fixes: c30c4b2555ba ("x86/tdx: Refactor __tdx_hypercall() to allow pass down more arguments")
Link: https://lore.kernel.org/all/[email protected]
Reported-by: kernel test robot <[email protected]>
Cc: Peter Zijlstra <[email protected]>
---
The patch is against tip/x86/tdx. tip/sched/core removes
TDX_HCALL_ISSUE_STI. The trird hunk of the patch is not relevant
after that.
---
arch/x86/coco/tdx/tdcall.S | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/arch/x86/coco/tdx/tdcall.S b/arch/x86/coco/tdx/tdcall.S
index 5da06d1a9ba3..2bd436a4790d 100644
--- a/arch/x86/coco/tdx/tdcall.S
+++ b/arch/x86/coco/tdx/tdcall.S
@@ -131,11 +131,10 @@ SYM_FUNC_START(__tdx_hypercall)
push %r13
push %r12
push %rbx
- push %rbp
/* Free RDI and RSI to be used as TDVMCALL arguments */
movq %rdi, %rax
- movq %rsi, %rbp
+ push %rsi
/* Copy hypercall registers from arg struct: */
movq TDX_HYPERCALL_r8(%rax), %r8
@@ -168,7 +167,7 @@ SYM_FUNC_START(__tdx_hypercall)
* HLT operation indefinitely. Since this is the not the desired
* result, conditionally call STI before TDCALL.
*/
- testq $TDX_HCALL_ISSUE_STI, %rbp
+ testq $TDX_HCALL_ISSUE_STI, 8(%rsp)
jz .Lskip_sti
sti
.Lskip_sti:
@@ -188,7 +187,7 @@ SYM_FUNC_START(__tdx_hypercall)
pop %rax
/* Copy hypercall result registers to arg struct if needed */
- testq $TDX_HCALL_HAS_OUTPUT, %rbp
+ testq $TDX_HCALL_HAS_OUTPUT, (%rsp)
jz .Lout
movq %r8, TDX_HYPERCALL_r8(%rax)
@@ -218,11 +217,12 @@ SYM_FUNC_START(__tdx_hypercall)
xor %r10d, %r10d
xor %r11d, %r11d
xor %rdi, %rdi
- xor %rsi, %rsi
xor %rdx, %rdx
+ /* Remove TDX_HCALL_* flags from the stack */
+ pop %rsi
+
/* Restore callee-saved GPRs as mandated by the x86_64 ABI */
- pop %rbp
pop %rbx
pop %r12
pop %r13
--
2.39.1
On Mon, Jan 30, 2023 at 04:53:54PM +0300, Kirill A. Shutemov wrote:
> If compiled with CONFIG_FRAME_POINTER=y, objtool in not happy that
> __tdx_hypercall() messes up RBP.
>
> objtool: __tdx_hypercall+0x7f: return with modified stack frame
>
> Rework the function to store TDX_HCALL_ flags on stack instead of RBP.
>
> Signed-off-by: Kirill A. Shutemov <[email protected]>
> Fixes: c30c4b2555ba ("x86/tdx: Refactor __tdx_hypercall() to allow pass down more arguments")
> Link: https://lore.kernel.org/all/[email protected]
> Reported-by: kernel test robot <[email protected]>
> Cc: Peter Zijlstra <[email protected]>
> ---
>
> The patch is against tip/x86/tdx. tip/sched/core removes
> TDX_HCALL_ISSUE_STI. The trird hunk of the patch is not relevant
> after that.
Right, this should work. But it does leave me wondering, should we
perhaps strive to completely remove the flags thing and move to
__tdx_hypercall() and __tdx_hypercall_ret() or something? That is,
simply have two different functions, one with and one without return
data.
It should be trivial to generate that without actual code duplication.
On Mon, Jan 30, 2023 at 04:53:54PM +0300, Kirill A. Shutemov wrote:
> If compiled with CONFIG_FRAME_POINTER=y, objtool in not happy that
> __tdx_hypercall() messes up RBP.
>
> objtool: __tdx_hypercall+0x7f: return with modified stack frame
>
> Rework the function to store TDX_HCALL_ flags on stack instead of RBP.
Also, on IRC you mentioned that per TDX spec, BP is a valid argument
register too and you were going to raise this and get it fixed, TDX
hypercalls must not use BP to pass data.
On Tue, Jan 31, 2023 at 09:34:12AM +0100, Peter Zijlstra wrote:
> On Mon, Jan 30, 2023 at 04:53:54PM +0300, Kirill A. Shutemov wrote:
> > If compiled with CONFIG_FRAME_POINTER=y, objtool in not happy that
> > __tdx_hypercall() messes up RBP.
> >
> > objtool: __tdx_hypercall+0x7f: return with modified stack frame
> >
> > Rework the function to store TDX_HCALL_ flags on stack instead of RBP.
>
> Also, on IRC you mentioned that per TDX spec, BP is a valid argument
> register too and you were going to raise this and get it fixed, TDX
> hypercalls must not use BP to pass data.
I've raised the question yesterday. No progress so far. It will take time
to get it into the public spec.
--
Kiryl Shutsemau / Kirill A. Shutemov
On Tue, Jan 31, 2023 at 09:32:37AM +0100, Peter Zijlstra wrote:
> On Mon, Jan 30, 2023 at 04:53:54PM +0300, Kirill A. Shutemov wrote:
> > If compiled with CONFIG_FRAME_POINTER=y, objtool in not happy that
Oops, just noticed a typo. s/in/is/
> > __tdx_hypercall() messes up RBP.
> >
> > objtool: __tdx_hypercall+0x7f: return with modified stack frame
> >
> > Rework the function to store TDX_HCALL_ flags on stack instead of RBP.
> >
> > Signed-off-by: Kirill A. Shutemov <[email protected]>
> > Fixes: c30c4b2555ba ("x86/tdx: Refactor __tdx_hypercall() to allow pass down more arguments")
> > Link: https://lore.kernel.org/all/[email protected]
> > Reported-by: kernel test robot <[email protected]>
> > Cc: Peter Zijlstra <[email protected]>
> > ---
> >
> > The patch is against tip/x86/tdx. tip/sched/core removes
> > TDX_HCALL_ISSUE_STI. The trird hunk of the patch is not relevant
> > after that.
>
> Right, this should work. But it does leave me wondering, should we
> perhaps strive to completely remove the flags thing and move to
> __tdx_hypercall() and __tdx_hypercall_ret() or something? That is,
> simply have two different functions, one with and one without return
> data.
>
> It should be trivial to generate that without actual code duplication.
Yeah, that's doable. I will give it a try. I guess on top this one (plus
sched/core changes) should be.
--
Kiryl Shutsemau / Kirill A. Shutemov
On Tue, Jan 31, 2023 at 11:39:01AM +0300, Kirill A. Shutemov wrote:
> On Tue, Jan 31, 2023 at 09:34:12AM +0100, Peter Zijlstra wrote:
> > On Mon, Jan 30, 2023 at 04:53:54PM +0300, Kirill A. Shutemov wrote:
> > > If compiled with CONFIG_FRAME_POINTER=y, objtool in not happy that
> > > __tdx_hypercall() messes up RBP.
> > >
> > > objtool: __tdx_hypercall+0x7f: return with modified stack frame
> > >
> > > Rework the function to store TDX_HCALL_ flags on stack instead of RBP.
> >
> > Also, on IRC you mentioned that per TDX spec, BP is a valid argument
> > register too and you were going to raise this and get it fixed, TDX
> > hypercalls must not use BP to pass data.
>
> I've raised the question yesterday. No progress so far. It will take time
> to get it into the public spec.
Sure, just making sure it's not forgotten about. Thanks!