Received-SPF: pass (google.com: domain of linux-kernel+bounces-52009-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Message-ID: <de174460-018c-4402-8fca-8555be669aa2@linux.intel.com>
Date: Mon, 5 Feb 2024 11:14:03 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v18 058/121] KVM: TDX: Retry seamcall when
 TDX_OPERAND_BUSY with operand SEPT
To: isaku.yamahata@intel.com
Cc: kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
 isaku.yamahata@gmail.com, Paolo Bonzini <pbonzini@redhat.com>,
 erdemaktas@google.com, Sean Christopherson <seanjc@google.com>,
 Sagi Shahar <sagis@google.com>, Kai Huang <kai.huang@intel.com>,
 chen.bo@intel.com, hang.yuan@intel.com, tina.zhang@intel.com,
 Yuan Yao <yuan.yao@intel.com>
References: <cover.1705965634.git.isaku.yamahata@intel.com>
 <d854fa4415ca6d2b5d055d0b29b147a5cdf232f7.1705965635.git.isaku.yamahata@intel.com>
From: Binbin Wu <binbin.wu@linux.intel.com>
In-Reply-To: <d854fa4415ca6d2b5d055d0b29b147a5cdf232f7.1705965635.git.isaku.yamahata@intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit


On 1/23/2024 7:53 AM, isaku.yamahata@intel.com wrote:
> From: Yuan Yao <yuan.yao@intel.com>
>
> TDX module internally uses locks to protect internal resources.  It tries
> to acquire the locks.  If it fails to obtain the lock, it returns
> TDX_OPERAND_BUSY error without spin because its execution time limitation.
>
> TDX SEAMCALL API reference describes what resources are used.  It's known
> which TDX SEAMCALL can cause contention with which resources.  VMM can
> avoid contention inside the TDX module by avoiding contentious TDX SEAMCALL
> with, for example, spinlock.  Because OS knows better its process
> scheduling and its scalability, a lock at OS/VMM layer would work better
> than simply retrying TDX SEAMCALLs.
>
> TDH.MEM.* API except for TDH.MEM.TRACK operates on a secure EPT tree and
> the TDX module internally tries to acquire the lock of the secure EPT tree.
> They return TDX_OPERAND_BUSY | TDX_OPERAND_ID_SEPT in case of failure to
> get the lock.  TDX KVM allows sept callbacks to return error so that TDP
> MMU layer can retry.
>
> TDH.VP.ENTER is an exception with zero-step attack mitigation.  Normally
> TDH.VP.ENTER uses only TD vcpu resources and it doesn't cause contention.
> When a zero-step attack is suspected, it obtains a secure EPT tree lock and
> tracks the GPAs causing a secure EPT fault.  Thus TDG.VP.ENTER may result

Should be TDH.VP.ENTER.

> in TDX_OPERAND_BUSY | TDX_OPERAND_ID_SEPT.  Also TDH.MEM.* SEAMCALLs may
> result in TDX_OPERAN_BUSY | TDX_OPERAND_ID_SEPT.
s/TDX_OPERAN_BUSY/TDX_OPERAND_BUSY

>
> Retry TDX TDH.MEM.* API and TDH.VP.ENTER on the error because the error is
> a rare event caused by zero-step attack mitigation and spinlock can not be
> used for TDH.VP.ENTER due to indefinite time execution.

Does it retry TDH.VP.ENTER on SEPT busy?
I didn't see the related code in this patch.


