[v19,029/130] KVM: TDX: Add C wrapper functions for SEAMCALLs to the TDX module

Commit Message

Isaku Yamahata Feb. 26, 2024, 8:25 a.m. UTC

From: Isaku Yamahata <[e-mail]>

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 <[e-mail]>
Signed-off-by: Isaku Yamahata <[e-mail]>
Reviewed-by: Binbin Wu <[e-mail]>
Reviewed-by: Yuan Yao <[e-mail]>
---
Changes
v19:
- Update the commit message to match the patch by Yuan
- Use seamcall() and seamcall_ret() by paolo

v18:
- removed stub functions for __seamcall{,_ret}()
- Added Reviewed-by Binbin
- Make tdx_seamcall() use struct tdx_module_args instead of taking
  each inputs.

v15 -> v16:
- use struct tdx_module_args instead of struct tdx_module_output
- Add tdh_mem_sept_rd() for SEPT_VE_DISABLE=1.

Signed-off-by: Isaku Yamahata <[e-mail]>
---
 arch/x86/kvm/vmx/tdx_ops.h | 360 +++++++++++++++++++++++++++++++++++++
 1 file changed, 360 insertions(+)
 create mode 100644 arch/x86/kvm/vmx/tdx_ops.h

Comments

Sean Christopherson March 15, 2024, 5:41 p.m. UTC | #1

On Mon, Feb 26, 2024, [e-mail] wrote:
> +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);
> +
> +	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;

This is nonsensical.  The reason KVM liberally uses BUG_ON(!kvm_rebooting) is
because it *greatly* simpifies the overall code by obviating the need for KVM to
check for errors that should never happen in practice.  On, and 

But KVM quite obviously needs to check the return code for all SEAMCALLs, and
the SEAMCALLs are (a) wrapped in functions and (b) preserve host state, i.e. we
don't need to worry about KVM consuming garbage or running with unknown hardware
state because something like INVVPID or INVEPT faulted.

Oh, and the other critical aspect of all of this is that unlike VMREAD, VMWRITE,
etc., SEAMCALLs almost always require a TDR or TDVPR, i.e. need a VM or vCPU.
Now that we've abandoned the macro shenanigans that allowed things like
tdh_mem_page_add() to be pure translators to their respective SEAMCALL, I don't
see any reason to take the physical addresses of the TDR/TDVPR in the helpers.

I.e.  if we do:

	u64 tdh_mng_addcx(struct kvm *kvm, hpa_t addr)

then the intermediate wrapper to the SEAMCALL assembly has the vCPU or VM and
thus can precisely terminate the one problematic VM.

So unless I'm missing something, I think that kvm_spurious_fault() should be
persona non grata for TDX, and that KVM should instead use KVM_BUG_ON().

Isaku Yamahata March 15, 2024, 7:23 p.m. UTC | #2

On Fri, Mar 15, 2024 at 10:41:28AM -0700,
Sean Christopherson <[e-mail]> wrote:

> On Mon, Feb 26, 2024, [e-mail] wrote:
> > +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);
> > +
> > +	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;
> 
> This is nonsensical.  The reason KVM liberally uses BUG_ON(!kvm_rebooting) is
> because it *greatly* simpifies the overall code by obviating the need for KVM to
> check for errors that should never happen in practice.  On, and 
> 
> But KVM quite obviously needs to check the return code for all SEAMCALLs, and
> the SEAMCALLs are (a) wrapped in functions and (b) preserve host state, i.e. we
> don't need to worry about KVM consuming garbage or running with unknown hardware
> state because something like INVVPID or INVEPT faulted.
> 
> Oh, and the other critical aspect of all of this is that unlike VMREAD, VMWRITE,
> etc., SEAMCALLs almost always require a TDR or TDVPR, i.e. need a VM or vCPU.
> Now that we've abandoned the macro shenanigans that allowed things like
> tdh_mem_page_add() to be pure translators to their respective SEAMCALL, I don't
> see any reason to take the physical addresses of the TDR/TDVPR in the helpers.
> 
> I.e.  if we do:
> 
> 	u64 tdh_mng_addcx(struct kvm *kvm, hpa_t addr)
> 
> then the intermediate wrapper to the SEAMCALL assembly has the vCPU or VM and
> thus can precisely terminate the one problematic VM.
> 
> So unless I'm missing something, I think that kvm_spurious_fault() should be
> persona non grata for TDX, and that KVM should instead use KVM_BUG_ON().

