Re: [PATCH v8 04/11] x86/tdx: Add Intel ARCH support to cc_platform_has()

["Kuppuswamy, Sathyanarayanan" <sathyanarayanan.kuppuswamy@linux.intel.com>]

On 10/6/21 11:02 AM, Borislav Petkov wrote:
> On Tue, Oct 05, 2021 at 02:16:11PM -0700, Josh Poimboeuf wrote:
>> I assume this needs a rebase on -tip since cc_platform.c already has an
>> empty version of this function (and it's static so it doesn't need to be
>> declared in a header).
> 
> Yes:
> 
> arch/x86/kernel/cc_platform.c:16:28: error: static declaration of ‘intel_cc_platform_has’ follows non-static declaration
>     16 | static bool __maybe_unused intel_cc_platform_has(enum cc_attr attr)
>        |                            ^~~~~~~~~~~~~~~~~~~~~
> In file included from ./include/linux/mem_encrypt.h:17,
>                   from arch/x86/kernel/cc_platform.c:12:
> ./arch/x86/include/asm/mem_encrypt.h:105:6: note: previous declaration of ‘intel_cc_platform_has’ was here
>    105 | bool intel_cc_platform_has(enum cc_attr attr);
>        |      ^~~~~~~~~~~~~~~~~~~~~
> make[2]: *** [scripts/Makefile.build:277: arch/x86/kernel/cc_platform.o] Error 1
> make[1]: *** [scripts/Makefile.build:540: arch/x86/kernel] Error 2
> make: *** [Makefile:1868: arch/x86] Error 2
> make: *** Waiting for unfinished jobs....
> 
> I had already started that function there - please add all TDX logic in
> cc_platform.c.
> 
> When you do your next set, you can use tip/master as a base. This should
> be used for all x86 patchsets anyway.

Yes. I have already rebased my patches on top of tip branch. Next version
will not have this issue.


> 
>>> +	/**
>>> +	 * @CC_ATTR_GUEST_TDX: Trusted Domain Extension Support
>>> +	 *
>>> +	 * The platform/OS is running as a TDX guest/virtual machine.
>>> +	 *
>>> +	 * Examples include Intel TDX.
>>> +	 */
>>> +	CC_ATTR_GUEST_TDX,
>>
>> Examples of TDX include TDX? :-)
> 
> Yeah, so whether we should be naming the actual conf. computing
> implementation came up during the cc_platform_has() review and looking
> forward in this patchset:
> 
> +       if (cc_platform_has(CC_ATTR_GUEST_TDX))
> +               return tdx_kvm_hypercall(nr, 0, 0, 0, 0);
> 
> you really need to test for TDX because you're doing a TDX-specific
> hypercall.
> 
> Which brings me back to the fastpath use of is_idx_guest(): this
> looks to me like a fastpath use - dunno how often one needs to do TDX
> hypercalls so I can imagine that for this, the is_tdx_guest() check
> should use a static branch.
> 
> But only with numbers to show the need for it.

Compared to TDX hypercall, additional function call should not take much
time. IMO, we don't need fast path for hypercalls.

Sean/Andi - Any comments?

> 
> Thx.
> 

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer