Re: [RFC v2 29/32] x86/tdx: Add helper to do MapGPA TDVMALL

From: Dave Hansen
Date: Wed May 19 2021 - 11:59:10 EST


On 4/26/21 11:01 AM, Kuppuswamy Sathyanarayanan wrote:
> From: "Kirill A. Shutemov" <kirill.shutemov@xxxxxxxxxxxxxxx>
>
> MapGPA TDVMCALL requests the host VMM to map a GPA range as private or
> shared memory mappings. Shared GPA mappings can be used for
> communication beteen TD guest and host VMM, for example for
> paravirtualized IO.

As usual, I hate the changelog. This appears to just be regurgitating
the spec.

Is this just for part of converting an existing mapping between private
and shared? If so, please say that.

> The new helper tdx_map_gpa() provides access to the operation.

<sigh> You got your own name wrong. It's tdg_map_gpa() in the patch.

BTW, I agree with Sean on this one: "tdg" is a horrible prefix. You
just proved Sean's point by mistyping it. *EVERYONE* is going to rpeat
that mistake: tdg -> tdx.

> diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h
> index dc80cf7f7d08..4789798d7737 100644
> --- a/arch/x86/include/asm/tdx.h
> +++ b/arch/x86/include/asm/tdx.h
> @@ -7,6 +7,11 @@
>
> #ifndef __ASSEMBLY__
>
> +enum tdx_map_type {
> + TDX_MAP_PRIVATE,
> + TDX_MAP_SHARED,
> +};

I like the enum, but please call out that this is a software construct,
not a part of any hardware or VMM ABI.

> #ifdef CONFIG_INTEL_TDX_GUEST
>
> #include <asm/cpufeature.h>
> @@ -112,6 +117,8 @@ unsigned short tdg_inw(unsigned short port);
> unsigned int tdg_inl(unsigned short port);
>
> extern phys_addr_t tdg_shared_mask(void);
> +extern int tdg_map_gpa(phys_addr_t gpa, int numpages,
> + enum tdx_map_type map_type);
>
> #else // !CONFIG_INTEL_TDX_GUEST
>
> @@ -155,6 +162,12 @@ static inline phys_addr_t tdg_shared_mask(void)
> {
> return 0;
> }
> +
> +static inline int tdg_map_gpa(phys_addr_t gpa, int numpages,
> + enum tdx_map_type map_type)
> +{
> + return -ENODEV;
> +}

FWIW, you could probably get away with just inlining tdg_map_gpa():

static inline int tdg_map_gpa(phys_addr_t gpa, int numpages, ...
{
u64 ret;

if (!IS_ENABLED(CONFIG_INTEL_TDX_GUEST))
return -ENODEV;

if (map_type == TDX_MAP_SHARED)
gpa |= tdg_shared_mask();

ret = tdvmcall(TDVMCALL_MAP_GPA, gpa, ...

return ret ? -EIO : 0;
}

Then you don't have three copies of the function signature that can get
out of sync.

> #endif /* CONFIG_INTEL_TDX_GUEST */
> #endif /* __ASSEMBLY__ */
> #endif /* _ASM_X86_TDX_H */
> diff --git a/arch/x86/kernel/tdx.c b/arch/x86/kernel/tdx.c
> index 7e391cd7aa2b..074136473011 100644
> --- a/arch/x86/kernel/tdx.c
> +++ b/arch/x86/kernel/tdx.c
> @@ -15,6 +15,8 @@
> #include "tdx-kvm.c"
> #endif
>
> +#define TDVMCALL_MAP_GPA 0x10001
> +
> static struct {
> unsigned int gpa_width;
> unsigned long attributes;
> @@ -98,6 +100,17 @@ static void tdg_get_info(void)
> physical_mask &= ~tdg_shared_mask();
> }
>
> +int tdg_map_gpa(phys_addr_t gpa, int numpages, enum tdx_map_type map_type)
> +{
> + u64 ret;
> +
> + if (map_type == TDX_MAP_SHARED)
> + gpa |= tdg_shared_mask();
> +
> + ret = tdvmcall(TDVMCALL_MAP_GPA, gpa, PAGE_SIZE * numpages, 0, 0);
> + return ret ? -EIO : 0;
> +}

The naming Intel chose here is nasty. This doesn't "map" anything. It
modifies an existing mapping from what I can tell. We could name it
much better than the spec, perhaps:

tdx_hcall_gpa_intent()

BTW, all of these hypercalls need a consistent prefix.

It also needs a comment:

/*
* 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.
*
* Note: shared->private conversions require further guest
* action to accept the page.
*/

The intent here is important. It makes it clear that this function
really only plays a role in the conversion process.