linux-kernel - Re: [PATCH v19 029/130] KVM: TDX: Add C wrapper functions for SEAMCALLs to the TDX module

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <579cb765-8a5e-4058-bc1d-9de7ac4b95d1@intel.com>
Date: Wed, 20 Mar 2024 13:03:21 +1300
From: "Huang, Kai" <kai.huang@...el.com>
To: <isaku.yamahata@...el.com>, <kvm@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>
CC: <isaku.yamahata@...il.com>, Paolo Bonzini <pbonzini@...hat.com>,
	<erdemaktas@...gle.com>, Sean Christopherson <seanjc@...gle.com>, Sagi Shahar
	<sagis@...gle.com>, <chen.bo@...el.com>, <hang.yuan@...el.com>,
	<tina.zhang@...el.com>, Sean Christopherson
	<sean.j.christopherson@...el.com>, Binbin Wu <binbin.wu@...ux.intel.com>,
	Yuan Yao <yuan.yao@...el.com>
Subject: Re: [PATCH v19 029/130] KVM: TDX: Add C wrapper functions for
 SEAMCALLs to the TDX module



On 26/02/2024 9:25 pm, isaku.yamahata@...el.com wrote:
> From: Isaku Yamahata <isaku.yamahata@...el.com>
> 
> A VMM interacts with the TDX module using a new instruction (SEAMCALL).
> For instance, a TDX VMM does not have full access to the VM control
> structure corresponding to VMX VMCS.  Instead, a VMM induces the TDX module
> to act on behalf via SEAMCALLs.
> 
> Define C wrapper functions for SEAMCALLs for readability.
> 
> Some SEAMCALL APIs donate host pages to TDX module or guest TD, and the
> donated pages are encrypted.  Those require the VMM to flush the cache
> lines to avoid cache line alias.
> 
> Signed-off-by: Sean Christopherson <sean.j.christopherson@...el.com>

Not valid anymore.

[...]

> +
> +static inline u64 tdx_seamcall(u64 op, struct tdx_module_args *in,
> +			       struct tdx_module_args *out)
> +{
> +	u64 ret;
> +
> +	if (out) {
> +		*out = *in;
> +		ret = seamcall_ret(op, out);
> +	} else
> +		ret = seamcall(op, in);

I think it's silly to have the @out argument in this way.

What is the main reason to still have it?

Yeah we used to have the @out in __seamcall() assembly function.  The 
assembly code checks the @out and skips copying registers to @out when 
it is NULL.

But it got removed when we tried to unify the assembly for 
TDCALL/TDVMCALL and SEAMCALL to have a *SINGLE* assembly macro.

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

To me that means we should just accept the fact we will always have a 
valid @out.

But there might be some case that you _obviously_ need the @out and I 
missed?


> +
> +	if (unlikely(ret == TDX_SEAMCALL_UD)) {
> +		/*
> +		 * SEAMCALLs fail with TDX_SEAMCALL_UD returned when VMX is off.
> +		 * This can happen when the host gets rebooted or live
> +		 * updated. In this case, the instruction execution is ignored
> +		 * as KVM is shut down, so the error code is suppressed. Other
> +		 * than this, the error is unexpected and the execution can't
> +		 * continue as the TDX features reply on VMX to be on.
> +		 */
> +		kvm_spurious_fault();
> +		return 0;
> +	}
> +	return ret;
> +}
> +
> +static inline u64 tdh_mng_addcx(hpa_t tdr, hpa_t addr)
> +{
> +	struct tdx_module_args in = {
> +		.rcx = addr,
> +		.rdx = tdr,
> +	};
> +
> +	clflush_cache_range(__va(addr), PAGE_SIZE);
> +	return tdx_seamcall(TDH_MNG_ADDCX, &in, NULL);
> +}
> +
> +static inline u64 tdh_mem_page_add(hpa_t tdr, gpa_t gpa, hpa_t hpa, hpa_t source,
> +				   struct tdx_module_args *out)
> +{
> +	struct tdx_module_args in = {
> +		.rcx = gpa,
> +		.rdx = tdr,
> +		.r8 = hpa,
> +		.r9 = source,
> +	};
> +
> +	clflush_cache_range(__va(hpa), PAGE_SIZE);
> +	return tdx_seamcall(TDH_MEM_PAGE_ADD, &in, out);
> +}
> +
> +static inline u64 tdh_mem_sept_add(hpa_t tdr, gpa_t gpa, int level, hpa_t page,
> +				   struct tdx_module_args *out)
> +{
> +	struct tdx_module_args in = {
> +		.rcx = gpa | level,
> +		.rdx = tdr,
> +		.r8 = page,
> +	};
> +
> +	clflush_cache_range(__va(page), PAGE_SIZE);
> +	return tdx_seamcall(TDH_MEM_SEPT_ADD, &in, out);
> +}
> +
> +static inline u64 tdh_mem_sept_rd(hpa_t tdr, gpa_t gpa, int level,
> +				  struct tdx_module_args *out)
> +{
> +	struct tdx_module_args in = {
> +		.rcx = gpa | level,
> +		.rdx = tdr,
> +	};
> +
> +	return tdx_seamcall(TDH_MEM_SEPT_RD, &in, out);
> +}

Not checked the whole series yet, but is this ever used in this series?

[...]

> +
> +static inline u64 tdh_sys_lp_shutdown(void)
> +{
> +	struct tdx_module_args in = {
> +	};
> +
> +	return tdx_seamcall(TDH_SYS_LP_SHUTDOWN, &in, NULL);
> +}

As Sean already pointed out, I am sure it's/should not used in this series.

That being said, I found it's not easy to determine whether one wrapper 
will be used by this series or not.  The other option is we introduce 
the wrapper(s) when they get actally used, but I can see (especially at 
this stage) it's also a apple vs orange question that people may have 
different preference.

Perhaps we can say something like below in changelog ...

"
Note, not all VM-managing related SEAMCALLs have a wrapper here, but 
only provide wrappers that are essential to the run the TDX guest with 
basic feature set.
"

.. so that people will at least to pay attention to this during the review?