Thank you for the feedback.  As I don't see any issues to do so, I'll convert
those wrappers to take struct kvm_tdx or struct vcpu_tdx, and eliminate
kvm_spurious_fault() in favor of KVM_BUG_ON().

Edgecombe, Rick P March 19, 2024, 11:24 p.m. UTC | #3

On Mon, 2024-02-26 at 00:25 -0800, [e-mail] wrote:
> +
> +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);
> +}

The caller of this later in the series looks like this:

	err = tdh_mem_sept_add(kvm_tdx, gpa, tdx_level, hpa, &out);
	if (unlikely(err == TDX_ERROR_SEPT_BUSY))
		return -EAGAIN;
	if (unlikely(err == (TDX_EPT_ENTRY_STATE_INCORRECT |
TDX_OPERAND_ID_RCX))) {
		union tdx_sept_entry entry = {
			.raw = out.rcx,
		};
		union tdx_sept_level_state level_state = {
			.raw = out.rdx,
		};

		/* someone updated the entry with same value. */
		if (level_state.level == tdx_level &&
		    level_state.state == TDX_SEPT_PRESENT &&
		    !entry.leaf && entry.pfn == (hpa >> PAGE_SHIFT))
			return -EAGAIN;
	}

The helper abstracts setting the arguments into the proper registers
fields passed in, but doesn't abstract pulling the result out from the
register fields. Then the caller has to manually extract them in this
verbose way. Why not have the helper do both?

Huang, Kai March 20, 2024, 12:03 a.m. UTC | #4

On 26/02/2024 9:25 pm, [e-mail] wrote:
> From: Isaku Yamahata <[e-mail]>
> 
> 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 <[e-mail]>

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/[e-mail]/

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?

Isaku Yamahata March 20, 2024, 12:09 a.m. UTC | #5

On Tue, Mar 19, 2024 at 11:24:37PM +0000,
"Edgecombe, Rick P" <[e-mail]> wrote:

> On Mon, 2024-02-26 at 00:25 -0800, [e-mail] wrote:
> > +
> > +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);
> > +}
> 
> The caller of this later in the series looks like this:
> 
> 	err = tdh_mem_sept_add(kvm_tdx, gpa, tdx_level, hpa, &out);
> 	if (unlikely(err == TDX_ERROR_SEPT_BUSY))
> 		return -EAGAIN;
> 	if (unlikely(err == (TDX_EPT_ENTRY_STATE_INCORRECT |
> TDX_OPERAND_ID_RCX))) {
> 		union tdx_sept_entry entry = {
> 			.raw = out.rcx,
> 		};
> 		union tdx_sept_level_state level_state = {
> 			.raw = out.rdx,
> 		};
> 
> 		/* someone updated the entry with same value. */
> 		if (level_state.level == tdx_level &&
> 		    level_state.state == TDX_SEPT_PRESENT &&
> 		    !entry.leaf && entry.pfn == (hpa >> PAGE_SHIFT))
> 			return -EAGAIN;
> 	}
> 
> The helper abstracts setting the arguments into the proper registers
> fields passed in, but doesn't abstract pulling the result out from the
> register fields. Then the caller has to manually extract them in this
> verbose way. Why not have the helper do both?

Yes. Let me update those arguments.

Edgecombe, Rick P March 20, 2024, 12:11 a.m. UTC | #6

On Tue, 2024-03-19 at 17:09 -0700, Isaku Yamahata wrote:
> > The helper abstracts setting the arguments into the proper
> > registers
> > fields passed in, but doesn't abstract pulling the result out from
> > the
> > register fields. Then the caller has to manually extract them in
> > this
> > verbose way. Why not have the helper do both?
> 
> Yes. Let me update those arguments.

What were you thinking exactly, like?

tdh_mem_sept_add(kvm_tdx, gpa, tdx_level, hpa, &entry, &level_state);

And for the other helpers?

Isaku Yamahata March 20, 2024, 5:41 a.m. UTC | #7

On Wed, Mar 20, 2024 at 12:11:17AM +0000,
"Edgecombe, Rick P" <[e-mail]> wrote:

