Messages in this thread Patch in this message | | | Subject | Re: [RFC v2 29/32] x86/tdx: Add helper to do MapGPA TDVMALL | From | Dave Hansen <> | Date | Wed, 19 May 2021 08:59:04 -0700 |
| |
On 4/26/21 11:01 AM, Kuppuswamy Sathyanarayanan wrote: > From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> > > 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.
| |