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

[Huang, Kai]

On Fri, 2024-03-15 at 17:48 +0000, Edgecombe, Rick P wrote:
> On Fri, 2024-03-15 at 09:33 -0700, Sean Christopherson wrote:
> > Heh, Like this one?
> > 
> >         static inline u64 tdh_sys_lp_shutdown(void)
> >         {
> >                 struct tdx_module_args in = {
> >                 };
> >         
> >                 return tdx_seamcall(TDH_SYS_LP_SHUTDOWN, &in, NULL);
> >         }
> > 
> > Which isn't actually used...
> 
> Looks like is was turned into a NOP in TDX 1.5. So will even forever be
> dead code. I see one other that is unused. Thanks for pointing it out.
> 
> > 
> > > But I'd also defer to the KVM maintainers on this.  They're the
> > > ones
> > > that have to play the symbol exporting game a lot more than I ever
> > > do.
> > > If they cringe at the idea of adding 20 (or whatever) exports, then
> > > that's a lot more important than the possibility of some other
> > > silly
> > > module abusing the generic exported __seamcall.
> > 
> > I don't care much about exports.  What I do care about is sane code,
> > and while
> > the current code _looks_ pretty, it's actually quite insane.
> > 
> > I get why y'all put SEAMCALL in assembly subroutines; the macro
> > shenanigans I
> > originally wrote years ago were their own brand of crazy, and dealing
> > with GPRs
> > that can't be asm() constraints often results in brittle code.
> 
> I guess it must be this, for the initiated:
> https://lore.kernel.org/lkml/25f0d2c2f73c20309a1b578cc5fc15f4fd6b9a13.1605232743.git.isaku.yamahata@xxxxxxxxx/

Hmm.. I lost memory of this.  :-(

> 
> > 
> > But the tdx_module_args structure approach generates truly atrocious
> > code.  Yes,
> > SEAMCALL is inherently slow, but that doesn't mean that we shouldn't
> > at least try
> > to generate efficient code.  And it's not just efficiency that is
> > lost, the
> > generated code ends up being much harder to read than it ought to be.
> > 
> > 
> [snip]
> > 
> > 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.
> > 
> 
> Thanks for the feedback both! It sounds like everyone is flexible on
> the exports. As for the generated code, oof.
> 
> Kai, I see the solution has gone through some iterations already. First
> the macro one linked above, then that was dropped pretty quick to
> something that loses the asm constraints:
> https://lore.kernel.org/lkml/e777bbbe10b1ec2c37d85dcca2e175fe3bc565ec.1625186503.git.isaku.yamahata@xxxxxxxxx/

Sorry I forgot for what reason we changed from the (bunch of) macros to this.

> 
> Then next the struct grew here, and here:
> https://lore.kernel.org/linux-mm/20230628211132.GS38236@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
> https://lore.kernel.org/linux-mm/20230630102141.GA2534364@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx/
> 
> Not sure I understand all of the constraints yet. Do you have any
> ideas?

This was due to Peter's request to unify the TDCALL/SEAMCALL and TDVMCALL
assembly, which was done by this series:

https://lore.kernel.org/lkml/cover.1692096753.git.kai.huang@xxxxxxxxx/

So when Peter requested this, the __seamcall() and __tdcall() (or
__tdx_module_call() before the above series) were already sharing one assembly
macro.  The TDVMCALL were using a different macro, though.  However the two
assembly macros used similar structure and similar code, so we tried to unify
them.

And another reason that we changed from ...

  u64 __seamcall(u64 fn, u64 rcx, u64 rdx, ..., struct tdx_module_args *out);

to  ...

  u64 __seamcall(u64, struct tdx_module_args *args);

... was the former doesn't extend.  

E.g., live migration related new SEAMCALLs use more registers as input.  It is
just insane to have so many individual regs as function argument.

And Peter wanted to use __seamcall() to cover TDH.VP.ENTER too, which
historically had been implemented in KVM using its own assembly, because
VP.ENTER basically uses all GPRs as input/output.