Re: [PATCH v9 05/18] x86/virt/tdx: Add SEAMCALL infrastructure

Dave Hansen
2023-02-13T17:48:46Z

On 2/13/23 03:59, Kai Huang wrote:
> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
> index 4a3ee64c1ca7..5c5ecfddb15b 100644
> --- a/arch/x86/include/asm/tdx.h
> +++ b/arch/x86/include/asm/tdx.h
> @@ -8,6 +8,10 @@
> #include <asm/ptrace.h>
> #include <asm/shared/tdx.h>
>
> +#ifdef CONFIG_INTEL_TDX_HOST
...
> +#define TDX_SEAMCALL_GP (TDX_SW_ERROR | X86_TRAP_GP)
> +#define TDX_SEAMCALL_UD (TDX_SW_ERROR | X86_TRAP_UD)
> +
> +#endif

All these kinds of header #ifdefs do it make it harder to write code in
.c files without matching #ifdefs. Think of code like this completely
made up example:

if (!tdx_enable()) {
// Success! Make a seamcall:
int something = tdx_seamcall();
if (something == TDX_SEAMCALL_UD)
// oh no!
}

tdx_enable() can never return 0 if CONFIG_INTEL_TDX_HOST=n, so the
entire if() block is optimized away by the compiler. *BUT*, if you've
#ifdef'd away TDX_SEAMCALL_UD, you'll get a compile error. People
usually fix the compile error like this:

if (!tdx_enable()) {
#ifdef CONFIG_INTEL_TDX_HOST
// Success! Make a seamcall:
int something = tdx_seamcall();
if (something == TDX_SEAMCALL_UD)
// oh no!
#endif
}

Which isn't great.

Defining things unconditionally in header files is *FINE*, as long as
the #ifdefs are there somewhere to make the code go away at compile time.

Please post an updated (and tested) patch as a reply to this.