Re: [PATCH v19 007/130] x86/virt/tdx: Export SEAMCALL functions

kirill.shutemov
2024-04-19T14:46:08Z

On Thu, Apr 18, 2024 at 11:26:11AM -0700, Sean Christopherson wrote:
> On Thu, Apr 18, 2024, kirill.shutemov@linux.intel.com wrote:
> > On Tue, Apr 16, 2024 at 07:45:18PM +0000, Edgecombe, Rick P wrote:
> > > On Wed, 2024-04-10 at 15:49 +0300, Kirill A. Shutemov wrote:
> > > > On Fri, Mar 15, 2024 at 09:33:20AM -0700, Sean Christopherson wrote:
> > > > > So my feedback is to not worry about the exports, and instead focus on
> > > > > figuring
> > > > > out a way to make the generated code less bloated and easier to read/debug.
> > > >
> > > > I think it was mistake trying to centralize TDCALL/SEAMCALL calls into
> > > > few megawrappers. I think we can get better results by shifting leaf
> > > > function wrappers into assembly.
> > > >
> > > > We are going to have more assembly, but it should produce better result.
> > > > Adding macros can help to write such wrapper and minimizer boilerplate.
> > > >
> > > > Below is an example of how it can look like. It's not complete. I only
> > > > converted TDCALLs, but TDVMCALLs or SEAMCALLs. TDVMCALLs are going to be
> > > > more complex.
> > > >
> > > > Any opinions? Is it something worth investing more time?
> > >
> > > We discussed offline how implementing these for each TDVM/SEAMCALL increases the
> > > chances of a bug in just one TDVM/SEAMCALL. Which could making debugging
> > > problems more challenging. Kirill raised the possibility of some code generating
> > > solution like cpufeatures.h, that could take a spec and generate correct calls.
> > >
> > > So far no big wins have presented themselves. Kirill, do we think the path to
> > > move the messy part out-of-line will not work?
> >
> > I converted all TDCALL and TDVMCALL leafs to direct assembly wrappers.
> > Here's WIP branch: https://github.com/intel/tdx/commits/guest-tdx-asm/
> >
> > I still need to clean it up and write commit messages and comments for all
> > wrappers.
> >
> > Now I think it worth the shot.
> >
> > Any feedback?
>
> I find it hard to review for correctness, and extremely susceptible to developer
> error. E.g. lots of copy+paste, and manual encoding of RCX to expose registers.

Yes, I agree. The approach requires careful manual work and is error-prone.
I was planning to get around and stare at every wrapper to make sure it is correct.
This approach is not scalable, though.

> It also bleeds TDX ABI into C code, e.g.
>
> /*
> * As per TDX GHCI CPUID ABI, r12-r15 registers contain contents of
> * EAX, EBX, ECX, EDX registers after the CPUID instruction execution.
> * So copy the register contents back to pt_regs.
> */
> regs->ax = args.r12;
> regs->bx = args.r13;
> regs->cx = args.r14;
> regs->dx = args.r15;
>
> Oh, and it requires input/output paramters, which is quite gross for C code *and*
> for assembly code, e.g.
>
> u64 tdvmcall_map_gpa(u64 *gpa, u64 size);
>
> and then the accompanying assembly code:
>
> FRAME_BEGIN
>
> save_regs r12,r13
>
> movq (%rdi), %r12
> movq %rsi, %r13
>
> movq $(TDX_R10 | TDX_R11 | TDX_R12 | TDX_R13), %rcx
>
> tdvmcall $TDVMCALL_MAP_GPA
>
> movq %r11, (%rdi)
>
> restore_regs r13,r12
>
> FRAME_END
> RET
>
> I think having one trampoline makes sense, e.g. to minimize the probability of
> leaking register state to the VMM. The part that I don't like, and which generates
> awful code, is shoving register state into a memory structure.

I don't think we can get away with single trampoline. We have outliers.

See TDG.VP.VMCALL<ReportFatalError> that uses pretty much all registers as
input. And I hope we wouldn't need TDG.VP.VMCALL<Instruction.PCONFIG> any
time soon. It uses all possible output registers.

But I guess we can make a *few* wrappers that covers all needed cases.

> The annoying part with the TDX ABI is that it heavily uses r8-r15, and asm()
> constraints don't play nice with r8-15. But that doesn't mean we can't use asm()
> with macros, it just means we have to play games with registers.
>
> Because REG-REG moves are super cheap, and ignoring the fatal error goofiness,
> there are at most four inputs. That means having a single trampoline take *all*
> possible inputs is a non-issue. And we can avoiding polluting the inline code if
> we bury the register shuffling in the trampoline.
>
> And if we use asm() wrappers to call the trampoline, then the trampoline doesn't
> need to precisely follow the C calling convention. I.e. the trampoline can return
> with the outputs still in r12-r15, and let the asm() wrappers extract the outputs
> they want.
>
> As it stands, TDVMCALLs either have 0, 1, or 4 outputs. I.e. we only need three
> asm() wrappers. We could get away with one wrapper, but then users of the wrappers
> would need dummy variables for inputs *and* outputs, and the outputs get gross.
>
> Completely untested, but this is what I'm thinking. Side topic, I think making
> "tdcall" a macro that takes a leaf is a mistake. If/when an assembler learns what
> tdcall is, we're going to have to rewrite all of that code. And what a coincidence,
> my suggestion needs a bare TDCALL! :-)

I guess rename the macro (if it still needed) to something like
tdcall_leaf or something.

