[v8,1/2] x86/tdx: Retry TDVMCALL_MAP_GPA() when needed

  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 fully enlightened TDX guest runs on Hyper-V, Hyper-V can return
the retry error when set_memory_decrypted() is called to decrypt up to
1GB of swiotlb bounce buffers.

Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Reviewed-by: Michael Kelley <mikelley@microsoft.com>
Signed-off-by: Dexuan Cui <decui@microsoft.com>
---

 arch/x86/coco/tdx/tdx.c           | 63 +++++++++++++++++++++++++------
 arch/x86/include/asm/shared/tdx.h |  2 +
 2 files changed, 53 insertions(+), 12 deletions(-)

Changes in v2:
  Used __tdx_hypercall() directly in tdx_map_gpa().
  Added a max_retry_cnt of 1000.
  Renamed a few variables, e.g., r11 -> map_fail_paddr.

Changes in v3:
  Changed max_retry_cnt from 1000 to 3.

Changes in v4:
  __tdx_hypercall(&args, TDX_HCALL_HAS_OUTPUT) -> __tdx_hypercall_ret()
  Added Kirill's Acked-by.

Changes in v5:
  Added Michael's Reviewed-by.

Changes in v6: None.

Changes in v7:
  Addressed Dave's comments:
  see https://lwn.net/ml/linux-kernel/SA1PR21MB1335736123C2BCBBFD7460C3BF46A@SA1PR21MB1335.namprd21.prod.outlook.com

Changes in v8:
  Rebased to tip.git's master branch.
  

Hi,