>
> Signed-off-by: Yuan Yao <yuan.yao@intel.com>
> Signed-off-by: Isaku Yamahata <isaku.yamahata@intel.com>
> ---
>   arch/x86/kvm/vmx/tdx_ops.h | 48 +++++++++++++++++++++++++++++++-------
>   1 file changed, 39 insertions(+), 9 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx/tdx_ops.h b/arch/x86/kvm/vmx/tdx_ops.h
> index cd12e9c2a421..53a6c3f692b0 100644
> --- a/arch/x86/kvm/vmx/tdx_ops.h
> +++ b/arch/x86/kvm/vmx/tdx_ops.h
> @@ -52,6 +52,36 @@ static inline u64 tdx_seamcall(u64 op, struct tdx_module_args *in,
>   void pr_tdx_error(u64 op, u64 error_code, const struct tdx_module_args *out);
>   #endif
>   
> +/*
> + * TDX module acquires its internal lock for resources.  It doesn't spin to get
> + * locks because of its restrictions of allowed execution time.  Instead, it
> + * returns TDX_OPERAND_BUSY with an operand id.
> + *
> + * Multiple VCPUs can operate on SEPT.  Also with zero-step attack mitigation,
> + * TDH.VP.ENTER may rarely acquire SEPT lock and release it when zero-step
> + * attack is suspected.  It results in TDX_OPERAND_BUSY | TDX_OPERAND_ID_SEPT
> + * with TDH.MEM.* operation.  Note: TDH.MEM.TRACK is an exception.
> + *
> + * Because TDP MMU uses read lock for scalability, spin lock around SEAMCALL
> + * spoils TDP MMU effort.  Retry several times with the assumption that SEPT
> + * lock contention is rare.  But don't loop forever to avoid lockup.  Let TDP
> + * MMU retry.
> + */
> +#define TDX_ERROR_SEPT_BUSY    (TDX_OPERAND_BUSY | TDX_OPERAND_ID_SEPT)
> +
> +static inline u64 tdx_seamcall_sept(u64 op, struct tdx_module_args *in,
> +				    struct tdx_module_args *out)
> +{
> +#define SEAMCALL_RETRY_MAX     16
> +	int retry = SEAMCALL_RETRY_MAX;
> +	u64 ret;
> +
> +	do {
> +		ret = tdx_seamcall(op, in, out);
> +	} while (ret == TDX_ERROR_SEPT_BUSY && retry-- > 0);
> +	return ret;
> +}
> +
>   static inline u64 tdh_mng_addcx(hpa_t tdr, hpa_t addr)
>   {
>   	struct tdx_module_args in = {
> @@ -74,7 +104,7 @@ static inline u64 tdh_mem_page_add(hpa_t tdr, gpa_t gpa, hpa_t hpa, hpa_t source
>   	};
>   
>   	clflush_cache_range(__va(hpa), PAGE_SIZE);
> -	return tdx_seamcall(TDH_MEM_PAGE_ADD, &in, out);
> +	return tdx_seamcall_sept(TDH_MEM_PAGE_ADD, &in, out);
>   }
>   
>   static inline u64 tdh_mem_sept_add(hpa_t tdr, gpa_t gpa, int level, hpa_t page,
> @@ -87,7 +117,7 @@ static inline u64 tdh_mem_sept_add(hpa_t tdr, gpa_t gpa, int level, hpa_t page,
>   	};
>   
>   	clflush_cache_range(__va(page), PAGE_SIZE);
> -	return tdx_seamcall(TDH_MEM_SEPT_ADD, &in, out);
> +	return tdx_seamcall_sept(TDH_MEM_SEPT_ADD, &in, out);
>   }
>   
>   static inline u64 tdh_mem_sept_rd(hpa_t tdr, gpa_t gpa, int level,
> @@ -98,7 +128,7 @@ static inline u64 tdh_mem_sept_rd(hpa_t tdr, gpa_t gpa, int level,
>   		.rdx = tdr,
>   	};
>   
> -	return tdx_seamcall(TDH_MEM_SEPT_RD, &in, out);
> +	return tdx_seamcall_sept(TDH_MEM_SEPT_RD, &in, out);
>   }
>   
>   static inline u64 tdh_mem_sept_remove(hpa_t tdr, gpa_t gpa, int level,
> @@ -109,7 +139,7 @@ static inline u64 tdh_mem_sept_remove(hpa_t tdr, gpa_t gpa, int level,
>   		.rdx = tdr,
>   	};
>   
> -	return tdx_seamcall(TDH_MEM_SEPT_REMOVE, &in, out);
> +	return tdx_seamcall_sept(TDH_MEM_SEPT_REMOVE, &in, out);
>   }
>   
>   static inline u64 tdh_vp_addcx(hpa_t tdvpr, hpa_t addr)
> @@ -133,7 +163,7 @@ static inline u64 tdh_mem_page_relocate(hpa_t tdr, gpa_t gpa, hpa_t hpa,
>   	};
>   
>   	clflush_cache_range(__va(hpa), PAGE_SIZE);
> -	return tdx_seamcall(TDH_MEM_PAGE_RELOCATE, &in, out);
> +	return tdx_seamcall_sept(TDH_MEM_PAGE_RELOCATE, &in, out);
>   }
>   
>   static inline u64 tdh_mem_page_aug(hpa_t tdr, gpa_t gpa, hpa_t hpa,
> @@ -146,7 +176,7 @@ static inline u64 tdh_mem_page_aug(hpa_t tdr, gpa_t gpa, hpa_t hpa,
>   	};
>   
>   	clflush_cache_range(__va(hpa), PAGE_SIZE);
> -	return tdx_seamcall(TDH_MEM_PAGE_AUG, &in, out);
> +	return tdx_seamcall_sept(TDH_MEM_PAGE_AUG, &in, out);
>   }
>   
>   static inline u64 tdh_mem_range_block(hpa_t tdr, gpa_t gpa, int level,
> @@ -157,7 +187,7 @@ static inline u64 tdh_mem_range_block(hpa_t tdr, gpa_t gpa, int level,
>   		.rdx = tdr,
>   	};
>   
> -	return tdx_seamcall(TDH_MEM_RANGE_BLOCK, &in, out);
> +	return tdx_seamcall_sept(TDH_MEM_RANGE_BLOCK, &in, out);
>   }
>   
>   static inline u64 tdh_mng_key_config(hpa_t tdr)
> @@ -307,7 +337,7 @@ static inline u64 tdh_mem_page_remove(hpa_t tdr, gpa_t gpa, int level,
>   		.rdx = tdr,
>   	};
>   
> -	return tdx_seamcall(TDH_MEM_PAGE_REMOVE, &in, out);
> +	return tdx_seamcall_sept(TDH_MEM_PAGE_REMOVE, &in, out);
>   }
>   
>   static inline u64 tdh_sys_lp_shutdown(void)
> @@ -335,7 +365,7 @@ static inline u64 tdh_mem_range_unblock(hpa_t tdr, gpa_t gpa, int level,
>   		.rdx = tdr,
>   	};
>   
> -	return tdx_seamcall(TDH_MEM_RANGE_UNBLOCK, &in, out);
> +	return tdx_seamcall_sept(TDH_MEM_RANGE_UNBLOCK, &in, out);
>   }
>   
>   static inline u64 tdh_phymem_cache_wb(bool resume)