> Side topic #2, I don't think the trampoline needs a stack frame, its a leaf function.

Hm. I guess.

> Side topic #3, the ud2 to induce panic should be out-of-line.

Yeah. I switched to the inline one while debugging one section mismatch
issue and forgot to switch back.

> Weird? Yeah. But at least we one need to document one weird calling convention,
> and the ugliness is contained to three macros and a small assembly function.

Okay, the approach is worth exploring. I can work on it.

You focuses here on TDVMCALL. What is your take on the rest of TDCALL?

> .pushsection .noinstr.text, "ax"
> SYM_FUNC_START(tdvmcall_trampoline)
> movq $TDX_HYPERCALL_STANDARD, %r10
> movq %rax, %r11
> movq %rdi, %r12
> movq %rsi, %r13
> movq %rdx, %r14
> movq %rcx, %r15
>
> movq $(TDX_R10 | TDX_R11 | TDX_R12 | TDX_R13 | TDX_R14 | TDX_R15), %rcx
>
> tdcall
>
> testq %rax, %rax
> jnz .Lpanic
>
> ret
>
> .Lpanic:
> ud2
> SYM_FUNC_END(tdvmcall_trampoline)
> .popsection
>
>
> #define TDVMCALL(reason, in1, in2, in3, in4) \
> ({ \
> long __ret; \
> \
> asm( \
> "call tdvmcall_trampoline\n\t" \
> "mov %%r10, %0\n\t" \
> : "=r" (__ret) \
> : "D" (in1), "S"(in2), "d"(in3), "c" (in4) \
> : "r12", "r13", "r14", "r15" \
> ); \
> __ret; \
> })
>
> #define TDVMCALL_1(reason, in1, in2, in3, in4, out1) \
> ({ \
> long __ret; \
> \
> asm( \
> "call tdvmcall_trampoline\n\t" \
> "mov %%r10, %0\n\t" \
> "mov %%r12, %1\n\t" \

It is r11, not r12.

> : "=r"(__ret) "=r" (out1) \
> : "a"(reason), "D" (in1), "S"(in2), "d"(in3), "c" (in4) \
> : "r12", "r13", "r14", "r15" \
> ); \
> __ret; \
> })
>
> #define TDVMCALL_4(reason, in1, in2, in3, in4, out1, out2, out3, out4) \
> ({ \
> long __ret; \
> \
> asm( \
> "call tdvmcall_trampoline\n\t" \
> "mov %%r10, %0\n\t" \
> "mov %%r12, %1\n\t" \
> "mov %%r13, %2\n\t" \
> "mov %%r14, %3\n\t" \
> "mov %%r15, %4\n\t" \
> : "=r" (__ret), \
> "=r" (out1), "=r" (out2), "=r" (out3), "=r" (out4) \
> : "a"(reason), "D" (in1), "S"(in2), "d"(in3), "c" (in4) \
> [reason] "i" (reason) \
> : "r12", "r13", "r14", "r15" \
> ); \
> __ret; \
> })
>
> static int handle_halt(struct ve_info *ve)
> {
> if (TDVMCALL(EXIT_REASON_HALT, irqs_disabled(), 0, 0, 0))
> return -EIO;
>
> return ve_instr_len(ve);
> }
>
> void __cpuidle tdx_safe_halt(void)
> {
> WARN_ONCE(TDVMCALL(EXIT_REASON_HALT, false, 0, 0, 0),
> "HLT instruction emulation failed");
> }
>
> static int read_msr(struct pt_regs *regs, struct ve_info *ve)
> {
> u64 val;
>
> if (TDVMCALL_1(EXIT_REASON_MSR_READ, regs->cx, 0, 0, 0, val))
> return -EIO;
>
> regs->ax = lower_32_bits(val);
> regs->dx = upper_32_bits(val);
>
> return ve_instr_len(ve);
> }
>
> static int write_msr(struct pt_regs *regs, struct ve_info *ve)
> {
> u64 val = (u64)regs->dx << 32 | regs->ax;
>
> if (TDVMCALL(EXIT_REASON_MSR_WRITE, regs->cx, val, 0, 0))
> return -EIO;
>
> return ve_instr_len(ve);
> }
> static int handle_cpuid(struct pt_regs *regs, struct ve_info *ve)
> {
> /*
> * Only allow VMM to control range reserved for hypervisor
> * communication.
> *
> * Return all-zeros for any CPUID outside the range. It matches CPU
> * behaviour for non-supported leaf.
> */
> if (regs->ax < 0x40000000 || regs->ax > 0x4FFFFFFF) {
> regs->ax = regs->bx = regs->cx = regs->dx = 0;
> return ve_instr_len(ve);
> }
>
> if (TDVMCALL_4(EXIT_REASON_CPUID, regs->ax, regs->cx, 0, 0,
> regs->ax, regs->bx, regs->cx, regs->dx))
> return -EIO;
>
> return ve_instr_len(ve);
> }
>
> static bool mmio_read(int size, u64 gpa, u64 *val)
> {
> *val = 0;
> return !TDVMCALL_1(EXIT_REASON_EPT_VIOLATION, size, EPT_READ, gpa, 0, val);
> }
>
> static bool mmio_write(int size, u64 gpa, u64 val)
> {
> return !TDVMCALL(EXIT_REASON_EPT_VIOLATION, size, EPT_WRITE, gpa, val);
> }

--
Kiryl Shutsemau / Kirill A. Shutemov