> On Tue, 2024-03-19 at 17:09 -0700, Isaku Yamahata wrote:
> > > The helper abstracts setting the arguments into the proper
> > > registers
> > > fields passed in, but doesn't abstract pulling the result out from
> > > the
> > > register fields. Then the caller has to manually extract them in
> > > this
> > > verbose way. Why not have the helper do both?
> > 
> > Yes. Let me update those arguments.
> 
> What were you thinking exactly, like?
> 
> tdh_mem_sept_add(kvm_tdx, gpa, tdx_level, hpa, &entry, &level_state);
> 
> And for the other helpers?

I have the following four helpers.  Other helpers will have no out argument.

tdh_mem_sept_add(kvm_tdx, gpa, tdx_level, hpa, &entry, &level_state);
tdh_mem_page_aug(kvm_tdx, gpa, hpa, &entry, &level_state);
tdh_mem_page_remove(kvm_tdx, gpa, tdx_level, &entry, &level_state);
tdh_mem_range_block(kvm_tdx, gpa, tdx_level, &entry, &level_state);

Isaku Yamahata March 20, 2024, 8:20 p.m. UTC | #8

On Tue, Mar 19, 2024 at 10:41:09PM -0700,
Isaku Yamahata <[e-mail]> wrote:

> On Wed, Mar 20, 2024 at 12:11:17AM +0000,
> "Edgecombe, Rick P" <[e-mail]> wrote:
> 
> > On Tue, 2024-03-19 at 17:09 -0700, Isaku Yamahata wrote:
> > > > The helper abstracts setting the arguments into the proper
> > > > registers
> > > > fields passed in, but doesn't abstract pulling the result out from
> > > > the
> > > > register fields. Then the caller has to manually extract them in
> > > > this
> > > > verbose way. Why not have the helper do both?
> > > 
> > > Yes. Let me update those arguments.
> > 
> > What were you thinking exactly, like?
> > 
> > tdh_mem_sept_add(kvm_tdx, gpa, tdx_level, hpa, &entry, &level_state);
> > 
> > And for the other helpers?
> 
> I have the following four helpers.  Other helpers will have no out argument.
> 
> tdh_mem_sept_add(kvm_tdx, gpa, tdx_level, hpa, &entry, &level_state);
> tdh_mem_page_aug(kvm_tdx, gpa, hpa, &entry, &level_state);
> tdh_mem_page_remove(kvm_tdx, gpa, tdx_level, &entry, &level_state);
> tdh_mem_range_block(kvm_tdx, gpa, tdx_level, &entry, &level_state);

By updating the code, I found that tdh_mem_range_block() doesn't need out
variables. and tdh_vp_rd() needs output.
tdh_mem_range_block() doesn't need the out.

u64 tdh_vp_rd(struct vcpu_tdx *tdx, u64 field, u64 *value)

Isaku Yamahata March 20, 2024, 9:36 p.m. UTC | #9

On Wed, Mar 20, 2024 at 01:03:21PM +1300,
"Huang, Kai" <[e-mail]> wrote:

> > +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/[e-mail]/
> 
> 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?

As I replied at [1], those four wrappers need to return values.
The first three on error, the last one on success.

  [1] https://lore.kernel.org/kvm/[e-mail]/

  tdh_mem_sept_add(kvm_tdx, gpa, tdx_level, hpa, &entry, &level_state);
  tdh_mem_page_aug(kvm_tdx, gpa, hpa, &entry, &level_state);
  tdh_mem_page_remove(kvm_tdx, gpa, tdx_level, &entry, &level_state);
  u64 tdh_vp_rd(struct vcpu_tdx *tdx, u64 field, u64 *value)

We can delete out from other wrappers.
Because only TDH.MNG.CREATE() and TDH.MNG.ADDCX() can return TDX_RND_NO_ENTROPY,
we can use __seamcall().  The TDX spec doesn't guarantee such error code
convention.  It's very unlikely, though.


> > +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?

Makes sense. We can split this patch into other patches that first use the
wrappers.

Huang, Kai March 20, 2024, 10:37 p.m. UTC | #10

