Remove the TODO TDX for the build complaints

[cperezvargas]

On drivers/hv/mshv_vtl_main.c: mshv_vtl_return_tdx(), there is TODO TDX for build complaints as below.

17656 + /* TODO TDX: pushq popq causes some build complaints unclear why
when mshv uses
17657 + it also. alignment checks even though tdcall has no alignment

The building complain are:
drivers/hv/mshv_vtl_main.o: warning: objtool: mshv_vtl_return_tdx+0x22d: call without frame pointer save/setup
drivers/hv/mshv_vtl_main.o: warning: objtool: mshv_vtl_switch_to_vtl0_irqoff+0x2b4: call without frame pointer save/setup

[ramesh-thomas]

(Migrated from internal issue #1258)

yjiang5 on Oct 30:

Please check microsoft/OHCL-Linux-Kernel#23 for the discussion there.

[ramesh-thomas]

The code:
drivers/hv/mshv_vtl_main.c

#define TDCALL_ASM ".byte 0x66,0x0f,0x01,0xcc"

/* TODO TDX: Confirm noinline produces the right asm for saving register state */
noinline void mshv_vtl_return_tdx(void)
{
struct tdx_tdg_vp_enter_exit_info *tdx_exit_info;
struct tdx_vp_state *tdx_vp_state;
struct mshv_vtl_run *vtl_run;
struct mshv_vtl_per_cpu *per_cpu;

register void *__sp asm("rsp");
register u64 r8 asm("r8");
register u64 r9 asm("r9");
register u64 r10 asm("r10");
register u64 r11 asm("r11");
register u64 r12 asm("r12");
register u64 r13 asm("r13");
register u64 r14 asm("r14");
register u64 r15 asm("r15");

...

r8 = 0;
r9 = 0;
r10 = 0;
r11 = 0;
r12 = 0;
r13 = 0;
r14 = 0;
r15 = 0;

/*
 * TODO TDX: pushq popq causes some build complaints unclear why when
 * mshv uses it also. Alignment checks even though tdcall has no alignment reqs?
 */
asm __volatile__ (\
        /* Save RBP onto the stack since it'll be clobbered and inline asm won't save it. */
        "pushq  %%rbp\n"
        TDCALL_ASM "\n"
        /* restore rbp from the stack */
        "popq   %%rbp\n"
        : "=a"(tdx_exit_info->rax), "=c"(tdx_exit_info->rcx),
          "=d"(tdx_exit_info->rdx), "=S"(tdx_exit_info->rsi), "=D"(tdx_exit_info->rdi),
          "=r" (r8), "=r" (r9), "=r" (r10), "=r" (r11), "=r"(r12), "=r"(r13), "=r"(r14),
          "=r"(r15), "+r" (__sp)
        : "a"(input_rax), "c"(input_rcx), "d"(input_rdx)
        : "rbx", "cc", "memory" /* TODO: is the "cc" necessary? */
);

tdx_exit_info->r8 = r8;
tdx_exit_info->r9 = r9;
tdx_exit_info->r10 = r10;
tdx_exit_info->r11 = r11;
tdx_exit_info->r12 = r12;
tdx_exit_info->r13 = r13;

[ramesh-thomas]

Yunhong's suggestion of using UNWIND_HINT_SAVE/UNWIND_HINT_RESTORE in the linked discussion microsoft/OHCL-Linux-Kernel#23:
"Add UNWIND_HINT_SAVE/UNWIND_HINT_RESTORE before and after the inline assembly code, so that objtools will save the CFI information before the "pushq %rbp" and restore it after "popq %rbp"."

Based on his suggestion, will the below code change resolve the issue?
asm volatile (
UNWIND_HINT_SAVE
/* Save RBP onto the stack since it'll be clobbered and inline asm won't save it. /
"pushq %%rbp\n"
TDCALL_ASM "\n"
/ restore rbp from the stack /
"popq %%rbp\n"
UNWIND_HINT_RESTORE
: "=a"(tdx_exit_info->rax), "=c"(tdx_exit_info->rcx),
"=d"(tdx_exit_info->rdx), "=S"(tdx_exit_info->rsi), "=D"(tdx_exit_info->rdi),
"=r" (r8), "=r" (r9), "=r" (r10), "=r" (r11), "=r"(r12), "=r"(r13), "=r"(r14),
"=r"(r15), "+r" (__sp)
: "a"(input_rax), "c"(input_rcx), "d"(input_rdx)
: "rbx", "cc", "memory" / TODO: is the "cc" necessary? */
);

[ramesh-thomas]

From discussion with @yamahata:

Isaku:
Yes, UNWIND_HINT_SAVE/RESTORE is the macro to suppress the objtool warning.
But to me, the funciton with inline assembly is too ugly. We can just write assembly code that invoke tdcall. And we can call it.
It seems too early optimization.

We need mov %rsp, %rbp for stack unwinder.

Isaku suggests using regular assembly in a .S file. It calls FRAME_BEING/FRAME_END macros to setup/save/restore the stack frame.

Isaku:
Yes. wite .S file to define a function and call it like linux/arch/x86/virt/vmx/tdx/tdxcall.S and linux/arch/x86/coco/tdx/tdcall.S
I think a function call overhead is negligible because we do tdcall instruction and updating control registers and MSRs.

You need to twist it for TDG.VP.ENTER. We have an implementation for TDH.VP.ENTER.
And struct for guest registers is different from KVM's.

The existing TDCALL doesn't save/restore necessary GPRs. the calling convention of TDG.VP.ENTER is different.
You need to adjust it.

@peterfang please confirm whether TDG.VP.ENTER modifies RBP

[ramesh-thomas]

: "rbx", "cc", "memory" /_ TODO: is the "cc" necessary? */ );

