linux-kernel - Re: [PATCH 2/6] x86/tdx: Retry TDVMCALL_MAP

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: <20221122000100.bizske6iltfgdwcu@box.shutemov.name>
Date:   Tue, 22 Nov 2022 03:01:00 +0300
From:   "Kirill A. Shutemov" <kirill@...temov.name>
To:     Dexuan Cui <decui@...rosoft.com>
Cc:     ak@...ux.intel.com, arnd@...db.de, bp@...en8.de,
        brijesh.singh@....com, dan.j.williams@...el.com,
        dave.hansen@...ux.intel.com, haiyangz@...rosoft.com, hpa@...or.com,
        jane.chu@...cle.com, kirill.shutemov@...ux.intel.com,
        kys@...rosoft.com, linux-arch@...r.kernel.org,
        linux-hyperv@...r.kernel.org, luto@...nel.org, mingo@...hat.com,
        peterz@...radead.org, rostedt@...dmis.org,
        sathyanarayanan.kuppuswamy@...ux.intel.com, seanjc@...gle.com,
        tglx@...utronix.de, tony.luck@...el.com, wei.liu@...nel.org,
        x86@...nel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/6] x86/tdx: Retry TDVMCALL_MAP_GPA() when needed

On Mon, Nov 21, 2022 at 11:51:47AM -0800, Dexuan Cui wrote:
> GHCI spec for TDX 1.0 says that the MapGPA call may fail with the R10
> error code = TDG.VP.VMCALL_RETRY (1), and the guest must retry this
> operation for the pages in the region starting at the GPA specified
> in R11.
> 
> When a TDX guest runs on Hyper-V, Hyper-V returns the retry error
> when hyperv_init() -> swiotlb_update_mem_attributes() ->
> set_memory_decrypted() decrypts up to 1GB of swiotlb bounce buffers.
> 
> Signed-off-by: Dexuan Cui <decui@...rosoft.com>
> ---
>  arch/x86/coco/tdx/tdx.c | 65 +++++++++++++++++++++++++++++++++++++----
>  1 file changed, 59 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
> index 3fee96931ff5..46971cc7d006 100644
> --- a/arch/x86/coco/tdx/tdx.c
> +++ b/arch/x86/coco/tdx/tdx.c
> @@ -20,6 +20,8 @@
>  /* TDX hypercall Leaf IDs */
>  #define TDVMCALL_MAP_GPA		0x10001
>  
> +#define TDVMCALL_STATUS_RETRY		1
> +
>  /* MMIO direction */
>  #define EPT_READ	0
>  #define EPT_WRITE	1
> @@ -52,6 +54,25 @@ static inline u64 _tdx_hypercall(u64 fn, u64 r12, u64 r13, u64 r14, u64 r15)
>  	return __tdx_hypercall(&args, 0);
>  }
>  
> +static inline u64 _tdx_hypercall_output_r11(u64 fn, u64 r12, u64 r13, u64 r14,
> +					    u64 r15, u64 *r11)
> +{
> +	struct tdx_hypercall_args args = {
> +		.r10 = TDX_HYPERCALL_STANDARD,
> +		.r11 = fn,
> +		.r12 = r12,
> +		.r13 = r13,
> +		.r14 = r14,
> +		.r15 = r15,
> +	};
> +
> +	u64 ret;
> +
> +	ret = __tdx_hypercall(&args, TDX_HCALL_HAS_OUTPUT);
> +	*r11 = args.r11;
> +	return ret;
> +}
> +

I'm not convinced it deserves a separate helper for one user.
Does it look that ugly if tdx_map_gpa() uses __tdx_hypercall() directly?

>  /* Called from __tdx_hypercall() for unrecoverable failure */
>  void __tdx_hypercall_failed(void)
>  {
> @@ -691,6 +712,43 @@ static bool try_accept_one(phys_addr_t *start, unsigned long len,
>  	return true;
>  }
>  
> +/*
> + * Notify the VMM about page mapping conversion. More info about ABI
> + * can be found in TDX Guest-Host-Communication Interface (GHCI),
> + * section "TDG.VP.VMCALL<MapGPA>"
> + */
> +static bool tdx_map_gpa(phys_addr_t start, phys_addr_t end, bool enc)
> +{
> +	u64 ret, r11;
> +
> +	while (1) {

Endless? Maybe an upper limit if no progress?

> +		ret = _tdx_hypercall_output_r11(TDVMCALL_MAP_GPA, start,
> +						end - start, 0, 0, &r11);
> +		if (!ret)
> +			break;
> +
> +		if (ret != TDVMCALL_STATUS_RETRY)
> +			break;
> +
> +		/*
> +		 * The guest must retry the operation for the pages in the
> +		 * region starting at the GPA specified in R11. Make sure R11
> +		 * contains a sane value.
> +		 */
> +		if ((r11 & ~cc_mkdec(0)) < (start & ~cc_mkdec(0)) ||
> +		    (r11 & ~cc_mkdec(0)) >= (end  & ~cc_mkdec(0)))
> +			return false;

Emm. All of them suppose to have shared bit set, why not compare directly
without cc_mkdec() dance?

> +
> +		start = r11;
> +
> +		/* Set the shared (decrypted) bit. */
> +		if (!enc)
> +			start |= cc_mkdec(0);
> +	}
> +
> +	return !ret;
> +}
> +
>  /*
>   * Inform the VMM of the guest's intent for this physical page: shared with
>   * the VMM or private to the guest.  The VMM is expected to change its mapping
> @@ -707,12 +765,7 @@ static bool tdx_enc_status_changed(unsigned long vaddr, int numpages, bool enc)
>  		end   |= cc_mkdec(0);
>  	}
>  
> -	/*
> -	 * Notify the VMM about page mapping conversion. More info about ABI
> -	 * can be found in TDX Guest-Host-Communication Interface (GHCI),
> -	 * section "TDG.VP.VMCALL<MapGPA>"
> -	 */
> -	if (_tdx_hypercall(TDVMCALL_MAP_GPA, start, end - start, 0, 0))
> +	if (!tdx_map_gpa(start, end, enc))
>  		return false;
>  
>  	/* private->shared conversion  requires only MapGPA call */
> -- 
> 2.25.1
> 

-- 
  Kiryl Shutsemau / Kirill A. Shutemov