[2/6] 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 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@microsoft.com>
---
 arch/x86/coco/tdx/tdx.c | 65 +++++++++++++++++++++++++++++++++++++----
 1 file changed, 59 insertions(+), 6 deletions(-)
  

On 11/21/22 11:51, Dexuan Cui wrote:
> +static bool tdx_map_gpa(phys_addr_t start, phys_addr_t end, bool enc)
> +{
> +	u64 ret, r11;

'r11' needs a real, logical name.

> +	while (1) {
> +		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;

This statement is, um, a wee bit ugly.

First, it's not obvious at all why the address masking is necessary.

Second, it's utterly insane to do that mask to 'r11' twice.  Third, it's
silly do logically the same thing to start and end every time through
the loop.

This also seems to have built in the idea that cc_mkdec() *SETS* bits
rather than clearing them.  That's true for TDX today, but it's a
horrible chunk of code to leave around because it'll confuse actual
legitimate cc_enc/dec() users.

The more I write about it, the more I dislike it.

Why can't this just be:

		if ((map_fail_paddr < start) ||
		    (map_fail_paddr >= end))
			return false;

If the hypervisor returns some crazy address in r11 that isn't masked
like the inputs, just fail.

> +		start = r11;
> +
> +		/* Set the shared (decrypted) bit. */
> +		if (!enc)
> +			start |= cc_mkdec(0);

Why is only one side of this necessary?  Shouldn't it need to be
something like:

		if (enc)
			start = cc_mkenc(start);
		else
			start = cc_mkdec(start);

??

> +	}
> +
> +	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 */
  

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@microsoft.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
>
  

> Sent: Monday, November 21, 2022 12:56 PM
> On 11/21/22 11:51, Dexuan Cui wrote:
> > +static bool tdx_map_gpa(phys_addr_t start, phys_addr_t end, bool enc)
> > +{
> > +	u64 ret, r11;
> 
> 'r11' needs a real, logical name.

OK, will use "map_fail_paddr" (as you implied below).
 
> > +	while (1) {
> > +		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;
> 
> This statement is, um, a wee bit ugly.
> 
> First, it's not obvious at all why the address masking is necessary.

It turns out that the masking is completely unnecessary :-)

I incorrectly assumed that if the input 'start' has the bit 47, Hyper-V
always returns a physical address without bit 47. This is not the case.

I'll remove the masking code in v2.
 
> Second, it's utterly insane to do that mask to 'r11' twice.  Third, it's
> silly do logically the same thing to start and end every time through
> the loop.
> 
> This also seems to have built in the idea that cc_mkdec() *SETS* bits
> rather than clearing them.  That's true for TDX today, but it's a
> horrible chunk of code to leave around because it'll confuse actual
> legitimate cc_enc/dec() users.
> 
> The more I write about it, the more I dislike it.
> 
> Why can't this just be:
> 
> 		if ((map_fail_paddr < start) ||
> 		    (map_fail_paddr >= end))
> 			return false;
> 
> If the hypervisor returns some crazy address in r11 that isn't masked
> like the inputs, just fail.

Will use your example code in v2.

> > +		start = r11;
> > +
> > +		/* Set the shared (decrypted) bit. */
> > +		if (!enc)
> > +			start |= cc_mkdec(0);
> 
> Why is only one side of this necessary?  Shouldn't it need to be
> something like:
> 
> 		if (enc)
> 			start = cc_mkenc(start);
> 		else
> 			start = cc_mkdec(start);
> 
> ??
The code is unnecessary. Will remove it in v2.
  

> From: Kirill A. Shutemov <kirill@shutemov.name>
> Sent: Monday, November 21, 2022 4:01 PM
> [...]
> On Mon, Nov 21, 2022 at 11:51:47AM -0800, Dexuan Cui wrote:
> [...]
> 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?

Will use __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?

I'll add a max count of 1000, which should be far more than enough.

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

The above code is unnecessary and will be removed.

So, I'll use the below in v2:

/*
 * 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)
{
        int max_retry_cnt = 1000, retry_cnt = 0;
        struct tdx_hypercall_args args;
        u64 map_fail_paddr, ret;

        while (1) {
                args.r10 = TDX_HYPERCALL_STANDARD;
                args.r11 = TDVMCALL_MAP_GPA;
                args.r12 = start;
                args.r13 = end - start;
                args.r14 = 0;
                args.r15 = 0;

                ret = __tdx_hypercall(&args, TDX_HCALL_HAS_OUTPUT);
                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.
                 */
                map_fail_paddr = args.r11 ;
                if (map_fail_paddr < start || map_fail_paddr >= end)
                        return false;

                if (map_fail_paddr == start) {
                        retry_cnt++;
                        if (retry_cnt > max_retry_cnt)
                                return false;
                } else {
                        retry_cnt = 0;;
                        start = map_fail_paddr;
                }
        }

        return !ret;
}
  

