Re: [PATCHv5 03/30] x86/tdx: Add __tdx_module_call() and __tdx_hypercall() helper functions

[Borislav Petkov]

On Fri, Mar 11, 2022 at 12:20:59AM +0300, Kirill A. Shutemov wrote:
> > > A guest uses TDCALL to communicate with both the TDX module and VMM.
> > > The value of the RAX register when executing the TDCALL instruction is
> > > used to determine the TDCALL type. A variant of TDCALL used to communicate
> > > with the VMM is called TDVMCALL.
> > > 
> > > Add generic interfaces to communicate with the TDX module and VMM
> > > (using the TDCALL instruction).
> > > 
> > > __tdx_hypercall()    - Used by the guest to request services from the
> > > 		       VMM (via TDVMCALL).
> > > __tdx_module_call()  - Used to communicate with the TDX module (via
> > > 		       TDCALL).
> > 
> > Ok, you need to fix this: this sounds to me like there are two insns:
> > TDCALL and TDVMCALL. But there's only TDCALL.
> > 
> > And I'm not even clear on how the differentiation is done - I guess
> > with %r11 which contains the VMCALL subfunction number in the
> > __tdx_hypercall() case but I'm not sure.
> 
> TDVMCALL is a leaf of TDCALL. The leaf number is encoded in RAX: RAX==0 is
> TDVMCALL.
> 
> I'm not completely sure what has to be fixed. Make it clear that TDVMCALL
> is leaf of TDCALL? Something like this:
> 
> 	__tdx_module_call()  - Used to communicate with the TDX module (via
> 			       TDCALL instruction).
> 	__tdx_hypercall()    - Used by the guest to request services from the
> 			       VMM (via TDVMCALL leaf of TDCALL).

Yes, it says above "via TDVMCALL" and "A variant of TDCALL used to
communicate with the VMM is called TDVMCALL." and that is ambiguous as
to whether this is about two instructions or one and a modification of
the same.

We write insn mnemonics with all caps so I see "TDCALL" and go, ah ok,
that's the insn but then when I see "TDVMCALL" I don't know what that
is. Another insn? Maybe, it is in all caps too...

> > And when explaining this, pls put it in the comment over the function so
> > that it is clear how the distinction is made.
> 
> But it's already there:

No not that - the explantion you wrote above that TDVMCALL is a leaf of
TDCALL. That needs to be there explicitly so that there's no confusion.

> Okay, will change. But it is not what we agreed about before:
> 
> https://lore.kernel.org/all/Yg5q742GsjCRHXZL@xxxxxxx

I must've been confused from doing so many things at the same time,
sorry about that. :-\

> To me "invalid opcode" at "RIP: 0010:__tdx_hypercall+0x6d/0x70" is pretty
> clear where it comes from, no?

Probably to you and a couple more people who know how to read oops
messages. You have to think about our common users too.

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette