Confirm noinline produces the right asm for saving register state

[cperezvargas]

In drivers/hv/mshv_vtl_main.c: mshv_vtl_return_tdx()

17605 +/* TODO TDX: Confirm noinline produces the right asm for saving register
state */
17606 +noinline static void mshv_vtl_return_tdx(void)

[ramesh-thomas]

(Migrated internal issue #1266. Also covers internal issue #1260 which is a duplicate.)

yjiang5 on Oct 30:

I checked the binary built out and it's working. One minor concern is, if the not-used output operand may have potential issue in future since gcc does not have explicit definition on such situation, but at least the build out binary is correct.

[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]

Discussed with @yamahata. His comment:
I think the reason why noinline is used is that the function uses “register void *__sp asm(“rsp”)”.
Also TDG.VP.ENTER may change RBP.
Probably the author doesn’t want to play with stack pointer. I’m not sure why the function doesn’t save/restore RBP.

@peterfang do you have any suggestion?

[ramesh-thomas]

From another discussion with @yamahata:
Older TDX code was modifying RBP, but newer code does not so the noinline directive should be ok

@peterfang please confirm if TDX modifies RBP in TDG.VP.ENTER

[ramesh-thomas]

@chris-oo looks like this reason for the question of noinline is related to the issue in #566. The noinline is not interfering with the TDCALL code which currently assumes only RBP needs to be saved. Refer to the objdump output in #566

[yamahata]

The function seems over complicated. I guess inline assembly and specifying register are for optimization. We should drop noinline, inline assebmly, asm("regname"). And introduce .S file to write small assembly code to call TDCALL. Probably its signature is somethign like u64 tdg_vp_enter_asm(tdx_exit_info). mshv_vtl_return_tdx() should just call tdg_vp_enter_asm(). The overhead of one function call is negligible because of the TDG call and acessing MSR. Then the ugly code will be gone.

• noinline will be gone.
• register asm("regname") will be gone. (including suspicousregister void *__sp asm("rsp") )
• So we don't worry about stack pointer and frame pointer in the C code.
• inline assembly will be gone.
• In assembly code, we can use Linux UNWIND_HINT_*, FRAME_BEGIN/END

This applies to the following issues.

• Remove the TODO TDX for the build complaints #566
• [TODO TDX] Remove the TODO TDX for hardcode VP.ENTER #580

[yamahata]

From another discussion with @yamahata: Older TDX code was modifying RBP, but newer code does not so the noinline directive should be ok

@peterfang please confirm if TDX modifies RBP in TDG.VP.ENTER

RBP isn't preserved with TDG.VP.ENTER.
The reference is https://www.intel.com/content/www/us/en/developer/tools/trust-domain-extensions/documentation.html
Intel® Trust Domain Extensions (Intel® TDX) Module
Architecture Application Binary Interface (ABI)
Reference Specification

5.5.15. TDG.VP.ENTER Leaf
Table 5.358: TDG.VP.ENTER Output Operands Definition on an L2→L1 Exits Following an L1→L2 Entry

Other state Any state that the L2 VM is allowed to use may be modified.

please note that some registers are implicitly preserved as VMX guest register state.

[ramesh-thomas]

@chris-oo @mebersol @jstarks please review the proposal from @yamahata to move the inline code to a separate .S assembly file.

[ramesh-thomas]

RBP isn't preserved with TDG.VP.ENTER. The reference is https://www.intel.com/content/www/us/en/developer/tools/trust-domain-extensions/documentation.html Intel® Trust Domain Extensions (Intel® TDX) Module Architecture Application Binary Interface (ABI) Reference Specification

5.5.15. TDG.VP.ENTER Leaf Table 5.358: TDG.VP.ENTER Output Operands Definition on an L2→L1 Exits Following an L1→L2 Entry

Other state Any state that the L2 VM is allowed to use may be modified.

please note that some registers are implicitly preserved as VMX guest register state.

@yamahata That section seems to imply any register can be modified in TDG.VP.ENTER. Does it explicitly mention anywhere which registers are preserved and which are not? The inline assembly code is only preserving RBP, so is it currently assumed that everything other than RBP is preserved?

[yamahata]

RBP isn't preserved with TDG.VP.ENTER. The reference is https://www.intel.com/content/www/us/en/developer/tools/trust-domain-extensions/documentation.html Intel® Trust Domain Extensions (Intel® TDX) Module Architecture Application Binary Interface (ABI) Reference Specification
5.5.15. TDG.VP.ENTER Leaf Table 5.358: TDG.VP.ENTER Output Operands Definition on an L2→L1 Exits Following an L1→L2 Entry

Other state Any state that the L2 VM is allowed to use may be modified.

please note that some registers are implicitly preserved as VMX guest register state.

@yamahata That section seems to imply any register can be modified in TDG.VP.ENTER. Does it explicitly mention anywhere which registers are preserved and which are not? The inline assembly code is only preserving RBP, so is it currently assumed that everything other than RBP is preserved?

RBP is NOT preserved.
Intel TDX Module Partitioning Architecture Specification
L1-to-L2 VM Entry and L2-to-L1 VM Exit: TDG.VP.ENTER
Figure 22.2: Example of L1→L2 VM Entry and Exit

The figure states on L2->L1 VM Exit, the tdx module restores the L1 VCPU states with VMRESUME with L1 vCPU RIP advanced.
Then we can look at the SDM Virtual Machine Control Structure. Guest-State Area, Guest Register State:

The following fields in the guest-state area correspond to processor registers:
• Control registers CR0, CR3, and CR4 (64 bits each; 32 bits on processors that do not support Intel 64 architecture).
• Debug register DR7 (64 bits; 32 bits on processors that do not support Intel 64 architecture).
• RSP, RIP, and RFLAGS (64 bits each; 32 bits on processors that do not support Intel 64 architecture).
• The following fields for each of the registers CS, SS, DS, ES, FS, GS, LDTR, and TR:

For L0 TDH.VP.ENTER RBP preserving, TDX module enhancement is needed to save/restore L0 vCPU RBP.
Do you want something similar to L1 TDG.VP.ENTER to preserve L1 vCPU RBP?

[peterfang]

From another discussion with @yamahata: Older TDX code was modifying RBP, but newer code does not so the noinline directive should be ok
@peterfang please confirm if TDX modifies RBP in TDG.VP.ENTER

RBP isn't preserved with TDG.VP.ENTER. The reference is https://www.intel.com/content/www/us/en/developer/tools/trust-domain-extensions/documentation.html Intel® Trust Domain Extensions (Intel® TDX) Module Architecture Application Binary Interface (ABI) Reference Specification

5.5.15. TDG.VP.ENTER Leaf Table 5.358: TDG.VP.ENTER Output Operands Definition on an L2→L1 Exits Following an L1→L2 Entry

Other state Any state that the L2 VM is allowed to use may be modified.

please note that some registers are implicitly preserved as VMX guest register state.

I can also confirm that the TDX module does not preserve RBP in TDG.VP.ENTER

[ramesh-thomas]

For L0 TDH.VP.ENTER RBP preserving, TDX module enhancement is needed to save/restore L0 vCPU RBP. Do you want something similar to L1 TDG.VP.ENTER to preserve L1 vCPU RBP?

Yes, that would be good.

Shouldn't it be normal expectation for the host to be not concerned of its state getting altered in VP.ENTER? Otherwise, we would need to know exactly what to save and restore, not just RBP. If this is not the case, I am not sure how even moving the code to a separate .S assembly file will help, unless we can be sure that the stack frame will preserve all state that can get altered.