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/20241030190039.77971-1-rick.p.edgecombe@xxxxxxxxx/
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(-)
--
2.47.0
