[RFC,v2,0/6] SEAMCALL Wrappers

Message

Edgecombe, Rick P Dec. 3, 2024, 1:03 a.m. UTC

Hi,

This is a followup to the "SEAMCALL Wrappers" RFC[0] that spun out of 
Dave’s comments on the SEAMCALL wrappers in the “TDX vCPU/VM creation” 
series [1]. To try to summarize Dave’s comments, he noted that the 
SEAMCALL wrappers were very thin, not even using proper types for things 
where types exist.

The last discussion was to use struct pages instead of u64. It is pretty 
much what Dave suggested with a minor tweak to instead include the tdcx 
page count in the TD struct instead of the vCPU one.

This is because it will not vary between vCPUs. Doing it that way 
basically preserves the existing data duplication, but these counts are 
basically "global metadata". The global metadata patches export them as a 
size, but KVM wants to use them as a page count. So we should not be 
including these counts in each TD scoped structure as is currently done. To
address the duplication we need to change the "global metadata patches"
to export the count instead of size.

Otherwise, in the spirit of looking to find better types for the other raw 
u64's, I played around again with the out params of 
tdh_phymem_page_reclaim(). In the end I opted for better names and a 
comment rather than anything fancier.

Here is the branch with the VM/vCPU caller adjustments as the last commit:
https://github.com/intel/tdx/tree/seamcall-rfc-v2

Thanks,

Rick

[0]
https://lore.kernel.org/kvm/[e-mail]/
[1]
https://lore.kernel.org/kvm/[e-mail]/


Rick Edgecombe (6):
  x86/virt/tdx: Add SEAMCALL wrappers for TDX KeyID management
  x86/virt/tdx: Add SEAMCALL wrappers for TDX TD creation
  x86/virt/tdx: Add SEAMCALL wrappers for TDX vCPU creation
  x86/virt/tdx: Add SEAMCALL wrappers for TDX page cache management
  x86/virt/tdx: Add SEAMCALL wrappers for TDX VM/vCPU field access
  x86/virt/tdx: Add SEAMCALL wrappers for TDX flush operations

 arch/x86/include/asm/tdx.h  |  38 ++++++
 arch/x86/virt/vmx/tdx/tdx.c | 240 ++++++++++++++++++++++++++++++++++++
 arch/x86/virt/vmx/tdx/tdx.h |  38 ++++--
 3 files changed, 309 insertions(+), 7 deletions(-)

Comments

Huang, Kai Dec. 4, 2024, 1:24 a.m. UTC | #1

> 
> This is because it will not vary between vCPUs. Doing it that way 
> basically preserves the existing data duplication, but these counts are 
> basically "global metadata". The global metadata patches export them as a 
> size, but KVM wants to use them as a page count. So we should not be 
> including these counts in each TD scoped structure as is currently done. To
> address the duplication we need to change the "global metadata patches"
> to export the count instead of size.
> 

Currently the global metadata reading script generates the struct member based
on the "field name" of the JSON file.  The JSON file stores them as "size":

  "TDR_BASE_SIZE", "TDCS_BASE_SIZE", "TDVPS_BASE_SIZE"

We will need to tweak the script to map "metadata field name" to "kernel
structure member name", and more "special handling for specific fields" when
auto generating the code.

It's feasible but I am not sure whether it's worth to do, since we are basically
talking about converting size to page count.

Also, from global metadata's point of view, perhaps it is also good to just
provide a metadata which is consistent with what module reports.  How kernel
uses the metadata is another layer on top of it.

Btw, perhaps we don't need to keep 'tdcs_nr_pages' and 'tdcx_nr_pages' in
'struct tdx_td', i.e., as per-TD variables.  They are constants for all TDX
guests.

E.g., assuming KVM is still going to use them, it can just access them using the
metadata structure:

	static inline int tdx_tdcs_nr_pages(void)
	{
		return tdx_sysinfo->td_ctrl.tdcx_base_size >> PAGE_SHIFT;
	}

AFAICT they are only used when creating/destroying TD for a couple of times, so
I assume doing ">> PAGE_SHIFT" a couple of times won't really matter.

Edgecombe, Rick P Dec. 4, 2024, 1:57 a.m. UTC | #2

On Wed, 2024-12-04 at 01:24 +0000, Huang, Kai wrote:
> Currently the global metadata reading script generates the struct member based
> on the "field name" of the JSON file.  The JSON file stores them as "size":
> 
>   "TDR_BASE_SIZE", "TDCS_BASE_SIZE", "TDVPS_BASE_SIZE"
> 
> We will need to tweak the script to map "metadata field name" to "kernel
> structure member name", and more "special handling for specific fields" when
> auto generating the code.
> 
> It's feasible but I am not sure whether it's worth to do, since we are basically
> talking about converting size to page count.

Ah, right. So given that this is generated code, we should probably just add the
wrappers like you suggest. In any case, we should remove the counts from the new
arch/x86 structs.

> 
> Also, from global metadata's point of view, perhaps it is also good to just
> provide a metadata which is consistent with what module reports.  How kernel
> uses the metadata is another layer on top of it.

I'm not sure I buy this one though. The exported arch/x86 interface shouldn't
have to match the HW directly.

> 
> Btw, perhaps we don't need to keep 'tdcs_nr_pages' and 'tdcx_nr_pages' in
> 'struct tdx_td', i.e., as per-TD variables.  They are constants for all TDX
> guests.
> 
> E.g., assuming KVM is still going to use them, it can just access them using the
> metadata structure:
> 
> 	static inline int tdx_tdcs_nr_pages(void)
> 	{
> 		return tdx_sysinfo->td_ctrl.tdcx_base_size >> PAGE_SHIFT;
> 	}
> 
> AFAICT they are only used when creating/destroying TD for a couple of times, so
> I assume doing ">> PAGE_SHIFT" a couple of times won't really matter.

None of the users are in fast paths. Using page count directly would be more
about reducing wrapper clutter.