Isaku: “cc”: This is not required because RFLAGS is preserved.

[ramesh-thomas]

Some options are:

1. If @peterfang confirms that latest TDX code does not modify RBP, then we can simply remove the push/pop rbp
2. If not, the warning is benign and can be suppressed using the "UNWIND" macros as shown above. I have compile tested and compared objdump output listed below.
3. Implement new assembly function in a .S file as suggested by @yamahata

@chris-oo please suggest which option to use. Below are objdump outputs for #2

Original code as it is now:
asm volatile (
213c: b8 19 00 00 00 mov $0x19,%eax
2141: 4c 89 e9 mov %r13,%rcx
2144: 4c 89 e2 mov %r12,%rdx
2147: 55 push %rbp
2148: 66 0f 01 cc tdcall
214c: 5d pop %rbp
214d: 48 8b 5d c8 mov -0x38(%rbp),%rbx
2151: 48 89 83 10 01 00 00 mov %rax,0x110(%rbx)
2158: 48 8b 45 c0 mov -0x40(%rbp),%rax
215c: 48 89 48 08 mov %rcx,0x8(%rax)
2160: 48 89 50 10 mov %rdx,0x10(%rax)
2164: 48 89 70 18 mov %rsi,0x18(%rax)
2168: 48 89 78 20 mov %rdi,0x20(%rax)
tdx_exit_info->r8 = r8;
216c: 4c 89 83 38 01 00 00 mov %r8,0x138(%rbx)
tdx_exit_info->r9 = r9;
2173: 4c 89 8b 40 01 00 00 mov %r9,0x140(%rbx)
tdx_exit_info->r10 = r10;
217a: 4c 89 93 48 01 00 00 mov %r10,0x148(%rbx)
tdx_exit_info->r11 = r11;
2181: 4c 89 9b 50 01 00 00 mov %r11,0x150(%rbx)
tdx_exit_info->r12 = r12;
2188: 4c 89 a3 58 01 00 00 mov %r12,0x158(%rbx)

With the UNWIND_HINT_SAVE and UNWIND_HINT_RESTORE macros:
asm volatile (
213c: b8 19 00 00 00 mov $0x19,%eax
2141: 4c 89 e9 mov %r13,%rcx
2144: 4c 89 e2 mov %r12,%rdx
2147: 55 push %rbp
2148: 66 0f 01 cc tdcall
214c: 5d pop %rbp
214d: 48 8b 5d c8 mov -0x38(%rbp),%rbx
2151: 48 89 83 10 01 00 00 mov %rax,0x110(%rbx)
2158: 48 8b 45 c0 mov -0x40(%rbp),%rax
215c: 48 89 48 08 mov %rcx,0x8(%rax)
2160: 48 89 50 10 mov %rdx,0x10(%rax)
2164: 48 89 70 18 mov %rsi,0x18(%rax)
2168: 48 89 78 20 mov %rdi,0x20(%rax)
tdx_exit_info->r8 = r8;
216c: 4c 89 83 38 01 00 00 mov %r8,0x138(%rbx)
tdx_exit_info->r9 = r9;
2173: 4c 89 8b 40 01 00 00 mov %r9,0x140(%rbx)
tdx_exit_info->r10 = r10;
217a: 4c 89 93 48 01 00 00 mov %r10,0x148(%rbx)
tdx_exit_info->r11 = r11;
2181: 4c 89 9b 50 01 00 00 mov %r11,0x150(%rbx)
tdx_exit_info->r12 = r12;
2188: 4c 89 a3 58 01 00 00 mov %r12,0x158(%rbx)

[ramesh-thomas]

Adding @mebersol @jstarks who are covering for @chris-oo

[chris-oo]

It seems like this issue depends on the other one we have about perhaps moving this return call to a separate asm file?

[ramesh-thomas]

Yes, the warning is about the push/pop RBP. We can either call the UNWIND_HINT_SAVE/UNWIND_HINT_RESTORE macros as shown in the code above or move the code to a separate .S file where the stack frame setup would be clearer to the compiler.