From: Dexuan Cui <decui@microsoft.com> Sent: Tuesday, November 22, 2022 7:27 PM
> 
> > From: Kirill A. Shutemov <kirill@shutemov.name>
> > Sent: Monday, November 21, 2022 4:01 PM
> > [...]
> > On Mon, Nov 21, 2022 at 11:51:47AM -0800, Dexuan Cui wrote:
> > [...]
> > 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?
> 
> Will use __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?
> 
> I'll add a max count of 1000, which should be far more than enough.
> 
> > > +		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?
> 
> The above code is unnecessary and will be removed.
> 
> So, I'll use the below in v2:
> 
> /*
>  * 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)
> {
>         int max_retry_cnt = 1000, retry_cnt = 0;
>         struct tdx_hypercall_args args;
>         u64 map_fail_paddr, ret;
> 
>         while (1) {
>                 args.r10 = TDX_HYPERCALL_STANDARD;
>                 args.r11 = TDVMCALL_MAP_GPA;
>                 args.r12 = start;
>                 args.r13 = end - start;
>                 args.r14 = 0;
>                 args.r15 = 0;
> 
>                 ret = __tdx_hypercall(&args, TDX_HCALL_HAS_OUTPUT);
>                 if (!ret)
>                         break;

The above test is redundant and can be removed.  The "success" case is
implicitly handled by the test below for != TDVMCALL_STATUS_RETRY.

> 
>                 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.
>                  */
>                 map_fail_paddr = args.r11 ;
>                 if (map_fail_paddr < start || map_fail_paddr >= end)
>                         return false;
> 
>                 if (map_fail_paddr == start) {
>                         retry_cnt++;
>                         if (retry_cnt > max_retry_cnt)
>                                 return false;
>                 } else {
>                         retry_cnt = 0;;
>                         start = map_fail_paddr;

Just summarizing the code, we increment the retry count if the hypercall
returns STATUS_RETRY but did nothing (i.e., map_fail_paddr == start).  But
if the hypercall returns STATUS_RETRY after making at least some progress,
then we reset the retry count.   So in the worst case, for example, if the
hypercall processed only one page on each invocation, the loop will continue
until completion, without hitting any retry limits.  That scenario seems
plausible and within the spec.

Do we have any indication about the likelihood of the "RETRY but did
nothing" case?   The spec doesn't appear to disallow this case, but does
Hyper-V actually do this?  It seems like a weird case.

Michael

>                 }
>         }
> 
>         return !ret;
> }
  

> From: Michael Kelley (LINUX) <mikelley@microsoft.com>
> Sent: Wednesday, November 23, 2022 5:30 AM
> > [...]
> > static bool tdx_map_gpa(phys_addr_t start, phys_addr_t end, bool enc)
> > {
> >         int max_retry_cnt = 1000, retry_cnt = 0;
> >         struct tdx_hypercall_args args;
> >         u64 map_fail_paddr, ret;
> >
> >         while (1) {
> >                 args.r10 = TDX_HYPERCALL_STANDARD;
> >                 args.r11 = TDVMCALL_MAP_GPA;
> >                 args.r12 = start;
> >                 args.r13 = end - start;
> >                 args.r14 = 0;
> >                 args.r15 = 0;
> >
> >                 ret = __tdx_hypercall(&args, TDX_HCALL_HAS_OUTPUT);
> >                 if (!ret)
> >                         break;
> 
> The above test is redundant and can be removed.  The "success" case is
> implicitly handled by the test below for != TDVMCALL_STATUS_RETRY.

Good point. Will remove the redundant test.

> >                 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.
> >                  */
> >                 map_fail_paddr = args.r11 ;
> >                 if (map_fail_paddr < start || map_fail_paddr >= end)
> >                         return false;
> >
> >                 if (map_fail_paddr == start) {
> >                         retry_cnt++;
> >                         if (retry_cnt > max_retry_cnt)
> >                                 return false;
> >                 } else {
> >                         retry_cnt = 0;;
> >                         start = map_fail_paddr;
> 
> Just summarizing the code, we increment the retry count if the hypercall
> returns STATUS_RETRY but did nothing (i.e., map_fail_paddr == start).  But
> if the hypercall returns STATUS_RETRY after making at least some progress,
> then we reset the retry count.   So in the worst case, for example, if the
> hypercall processed only one page on each invocation, the loop will continue
> until completion, without hitting any retry limits.  That scenario seems
> plausible and within the spec.

Exactly.

> Do we have any indication about the likelihood of the "RETRY but did
> nothing" case? The spec doesn't appear to disallow this case, but does
> Hyper-V actually do this?  It seems like a weird case.
> 
> Michael

Yes, Hyper-V does do this, according to my test. It looks like this is not
because the operation is too time-consuming -- it looks like there is some
Hyper-V specific activity going on.