On 6/20/23 8:48 AM, 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 fully enlightened TDX guest runs on Hyper-V, Hyper-V can return
> the retry error when set_memory_decrypted() is called to decrypt up to
> 1GB of swiotlb bounce buffers.
> 
> Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Reviewed-by: Michael Kelley <mikelley@microsoft.com>
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
> 
>  arch/x86/coco/tdx/tdx.c           | 63 +++++++++++++++++++++++++------
>  arch/x86/include/asm/shared/tdx.h |  2 +
>  2 files changed, 53 insertions(+), 12 deletions(-)
> 
> Changes in v2:
>   Used __tdx_hypercall() directly in tdx_map_gpa().
>   Added a max_retry_cnt of 1000.
>   Renamed a few variables, e.g., r11 -> map_fail_paddr.
> 
> Changes in v3:
>   Changed max_retry_cnt from 1000 to 3.
> 
> Changes in v4:
>   __tdx_hypercall(&args, TDX_HCALL_HAS_OUTPUT) -> __tdx_hypercall_ret()
>   Added Kirill's Acked-by.
> 
> Changes in v5:
>   Added Michael's Reviewed-by.
> 
> Changes in v6: None.
> 
> Changes in v7:
>   Addressed Dave's comments:
>   see https://lwn.net/ml/linux-kernel/SA1PR21MB1335736123C2BCBBFD7460C3BF46A@SA1PR21MB1335.namprd21.prod.outlook.com
> 
> Changes in v8:
>   Rebased to tip.git's master branch.
> 
> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
> index 1d6b863c42b0..0c198ab73aa7 100644
> --- a/arch/x86/coco/tdx/tdx.c
> +++ b/arch/x86/coco/tdx/tdx.c
> @@ -703,14 +703,16 @@ static bool tdx_cache_flush_required(void)
>  }
>  
>  /*
> - * 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
> - * of the page in response.
> + * 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_enc_status_changed(unsigned long vaddr, int numpages, bool enc)
> +static bool tdx_map_gpa(phys_addr_t start, phys_addr_t end, bool enc)
>  {
> -	phys_addr_t start = __pa(vaddr);
> -	phys_addr_t end   = __pa(vaddr + numpages * PAGE_SIZE);
> +	const int max_retries_per_page = 3;

Add some details about why you chose 3? Maybe you can also use macro for it.

> +	struct tdx_hypercall_args args;
> +	u64 map_fail_paddr, ret;
> +	int retry_count = 0;
>  
>  	if (!enc) {
>  		/* Set the shared (decrypted) bits: */
> @@ -718,12 +720,49 @@ 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))
> +	while (retry_count < max_retries_per_page) {
> +		memset(&args, 0, sizeof(args));
> +		args.r10 = TDX_HYPERCALL_STANDARD;
> +		args.r11 = TDVMCALL_MAP_GPA;
> +		args.r12 = start;
> +		args.r13 = end - start;
> +
> +		ret = __tdx_hypercall_ret(&args);
> +		if (ret != TDVMCALL_STATUS_RETRY)
> +			return !ret;
> +		/*
> +		 * The guest must retry the operation for the pages in the
> +		 * region starting at the GPA specified in R11. R11 comes
> +		 * from the untrusted VMM. Sanity check it.
> +		 */
> +		map_fail_paddr = args.r11;

Do you really need map_fail_paddr? Why not directly use args.r11?

> +		if (map_fail_paddr < start || map_fail_paddr >= end)
> +			return false;
> +
> +		/* "Consume" a retry without forward progress */
> +		if (map_fail_paddr == start) {
> +			retry_count++;
> +			continue;
> +		}
> +
> +		start = map_fail_paddr;
> +		retry_count = 0;
> +	}
> +
> +	return false;
> +}
> +
> +/*
> + * 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
> + * of the page in response.
> + */
> +static bool tdx_enc_status_changed(unsigned long vaddr, int numpages, bool enc)
> +{
> +	phys_addr_t start = __pa(vaddr);
> +	phys_addr_t end   = __pa(vaddr + numpages * PAGE_SIZE);
> +
> +	if (!tdx_map_gpa(start, end, enc))
>  		return false;
>  
>  	/* shared->private conversion requires memory to be accepted before use */
> diff --git a/arch/x86/include/asm/shared/tdx.h b/arch/x86/include/asm/shared/tdx.h
> index 90ea813c4b99..9db89a99ae5b 100644
> --- a/arch/x86/include/asm/shared/tdx.h
> +++ b/arch/x86/include/asm/shared/tdx.h
> @@ -24,6 +24,8 @@
>  #define TDVMCALL_MAP_GPA		0x10001
>  #define TDVMCALL_REPORT_FATAL_ERROR	0x10003
>  
> +#define TDVMCALL_STATUS_RETRY		1
> +
>  #ifndef __ASSEMBLY__
>  
>  /*
  

> From: Sathyanarayanan Kuppuswamy
> Sent: Tuesday, June 20, 2023 11:31 AM
> > ...
> > -static bool tdx_enc_status_changed(unsigned long vaddr, int numpages,
> bool enc)
> > +static bool tdx_map_gpa(phys_addr_t start, phys_addr_t end, bool enc)
> >  {
> > -	phys_addr_t start = __pa(vaddr);
> > -	phys_addr_t end   = __pa(vaddr + numpages * PAGE_SIZE);
> > +	const int max_retries_per_page = 3;
> 
> Add some details about why you chose 3? Maybe you can also use macro for it.

It's a small number recommended by Kirill:
https://lwn.net/ml/linux-kernel/20221208194800.n27ak4xj6pmyny46@box.shutemov.name/

The spec doesn't define a max retry count. Normally I guess a max retry count
of 2 should be enough, at least for Hyper-V according to my testing.

Maybe we can add a comment like this:

/* Retrying the hypercall a second time should succeed; use 3 just in case. */

Does this look good to all?

> > +	struct tdx_hypercall_args args;
> > +	u64 map_fail_paddr, ret;
> > +	int retry_count = 0;
> >
> >  	if (!enc) {
> >  		/* Set the shared (decrypted) bits: */
> > @@ -718,12 +720,49 @@ 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))
> > +	while (retry_count < max_retries_per_page) {
> > +		memset(&args, 0, sizeof(args));
> > +		args.r10 = TDX_HYPERCALL_STANDARD;
> > +		args.r11 = TDVMCALL_MAP_GPA;
> > +		args.r12 = start;
> > +		args.r13 = end - start;
> > +
> > +		ret = __tdx_hypercall_ret(&args);
> > +		if (ret != TDVMCALL_STATUS_RETRY)
> > +			return !ret;
> > +		/*
> > +		 * The guest must retry the operation for the pages in the
> > +		 * region starting at the GPA specified in R11. R11 comes
> > +		 * from the untrusted VMM. Sanity check it.
> > +		 */
> > +		map_fail_paddr = args.r11;
> 
> Do you really need map_fail_paddr? Why not directly use args.r11?
> 
> > +		if (map_fail_paddr < start || map_fail_paddr >= end)
> > +			return false;

Originally, I used r11. 

Dave says " 'r11' needs a real, logical name":
https://lwn.net/ml/linux-kernel/6bb65614-d420-49d3-312f-316dc8ca4cc4@intel.com/
  

Hi,

On 6/20/23 12:23 PM, Dexuan Cui wrote:
>> From: Sathyanarayanan Kuppuswamy
>> Sent: Tuesday, June 20, 2023 11:31 AM
>>> ...
>>> -static bool tdx_enc_status_changed(unsigned long vaddr, int numpages,
>> bool enc)
>>> +static bool tdx_map_gpa(phys_addr_t start, phys_addr_t end, bool enc)
>>>  {
>>> -	phys_addr_t start = __pa(vaddr);
>>> -	phys_addr_t end   = __pa(vaddr + numpages * PAGE_SIZE);
>>> +	const int max_retries_per_page = 3;
>>
>> Add some details about why you chose 3? Maybe you can also use macro for it.
> 
> It's a small number recommended by Kirill:
> https://lwn.net/ml/linux-kernel/20221208194800.n27ak4xj6pmyny46@box.shutemov.name/
> 
> The spec doesn't define a max retry count. Normally I guess a max retry count
> of 2 should be enough, at least for Hyper-V according to my testing.
> 
> Maybe we can add a comment like this:
> 
> /* Retrying the hypercall a second time should succeed; use 3 just in case. */
> 
> Does this look good to all?

Looks fine to me.

> 
>>> +	struct tdx_hypercall_args args;
>>> +	u64 map_fail_paddr, ret;
>>> +	int retry_count = 0;
>>>
>>>  	if (!enc) {
>>>  		/* Set the shared (decrypted) bits: */
>>> @@ -718,12 +720,49 @@ 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))
>>> +	while (retry_count < max_retries_per_page) {
>>> +		memset(&args, 0, sizeof(args));
>>> +		args.r10 = TDX_HYPERCALL_STANDARD;
>>> +		args.r11 = TDVMCALL_MAP_GPA;
>>> +		args.r12 = start;
>>> +		args.r13 = end - start;
>>> +
>>> +		ret = __tdx_hypercall_ret(&args);
>>> +		if (ret != TDVMCALL_STATUS_RETRY)
>>> +			return !ret;
>>> +		/*
>>> +		 * The guest must retry the operation for the pages in the
>>> +		 * region starting at the GPA specified in R11. R11 comes
>>> +		 * from the untrusted VMM. Sanity check it.
>>> +		 */
>>> +		map_fail_paddr = args.r11;
>>
>> Do you really need map_fail_paddr? Why not directly use args.r11?
>>
>>> +		if (map_fail_paddr < start || map_fail_paddr >= end)
>>> +			return false;
> 
> Originally, I used r11. 
> 
> Dave says " 'r11' needs a real, logical name":
> https://lwn.net/ml/linux-kernel/6bb65614-d420-49d3-312f-316dc8ca4cc4@intel.com/

Got it.

Reviewed-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
  

On 6/20/23 08:48, Dexuan Cui wrote:
> -static bool tdx_enc_status_changed(unsigned long vaddr, int numpages, bool enc)
> +static bool tdx_map_gpa(phys_addr_t start, phys_addr_t end, bool enc)
>  {
> -	phys_addr_t start = __pa(vaddr);
> -	phys_addr_t end   = __pa(vaddr + numpages * PAGE_SIZE);
> +	const int max_retries_per_page = 3;
> +	struct tdx_hypercall_args args;
> +	u64 map_fail_paddr, ret;
> +	int retry_count = 0;
>  
>  	if (!enc) {
>  		/* Set the shared (decrypted) bits: */
> @@ -718,12 +720,49 @@ 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))
> +	while (retry_count < max_retries_per_page) {
> +		memset(&args, 0, sizeof(args));
> +		args.r10 = TDX_HYPERCALL_STANDARD;
> +		args.r11 = TDVMCALL_MAP_GPA;
> +		args.r12 = start;
> +		args.r13 = end - start;
> +

What's wrong with:

	while (retry_count < max_retries_per_page) {
		struct tdx_hypercall_args args = {
			.r10 = TDX_HYPERCALL_STANDARD,
			.r11 = TDVMCALL_MAP_GPA,
			.r12 = start,
			.r13 = end - start };

?

Or maybe with the brackets slightly differently arranged.

Why'd you declare all the variables outside the while() loop anyway?
  

> From: Dave Hansen <dave.hansen@intel.com>
> Sent: Tuesday, June 20, 2023 4:34 PM
> >  ...
> > -	if (_tdx_hypercall(TDVMCALL_MAP_GPA, start, end - start, 0, 0))
> > +	while (retry_count < max_retries_per_page) {
> > +		memset(&args, 0, sizeof(args));
> > +		args.r10 = TDX_HYPERCALL_STANDARD;
> > +		args.r11 = TDVMCALL_MAP_GPA;
> > +		args.r12 = start;
> > +		args.r13 = end - start;
> > +
> 
> What's wrong with:
> 
> 	while (retry_count < max_retries_per_page) {
> 		struct tdx_hypercall_args args = {
> 			.r10 = TDX_HYPERCALL_STANDARD,
> 			.r11 = TDVMCALL_MAP_GPA,
> 			.r12 = start,
> 			.r13 = end - start };
> 
> ?
> 
> Or maybe with the brackets slightly differently arranged.
> 
> Why'd you declare all the variables outside the while() loop anyway?

Thanks for the suggestion of making the code compact!
I'll apply the below diff, and post v9 tomorrow (trying to
not post too frequently...)

diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
index 08eac8f46c11..1cb7e9ee3a68 100644
--- a/arch/x86/coco/tdx/tdx.c
+++ b/arch/x86/coco/tdx/tdx.c
@@ -710,9 +710,8 @@ static bool tdx_cache_flush_required(void)
  */
 static bool tdx_map_gpa(phys_addr_t start, phys_addr_t end, bool enc)
 {
+	/* Retrying the hypercall a second time should succeed; use 3 just in case */
 	const int max_retries_per_page = 3;
-	struct tdx_hypercall_args args;
-	u64 map_fail_paddr, ret;
 	int retry_count = 0;
 
 	if (!enc) {
@@ -722,13 +721,14 @@ static bool tdx_map_gpa(phys_addr_t start, phys_addr_t end, bool enc)
 	}
 
 	while (retry_count < max_retries_per_page) {
-		memset(&args, 0, sizeof(args));
-		args.r10 = TDX_HYPERCALL_STANDARD;
-		args.r11 = TDVMCALL_MAP_GPA;
-		args.r12 = start;
-		args.r13 = end - start;
-
-		ret = __tdx_hypercall_ret(&args);
+		struct tdx_hypercall_args args = {
+			.r10 = TDX_HYPERCALL_STANDARD,
+			.r11 = TDVMCALL_MAP_GPA,
+			.r12 = start,
+			.r13 = end - start };
+
+		u64 map_fail_paddr;
+		u64 ret = __tdx_hypercall_ret(&args);
 		if (ret != TDVMCALL_STATUS_RETRY)
 			return !ret;
 		/*


The function now looks like this:

/*
 * 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)
{
	/* Retrying the hypercall a second time should succeed; use 3 just in case */
	const int max_retries_per_page = 3;
	int retry_count = 0;

	if (!enc) {
		/* Set the shared (decrypted) bits: */
		start |= cc_mkdec(0);
		end   |= cc_mkdec(0);
	}

	while (retry_count < max_retries_per_page) {
		struct tdx_hypercall_args args = {
			.r10 = TDX_HYPERCALL_STANDARD,
			.r11 = TDVMCALL_MAP_GPA,
			.r12 = start,
			.r13 = end - start };

		u64 map_fail_paddr;
		u64 ret = __tdx_hypercall_ret(&args);
		if (ret != TDVMCALL_STATUS_RETRY)
			return !ret;
		/*
		 * The guest must retry the operation for the pages in the
		 * region starting at the GPA specified in R11. R11 comes
		 * from the untrusted VMM. Sanity check it.
		 */
		map_fail_paddr = args.r11;
		if (map_fail_paddr < start || map_fail_paddr >= end)
			return false;

		/* "Consume" a retry without forward progress */
		if (map_fail_paddr == start) {
			retry_count++;
			continue;
		}

		start = map_fail_paddr;
		retry_count = 0;
	}

	return false;
}