[RFC] hv, vtl: Fix the frame pointer build warning

[yjiang5]

Currently there are stack frame pointer warning as following.

drivers/hv/mshv_vtl_main.o: warning: objtool: mshv_vtl_return_tdx+0x22d: call without frame pointer save/setup

Such warning comes from the "pushq %%rbp" and "popq %%rbp" instructions around the tdcall.

When CONFIG_FRAME_POINTER is enabled, the objtools expects the stack frame pointer is setup as shown in has_valid_stack_frame() function:

static bool has_valid_stack_frame(struct insn_state state) { struct cfi_state cfi = &state->cfi;

if (cfi->cfa.base == CFI_BP &&
    check_reg_frame_pos(&cfi->regs[CFI_BP], -cfi->cfa.offset) &&
    check_reg_frame_pos(&cfi->regs[CFI_RA], -cfi->cfa.offset + 8))
    return true;

if (cfi->drap && cfi->regs[CFI_BP].base == CFI_BP)
    return true;

return false;

}

However, the "popq %%rbp" instruction confuses the objtools. As shown in update_cfi_state(), the CFA's base is changed to SP after the "pop %rbp" instuction. case OP_SRC_POP: case OP_SRC_POPF: if (!cfi->drap && op->dest.reg == cfa->base) {

        /* pop %rbp */
        cfa->base = CFI_SP;
    }

This cause the has_valid_stack_frame() fail in the next function call kernel_fpu_end().

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".

[chris-oo]

This looks good to me, but I'm not an expert on how unwinding works in Linux... Is there anyone else who has expertise in this area?

[romank-msft]

This looks short and sweet!

Another way of fixing this would be where the benefits include the compiler deciding when to save/restore rbp (potentially more room for optimization) and less assembly:

• Separate out all the switching code into a separate file, e.g. _mshv_vtl_return.c
• Compile it without frame pointer (i.e. treat rbp as a GPR): CFLAGS_mshv_vtl_return.o := -fomit-frame-pointer

• and then (non-TDX), still should apply

  STACK_FRAME_NON_STANDARD_FP(mshv_vtl_return);
  void mshv_vtl_return(struct mshv_cpu_context *vtl0, u64 hypercall_addr)
  {
  struct hv_vp_assist_page *hvp;
  register u64 rbp asm("rbp");
  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");
  hvp = hv_vp_assist_page[smp_processor_id()];
  hvp->vtl_ret_x64rax = vtl0->rax;
  hvp->vtl_ret_x64rcx = vtl0->rcx;
  kernel_fpu_begin_mask(0);
  fxrstor(&vtl0->fx_state);
  native_write_cr2(vtl0->cr2);
  
  rbp = vtl0->rbp;
  r8 = vtl0->r8;
  r9 = vtl0->r9;
  r10 = vtl0->r10;
  r11 = vtl0->r11;
  r12 = vtl0->r12;
  r13 = vtl0->r13;
  r14 = vtl0->r14;
  r15 = vtl0->r15;
  asm_inline volatile (
  // Transition to the lower VTL
      CALL_NOSPEC
  // The registers contain the lower VTL's context after it is trapped,
  // and the excution continues here. 
      : "=a"(vtl0->rax), "=c"(vtl0->rcx),
        "+d"(vtl0->rdx), "+b"(vtl0->rbx), "+S"(vtl0->rsi), "+D"(vtl0->rdi),
        "+r"(rbp), "+r"(r8), "+r"(r9), "+r"(r10), "+r"(r11),
        "+r"(r12), "+r"(r13), "+r"(r14), "+r"(r15)
      : THUNK_TARGET(hypercall_addr), "c"(HV_VTL_RETURN_INPUT_NORMAL_RETURN)
      : "cc", "memory");
  vtl0->rbp = rbp;
  vtl0->r8 = r8;
  vtl0->r9 = r9;
  vtl0->r10 = r10;
  vtl0->r11 = r11;
  vtl0->r12 = r12;
  vtl0->r13 = r13;
  vtl0->r14 = r14;
  vtl0->r15 = r15;
  vtl0->cr2 = native_read_cr2();
  fxsave(&vtl0->fx_state);
  kernel_fpu_end();
  }

  What drawbacks do you see in this approach? To put this on the quantitative footing, could disasm the both versions and see what's better.

[yjiang5]

Sure, I can have a try on this. I have some question/concern on the assembly code, but I can try to see if it works and then back to you.