On 21/03/2024 10:36 am, Isaku Yamahata wrote:
> On Wed, Mar 20, 2024 at 01:03:21PM +1300,
> "Huang, Kai" <[e-mail]> wrote:
> 
>>> +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/[e-mail]/
>>
>> 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?
> 
> As I replied at [1], those four wrappers need to return values.
> The first three on error, the last one on success.
> 
>    [1] https://lore.kernel.org/kvm/[e-mail]/
> 
>    tdh_mem_sept_add(kvm_tdx, gpa, tdx_level, hpa, &entry, &level_state);
>    tdh_mem_page_aug(kvm_tdx, gpa, hpa, &entry, &level_state);
>    tdh_mem_page_remove(kvm_tdx, gpa, tdx_level, &entry, &level_state);
>    u64 tdh_vp_rd(struct vcpu_tdx *tdx, u64 field, u64 *value)
> 
> We can delete out from other wrappers.

Ah, OK.  I got you don't want to invent separate wrappers for each 
seamcall() variants like:

  - tdx_seamcall(u64 fn, struct tdx_module_args *args);
  - tdx_seamcall_ret(u64 fn, struct tdx_module_args *args);
  - tdx_seamcall_saved_ret(u64 fn, struct tdx_module_args *args);

To be honest I found they were kinda annoying myself during the "unify 
TDCALL/SEAMCALL and TDVMCALL assembly" patchset.

But life is hard...

And given (it seems) we are going to remove kvm_spurious_fault(), I 
think the tdx_seamcall() variants are just very simple wrapper of plain 
seamcall() variants.

So how about we have some macros:

static inline bool is_seamcall_err_kernel_defined(u64 err)
{
	return err & TDX_SW_ERROR;
}

#define TDX_KVM_SEAMCALL(_kvm, _seamcall_func, _fn, _args)	\
	({				\
		u64 _ret = _seamcall_func(_fn, _args);
		KVM_BUG_ON(_kvm, is_seamcall_err_kernel_defined(_ret));
		_ret;
	})

#define tdx_kvm_seamcall(_kvm, _fn, _args)	\
	TDX_KVM_SEAMCALL(_kvm, seamcall, _fn, _args)

#define tdx_kvm_seamcall_ret(_kvm, _fn, _args)	\
	TDX_KVM_SEAMCALL(_kvm, seamcall_ret, _fn, _args)

#define tdx_kvm_seamcall_saved_ret(_kvm, _fn, _args)	\
	TDX_KVM_SEAMCALL(_kvm, seamcall_saved_ret, _fn, _args)

This is consistent with what we have in TDX host code, and this handles 
NO_ENTROPY error internally.

Or, maybe we can just use the seamcall_ret() for ALL SEAMCALLs, except 
using seamcall_saved_ret() for TDH.VP.ENTER.

u64 tdx_kvm_seamcall(sruct kvm*kvm, u64 fn,
			struct tdx_module_args *args)
{
	u64 ret = seamcall_ret(fn, args);

