[RFC,0/6] SEAMCALL Wrappers

Message

Rick Edgecombe Nov. 15, 2024, 8:20 p.m. UTC

Hi,

This is a quick followup to Dave’s comments on the SEAMCALL wrappers in 
the “TDX vCPU/VM creation” series [0]. 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, like pages. There were a 
couple directions discussed for passing the pages that are handed to TDX 
module for it to use to manage a TD state:
 - Using virtual addresses to host mappings for the page
 - Using structs for each type of physical page, such that there could be
   type checking on all of the similar tdXYZ TDX page types.
 - Pulling in “linux/kvm_types.h” in arch/x86 code to handle types 
   (especially since the later MMU SEAMCALLs take GFN arguments)

There was also some repeated points made that the argument names could use 
some semantic clarity, including possible creating enums for out args.

But overall, I interpreted there to be a wish for the wrappers to do 
something a little more useful than use the EXPORT infrastructure as an 
allow list for approved SEAMCALLs.

I first played around with basically keeping the existing design and using 
KVM’s hpa_t, etc for the types. This really didn’t add much improvement. 
Using virtual addresses simplified some of the code on the KVM side, but 
contrasted with KVM code style that is used to handling physical addresses 
for most things. It also was weird considering that these pages are 
encrypted and can’t be used from the kernel.

The solution in this RFC
------------------------
As we discussed on that series, the SEAMCALL wrappers used to take the KVM 
defined structs that represent TDs and vCPUs, instead of raw tdXYZ page 
references. This was pretty handy and good for avoiding passing the wrong 
type of TDX tdXYZ page, but these structs have a bunch of other stuff that 
is specific to KVM. We don’t want to leak that stuff outside of KVM. But I 
think it is ok for the arch/x86 code to know about TDX arch specific things
that VMMs are generally required to have to manage. So this RFC creates
two structs that represent TDs and vCPUs. They hold references to the tdXYZ
pages that are provided to the TDX module and associated with there 
respective architectural concept (TD and vCPU):
	struct tdx_td {
		hpa_t tdr;
		hpa_t *tdcs;
	};

	struct tdx_vp {
		hpa_t tdvpr;
		hpa_t *tdcx;
	};

I used hpa_t based on that it is commonly used to hold physical addresses 
in KVM, including similar kernel allocated memory like TDP root pages. So 
I thought it fit KVM better, stylistically. It was my best attempt at 
guessing what KVM maintainers would like. Other options could be 
kvm_pfn_t. Or just struct page.

They are passed into the SEAMCALL wrappers like:
	u64 tdh_mng_vpflushdone(struct tdx_td *td)

These new structs are then placed inside KVM’s structs that track TDX 
state for the same concept, where previously those KVM structs held the 
references to the pages directly, like:
struct kvm_tdx {
	struct kvm kvm;

-	u64 tdr;
-	u64 *tdcs;
+	struct tdx_td td;
	...
};

It does get a bit nested though, for example:
	kvm->kvm_arch->(container_of)kvm_tdx->tdx_td->tdr
...but the actual final dereferences in the code don’t need to go that
deep, and look like:
	err = tdh_mng_vpflushdone(&kvm_tdx->td);

Overall, it's not a huge change, but does give the arch/x86 a little more
purpose. Please let me know what you think.

There also was a suggestion from Dave to create a helper to hold a comment 
on the “CLFLUSH_BEFORE_ALLOC” reasoning. This is implemented in this RFC.

Separate from discussions with Dave on the SEAMCALLs, there was some some 
suggestions on how we might remove or combine specific SEAMCALLs. I didn’t 
try this here, because this RFC is more about exploring in general how we 
want to distribute things between KVM and arch/x86 for these SEAMCALL 
wrappers.

So in summary the RFC only has:
 - Use structs to hold tdXYZ fields for TD and vCPUs
 - Make helper to hold CLFLUSH_BEFORE_ALLOC comments
 - Use semantic names for out args
 - (Add Kai's sign-off that should have been in the last version)
  
Patches 1 and 3 contain new commit log verbiage justifying specific design
choices behind the struct definitions.

I didn’t create enums for the out args. Just using proper names for the 
args seemed like a good balance between code clarity and not 
over-engineering. But please correct if this was the wrong judgment.

Here is a branch for seeing the callers. I didn’t squash the caller 
changes into the patches yet either, the caller changes are all just in the
HEAD commit. I also only converted the “VM/vCPU creation” SEAMCALLs to the
approach described above:
https://github.com/intel/tdx/tree/seamcall-rfc

[0] 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  |  29 +++++
 arch/x86/virt/vmx/tdx/tdx.c | 224 ++++++++++++++++++++++++++++++++++++
 arch/x86/virt/vmx/tdx/tdx.h |  38 ++++--
 3 files changed, 284 insertions(+), 7 deletions(-)