Re: [PATCH v3 02/24] x86/virt/tdx: Add SEAMCALL wrapper tdh_mem_page_demote()

["Huang, Kai" <kai.huang@intel.com>]

> 
> Enable tdh_mem_page_demote() only on TDX modules that support feature
> TDX_FEATURES0.ENHANCE_DEMOTE_INTERRUPTIBILITY, which does not return error
> TDX_INTERRUPTED_RESTARTABLE on basic TDX (i.e., without TD partition) [2].
> 
> This is because error TDX_INTERRUPTED_RESTARTABLE is difficult to handle.
> The TDX module provides no guaranteed maximum retry count to ensure forward
> progress of the demotion. Interrupt storms could then result in a DoS if
> host simply retries endlessly for TDX_INTERRUPTED_RESTARTABLE. Disabling
> interrupts before invoking the SEAMCALL also doesn't work because NMIs can
> also trigger TDX_INTERRUPTED_RESTARTABLE. Therefore, the tradeoff for basic
> TDX is to disable the TDX_INTERRUPTED_RESTARTABLE error given the
> reasonable execution time for demotion. [1]
> 

[...]

> v3:
> - Use a var name that clearly tell that the page is used as a page table
>   page. (Binbin).
> - Check if TDX module supports feature ENHANCE_DEMOTE_INTERRUPTIBILITY.
>   (Kai).
> 
[...]

> +u64 tdh_mem_page_demote(struct tdx_td *td, u64 gpa, int level, struct page *new_sept_page,
> +			u64 *ext_err1, u64 *ext_err2)
> +{
> +	struct tdx_module_args args = {
> +		.rcx = gpa | level,
> +		.rdx = tdx_tdr_pa(td),
> +		.r8 = page_to_phys(new_sept_page),
> +	};
> +	u64 ret;
> +
> +	if (!tdx_supports_demote_nointerrupt(&tdx_sysinfo))
> +		return TDX_SW_ERROR;
> 

For the record, while I replied my suggestion [*] to this patch in v2, it
was basically because the discussion was already in that patch -- I didn't
mean to do this check inside tdh_mem_page_demote(), but do this check in
KVM page fault patch and return 4K as maximum mapping level.

The precise words were:

  So if the decision is to not use 2M page when TDH_MEM_PAGE_DEMOTE can 
  return TDX_INTERRUPTED_RESTARTABLE, maybe we can just check this 
  enumeration in fault handler and always make mapping level as 4K?

Looking at this series, this is eventually done in your last patch.  But I
don't quite understand what's the additional value of doing such check and
return TDX_SW_ERROR in this SEAMCALL wrapper.

Currently in this series, it doesn't matter whether this wrapper returns
TDX_SW_ERROR or the real TDX_INTERRUPTED_RESTARTABLE -- KVM terminates the
TD anyway (see your patch 8) because this is unexpected as checked in your
last patch.

IMHO we should get rid of this check in this low level wrapper.

[*]:
https://lore.kernel.org/all/fbf04b09f13bc2ce004ac97ee9c1f2c965f44fdf.camel@intel.com/#t