	KVM_BUG_ON(kvm, is_seamcall_err_kernel_defined(ret);

	return ret;
}

IIUC this at least should give us a single tdx_kvm_seamcall() API for 
majority (99%) code sites?

And obviously I'd like other people to weigh in too.

> Because only TDH.MNG.CREATE() and TDH.MNG.ADDCX() can return TDX_RND_NO_ENTROPY, > we can use __seamcall().  The TDX spec doesn't guarantee such error code
> convention.  It's very unlikely, though.

I don't quite follow the "convention" part.  Can you elaborate?

NO_ENTROPY is already handled in seamcall() variants.  Can we just use 
them directly?

> 
> 
>>> +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?
> 
> Makes sense. We can split this patch into other patches that first use the
> wrappers.

Obviously I didn't want to make you do dramatic patchset reorganization, 
so it's up to you.

Isaku Yamahata March 22, 2024, 12:16 a.m. UTC | #11

On Thu, Mar 21, 2024 at 11:37:58AM +1300,
"Huang, Kai" <[e-mail]> wrote:

> 
> 
> On 21/03/2024 10:36 am, Isaku Yamahata wrote:
> > On Wed, Mar 20, 2024 at 01:03:21PM +1300,
> > "Huang, Kai" <[e-mail]> wrote:
> > 
> > > > +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/[e-mail]/
> > > 
> > > 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?
> > 
> > As I replied at [1], those four wrappers need to return values.
> > The first three on error, the last one on success.
> > 
> >    [1] https://lore.kernel.org/kvm/[e-mail]/
> > 
> >    tdh_mem_sept_add(kvm_tdx, gpa, tdx_level, hpa, &entry, &level_state);
> >    tdh_mem_page_aug(kvm_tdx, gpa, hpa, &entry, &level_state);
> >    tdh_mem_page_remove(kvm_tdx, gpa, tdx_level, &entry, &level_state);
> >    u64 tdh_vp_rd(struct vcpu_tdx *tdx, u64 field, u64 *value)
> > 
> > We can delete out from other wrappers.
> 
> Ah, OK.  I got you don't want to invent separate wrappers for each
> seamcall() variants like:
> 
>  - tdx_seamcall(u64 fn, struct tdx_module_args *args);
>  - tdx_seamcall_ret(u64 fn, struct tdx_module_args *args);
>  - tdx_seamcall_saved_ret(u64 fn, struct tdx_module_args *args);
> 
> To be honest I found they were kinda annoying myself during the "unify
> TDCALL/SEAMCALL and TDVMCALL assembly" patchset.
> 
> But life is hard...
> 
> And given (it seems) we are going to remove kvm_spurious_fault(), I think
> the tdx_seamcall() variants are just very simple wrapper of plain seamcall()
> variants.
> 
> So how about we have some macros:
> 
> static inline bool is_seamcall_err_kernel_defined(u64 err)
> {
> 	return err & TDX_SW_ERROR;
> }
> 
> #define TDX_KVM_SEAMCALL(_kvm, _seamcall_func, _fn, _args)	\
> 	({				\
> 		u64 _ret = _seamcall_func(_fn, _args);
> 		KVM_BUG_ON(_kvm, is_seamcall_err_kernel_defined(_ret));
> 		_ret;
> 	})

As we can move out KVM_BUG_ON() to the call site, we can simply have
seamcall() or seamcall_ret().
The call site has to check error. whether it is TDX_SW_ERROR or not.
And if it hit the unexpected error, it will mark the guest bugged.


> #define tdx_kvm_seamcall(_kvm, _fn, _args)	\
> 	TDX_KVM_SEAMCALL(_kvm, seamcall, _fn, _args)
> 
> #define tdx_kvm_seamcall_ret(_kvm, _fn, _args)	\
> 	TDX_KVM_SEAMCALL(_kvm, seamcall_ret, _fn, _args)
> 
> #define tdx_kvm_seamcall_saved_ret(_kvm, _fn, _args)	\
> 	TDX_KVM_SEAMCALL(_kvm, seamcall_saved_ret, _fn, _args)
> 
> This is consistent with what we have in TDX host code, and this handles
> NO_ENTROPY error internally.
> 
> Or, maybe we can just use the seamcall_ret() for ALL SEAMCALLs, except using
> seamcall_saved_ret() for TDH.VP.ENTER.
> 
> u64 tdx_kvm_seamcall(sruct kvm*kvm, u64 fn,
> 			struct tdx_module_args *args)
> {
> 	u64 ret = seamcall_ret(fn, args);
> 
> 	KVM_BUG_ON(kvm, is_seamcall_err_kernel_defined(ret);
> 
> 	return ret;
> }
> 
> IIUC this at least should give us a single tdx_kvm_seamcall() API for
> majority (99%) code sites?

We can eleiminate tdx_kvm_seamcall() and use seamcall() or seamcall_ret()
directly.


> And obviously I'd like other people to weigh in too.
> 
> > Because only TDH.MNG.CREATE() and TDH.MNG.ADDCX() can return TDX_RND_NO_ENTROPY, > we can use __seamcall().  The TDX spec doesn't guarantee such error code
> > convention.  It's very unlikely, though.
> 
> I don't quite follow the "convention" part.  Can you elaborate?
> 
> NO_ENTROPY is already handled in seamcall() variants.  Can we just use them
> directly?

I intended for bad code generation.  If the loop on NO_ENTRY error harms the
code generation, we might be able to use __seamcall() or __seamcall_ret()
instead of seamcall(), seamcall_ret().

Huang, Kai March 22, 2024, 4:33 a.m. UTC | #12

> > 
> > So how about we have some macros:
> > 
> > static inline bool is_seamcall_err_kernel_defined(u64 err)
> > {
> > 	return err & TDX_SW_ERROR;
> > }
> > 
> > #define TDX_KVM_SEAMCALL(_kvm, _seamcall_func, _fn, _args)	\
> > 	({				\
> > 		u64 _ret = _seamcall_func(_fn, _args);
> > 		KVM_BUG_ON(_kvm, is_seamcall_err_kernel_defined(_ret));
> > 		_ret;
> > 	})
> 
> As we can move out KVM_BUG_ON() to the call site, we can simply have
> seamcall() or seamcall_ret().
> The call site has to check error. whether it is TDX_SW_ERROR or not.
> And if it hit the unexpected error, it will mark the guest bugged.

How many call sites are we talking about?

I think handling KVM_BUG_ON() in macro should be able to eliminate bunch of
individual KVM_BUG_ON()s in these call sites?

> 
> 
> > #define tdx_kvm_seamcall(_kvm, _fn, _args)	\
> > 	TDX_KVM_SEAMCALL(_kvm, seamcall, _fn, _args)
> > 
> > #define tdx_kvm_seamcall_ret(_kvm, _fn, _args)	\
> > 	TDX_KVM_SEAMCALL(_kvm, seamcall_ret, _fn, _args)
> > 
> > #define tdx_kvm_seamcall_saved_ret(_kvm, _fn, _args)	\
> > 	TDX_KVM_SEAMCALL(_kvm, seamcall_saved_ret, _fn, _args)
> > 
> > This is consistent with what we have in TDX host code, and this handles
> > NO_ENTROPY error internally.
> > 
> > 

[...]

> > 
> > > Because only TDH.MNG.CREATE() and TDH.MNG.ADDCX() can return TDX_RND_NO_ENTROPY, > we can use __seamcall().  The TDX spec doesn't guarantee such error code
> > > convention.  It's very unlikely, though.
> > 
> > I don't quite follow the "convention" part.  Can you elaborate?
> > 
> > NO_ENTROPY is already handled in seamcall() variants.  Can we just use them
> > directly?
> 
> I intended for bad code generation.  If the loop on NO_ENTRY error harms the
> code generation, we might be able to use __seamcall() or __seamcall_ret()
> instead of seamcall(), seamcall_ret().

This doesn't make sense to me.

Firstly, you have to *prove* the loop generates worse code.

Secondly, if it does generate worse code, and we care about it, we should fix it
in the host seamcall() code.  No?

Isaku Yamahata March 22, 2024, 11:26 p.m. UTC | #13

On Fri, Mar 22, 2024 at 04:33:21AM +0000,
"Huang, Kai" <[e-mail]> wrote:

> > > 
> > > So how about we have some macros:
> > > 
> > > static inline bool is_seamcall_err_kernel_defined(u64 err)
> > > {
> > > 	return err & TDX_SW_ERROR;
> > > }
> > > 
> > > #define TDX_KVM_SEAMCALL(_kvm, _seamcall_func, _fn, _args)	\
> > > 	({				\
> > > 		u64 _ret = _seamcall_func(_fn, _args);
> > > 		KVM_BUG_ON(_kvm, is_seamcall_err_kernel_defined(_ret));
> > > 		_ret;
> > > 	})
> > 
> > As we can move out KVM_BUG_ON() to the call site, we can simply have
> > seamcall() or seamcall_ret().
> > The call site has to check error. whether it is TDX_SW_ERROR or not.
> > And if it hit the unexpected error, it will mark the guest bugged.
> 
> How many call sites are we talking about?
> 
> I think handling KVM_BUG_ON() in macro should be able to eliminate bunch of
> individual KVM_BUG_ON()s in these call sites?

16: custom error check is needed
6: always error

So I'd like to consistently have error check in KVM code, not in macro or
wrapper.

Huang, Kai April 24, 2024, 10:50 a.m. UTC | #14

On Mon, 2024-02-26 at 00:25 -0800, [e-mail] wrote:
> +#include <linux/compiler.h>
> +
> +#include <asm/cacheflush.h>
> +#include <asm/asm.h>
> +#include <asm/kvm_host.h>

None of above are needed to build this file, except the
<asm/cacheflush.h>.

I am removing them.

I am also adding <asm/tdx.h> because this file uses 'struct
tdx_module_args'.  And <asm/page.h> for __va().
 
> +
> +#include "tdx_errno.h"
> +#include "tdx_arch.h"

I am moving them to "tdx.h", and make "tdx.h" to include this "tdx_ops.h"
as well, and declare the C file should never include "tdx_ops.h" directly:

https://lore.kernel.org/lkml/[e-mail]/

Patch

diff --git a/arch/x86/kvm/vmx/tdx_ops.h b/arch/x86/kvm/vmx/tdx_ops.h
new file mode 100644
index 000000000000..c5bb165b260e
--- /dev/null
+++ b/arch/x86/kvm/vmx/tdx_ops.h
@@ -0,0 +1,360 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/* constants/data definitions for TDX SEAMCALLs */
+
+#ifndef __KVM_X86_TDX_OPS_H
+#define __KVM_X86_TDX_OPS_H
+
+#include <linux/compiler.h>
+
+#include <asm/cacheflush.h>
+#include <asm/asm.h>
+#include <asm/kvm_host.h>
+
+#include "tdx_errno.h"
+#include "tdx_arch.h"
+#include "x86.h"
+
+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);
+
+	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);
+}
+
+static inline u64 tdh_mem_sept_remove(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_REMOVE, &in, out);
+}
+
+static inline u64 tdh_vp_addcx(hpa_t tdvpr, hpa_t addr)
+{
+	struct tdx_module_args in = {
+		.rcx = addr,
+		.rdx = tdvpr,
+	};
+
+	clflush_cache_range(__va(addr), PAGE_SIZE);
+	return tdx_seamcall(TDH_VP_ADDCX, &in, NULL);
+}
+
+static inline u64 tdh_mem_page_relocate(hpa_t tdr, gpa_t gpa, hpa_t hpa,
+					struct tdx_module_args *out)
+{
+	struct tdx_module_args in = {
+		.rcx = gpa,
+		.rdx = tdr,
+		.r8 = hpa,
+	};
+
+	clflush_cache_range(__va(hpa), PAGE_SIZE);
+	return tdx_seamcall(TDH_MEM_PAGE_RELOCATE, &in, out);
+}
+
+static inline u64 tdh_mem_page_aug(hpa_t tdr, gpa_t gpa, hpa_t hpa,
+				   struct tdx_module_args *out)
+{
+	struct tdx_module_args in = {
+		.rcx = gpa,
+		.rdx = tdr,
+		.r8 = hpa,
+	};
+
+	clflush_cache_range(__va(hpa), PAGE_SIZE);
+	return tdx_seamcall(TDH_MEM_PAGE_AUG, &in, out);
+}
+
+static inline u64 tdh_mem_range_block(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_RANGE_BLOCK, &in, out);
+}
+
+static inline u64 tdh_mng_key_config(hpa_t tdr)
+{
+	struct tdx_module_args in = {
+		.rcx = tdr,
+	};
+
+	return tdx_seamcall(TDH_MNG_KEY_CONFIG, &in, NULL);
+}
+
+static inline u64 tdh_mng_create(hpa_t tdr, int hkid)
+{
+	struct tdx_module_args in = {
+		.rcx = tdr,
+		.rdx = hkid,
+	};
+
+	clflush_cache_range(__va(tdr), PAGE_SIZE);
+	return tdx_seamcall(TDH_MNG_CREATE, &in, NULL);
+}
+
+static inline u64 tdh_vp_create(hpa_t tdr, hpa_t tdvpr)
+{
+	struct tdx_module_args in = {
+		.rcx = tdvpr,
+		.rdx = tdr,
+	};
+
+	clflush_cache_range(__va(tdvpr), PAGE_SIZE);
+	return tdx_seamcall(TDH_VP_CREATE, &in, NULL);
+}
+
+static inline u64 tdh_mng_rd(hpa_t tdr, u64 field, struct tdx_module_args *out)
+{
+	struct tdx_module_args in = {
+		.rcx = tdr,
+		.rdx = field,
+	};
+
+	return tdx_seamcall(TDH_MNG_RD, &in, out);
+}
+
+static inline u64 tdh_mr_extend(hpa_t tdr, gpa_t gpa,
+				struct tdx_module_args *out)
+{
+	struct tdx_module_args in = {
+		.rcx = gpa,
+		.rdx = tdr,
+	};
+
+	return tdx_seamcall(TDH_MR_EXTEND, &in, out);
+}
+
+static inline u64 tdh_mr_finalize(hpa_t tdr)
+{
+	struct tdx_module_args in = {
+		.rcx = tdr,
+	};
+
+	return tdx_seamcall(TDH_MR_FINALIZE, &in, NULL);
+}
+
+static inline u64 tdh_vp_flush(hpa_t tdvpr)
+{
+	struct tdx_module_args in = {
+		.rcx = tdvpr,
+	};
+
+	return tdx_seamcall(TDH_VP_FLUSH, &in, NULL);
+}
+
+static inline u64 tdh_mng_vpflushdone(hpa_t tdr)
+{
+	struct tdx_module_args in = {
+		.rcx = tdr,
+	};
+
+	return tdx_seamcall(TDH_MNG_VPFLUSHDONE, &in, NULL);
+}
+
+static inline u64 tdh_mng_key_freeid(hpa_t tdr)
+{
+	struct tdx_module_args in = {
+		.rcx = tdr,
+	};
+
+	return tdx_seamcall(TDH_MNG_KEY_FREEID, &in, NULL);
+}
+
+static inline u64 tdh_mng_init(hpa_t tdr, hpa_t td_params,
+			       struct tdx_module_args *out)
+{
+	struct tdx_module_args in = {
+		.rcx = tdr,
+		.rdx = td_params,
+	};
+
+	return tdx_seamcall(TDH_MNG_INIT, &in, out);
+}
+
+static inline u64 tdh_vp_init(hpa_t tdvpr, u64 rcx)
+{
+	struct tdx_module_args in = {
+		.rcx = tdvpr,
+		.rdx = rcx,
+	};
+
+	return tdx_seamcall(TDH_VP_INIT, &in, NULL);
+}
+
+static inline u64 tdh_vp_rd(hpa_t tdvpr, u64 field,
+			    struct tdx_module_args *out)
+{
+	struct tdx_module_args in = {
+		.rcx = tdvpr,
+		.rdx = field,
+	};
+
+	return tdx_seamcall(TDH_VP_RD, &in, out);
+}
+
+static inline u64 tdh_mng_key_reclaimid(hpa_t tdr)
+{
+	struct tdx_module_args in = {
+		.rcx = tdr,
+	};
+
+	return tdx_seamcall(TDH_MNG_KEY_RECLAIMID, &in, NULL);
+}
+
+static inline u64 tdh_phymem_page_reclaim(hpa_t page,
+					  struct tdx_module_args *out)
+{
+	struct tdx_module_args in = {
+		.rcx = page,
+	};
+
+	return tdx_seamcall(TDH_PHYMEM_PAGE_RECLAIM, &in, out);
+}
+
+static inline u64 tdh_mem_page_remove(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_PAGE_REMOVE, &in, out);
+}
+
+static inline u64 tdh_sys_lp_shutdown(void)
+{
+	struct tdx_module_args in = {
+	};
+
+	return tdx_seamcall(TDH_SYS_LP_SHUTDOWN, &in, NULL);
+}
+
+static inline u64 tdh_mem_track(hpa_t tdr)
+{
+	struct tdx_module_args in = {
+		.rcx = tdr,
+	};
+
+	return tdx_seamcall(TDH_MEM_TRACK, &in, NULL);
+}
+
+static inline u64 tdh_mem_range_unblock(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_RANGE_UNBLOCK, &in, out);
+}
+
+static inline u64 tdh_phymem_cache_wb(bool resume)
+{
+	struct tdx_module_args in = {
+		.rcx = resume ? 1 : 0,
+	};
+
+	return tdx_seamcall(TDH_PHYMEM_CACHE_WB, &in, NULL);
+}
+
+static inline u64 tdh_phymem_page_wbinvd(hpa_t page)
+{
+	struct tdx_module_args in = {
+		.rcx = page,
+	};
+
+	return tdx_seamcall(TDH_PHYMEM_PAGE_WBINVD, &in, NULL);
+}
+
+static inline u64 tdh_vp_wr(hpa_t tdvpr, u64 field, u64 val, u64 mask,
+			    struct tdx_module_args *out)
+{
+	struct tdx_module_args in = {
+		.rcx = tdvpr,
+		.rdx = field,
+		.r8 = val,
+		.r9 = mask,
+	};
+
+	return tdx_seamcall(TDH_VP_WR, &in, out);
+}
+
+#endif /* __KVM_X86_TDX_OPS_H */