lkml.org 
[lkml]   [2021]   [May]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [RFC v2 29/32] x86/tdx: Add helper to do MapGPA TDVMALL
From
Date


On 5/19/21 8:59 AM, Dave Hansen wrote:
> 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.
>

How about following change?

x86/tdx: Add helper to do MapGPA hypercall

MapGPA hypercall is used by TDX guests to request VMM convert
the existing mapping of given GPA address range between
private/shared.

tdx_hcall_gpa_intent() is the wrapper used for making MapGPA
hypercall.


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

I can use tdx_hcall_gpa_intent().

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

I agree that this simplifies the function definition. But, there are
other TDX hypercalls definitions in tdx.c. I can't move all of them to
the header file. If possible, I would like to group all hypercalls in
the same place.

Also, IMO, it is better to hide hypercall internal implementation details
in C file. For example, user of MapGPA hypercall does not care about the
TDVMCALL_MAP_GPA leaf id value. If we inline this function we have to
move such details to header file.


>
>> #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()

I will use this function name in next version.

>
> BTW, all of these hypercalls need a consistent prefix.

I can include _hcall in other hypercall helper functions as well.

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

Thanks. I will include it in next version.

>

--
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

\
 
 \ /
  Last update: 2021-05-21 01:15    [W:0.296 / U:0.472 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site