| Message ID | 20250115160912.617654-1-pbonzini@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | x86/virt/tdx: Add SEAMCALL wrappers for KVM | expand |
The series looks fine. For the bits that don't have it yet:
Acked-by: Dave Hansen <dave.hansen@linux.intel.com>
On Wed, 2025-01-15 at 11:08 -0500, Paolo Bonzini wrote: > Hi, > > This is the final-ish version of the "SEAMCALL Wrappers" RFC[0], with > all the wrappers extracted out of the corresponding TDX patches. > This version of the series uses u64 only for guest physical addresses > and error return values: > > * u64 pfn is replaced by struct page > > * u64 level is replaced by int level > > * u64 tdr and u64 tdvpr are replaced by structs that contain struct page > for them as well as for tdcs and tdcx. > > A couple functions are also moved over from KVM to tdx.h > > static inline u64 mk_keyed_paddr(u16 hkid, struct page *page) > static inline int pg_level_to_tdx_sept_level(enum pg_level level) > > The plan is to include these in kvm.git together with their first user. It looks like you missed these build issues and bugs from v2: https://lore.kernel.org/kvm/6345272506c5bc707f11b6f54c4bd5015cedcd95.camel@intel.com/ https://lore.kernel.org/kvm/3f8fa8fc98b532add1ff14034c0c868cdbeca7f8.camel@intel.com/
On 1/15/25 20:14, Edgecombe, Rick P wrote: > It looks like you missed these build issues and bugs from v2: > https://lore.kernel.org/ > kvm/6345272506c5bc707f11b6f54c4bd5015cedcd95.camel@intel.com/ > https://lore.kernel.org/ > kvm/3f8fa8fc98b532add1ff14034c0c868cdbeca7f8.camel@intel.com/ I did, I'll update tomorrow and repost. WRT hkid, I interpreted "I'd personally probably just keep 'hkid' as an int everywhere until the point where it gets shoved into the TDX module ABI" as "it can be u16 in the SEAMCALLs and in mk_keyed_paddr" (as the latter builds an argument to the SEAMCALLs). I understood his objection to be more about tdx_guest_keyid_alloc/tdx_guest_keyid_free and struct kvm_tdx: > Oh, and casts like this: > >> static inline void tdx_disassociate_vp(struct kvm_vcpu *vcpu) >> @@ -2354,7 +2354,8 @@ static int __tdx_td_init(struct kvm *kvm, struct td_params *td_params, >> ret = tdx_guest_keyid_alloc(); >> if (ret < 0) >> return ret; >> - kvm_tdx->hkid = ret; >> + kvm_tdx->hkid = (u16)ret; >> + kvm_tdx->hkid_assigned = true; > > are a bit silly, don't you think? so I didn't change tdx_guest_keyid_alloc(). Paolo
On Wed, 2025-01-15 at 20:36 +0100, Paolo Bonzini wrote: > WRT hkid, I interpreted "I'd personally probably just keep 'hkid' as an > int everywhere until the point where it gets shoved into the TDX module > ABI" as "it can be u16 in the SEAMCALLs and in mk_keyed_paddr" (as the > latter builds an argument to the SEAMCALLs). > > I understood his objection to be more about > tdx_guest_keyid_alloc/tdx_guest_keyid_free and struct kvm_tdx: > > > Oh, and casts like this: > > > > > static inline void tdx_disassociate_vp(struct kvm_vcpu *vcpu) > > > @@ -2354,7 +2354,8 @@ static int __tdx_td_init(struct kvm *kvm, struct td_params *td_params, > > > ret = tdx_guest_keyid_alloc(); > > > if (ret < 0) > > > return ret; > > > - kvm_tdx->hkid = ret; > > > + kvm_tdx->hkid = (u16)ret; > > > + kvm_tdx->hkid_assigned = true; > > > > are a bit silly, don't you think? > > so I didn't change tdx_guest_keyid_alloc(). There was a related comment on the GPA union Yan was suggesting: https://lore.kernel.org/kvm/753cd9f1-5eb7-480f-ae4f-d263aaecdd6c@intel.com/ Basically that the bit fields have subtle behavior when you shift them (ironically the exact bug that happened with u16 keyid). But I think your reasoning seems valid, especially since Dave has since quoted that function without commenting on that aspect. So let's leave it.
Hi, This is the final-ish version of the "SEAMCALL Wrappers" RFC[0], with all the wrappers extracted out of the corresponding TDX patches. This version of the series uses u64 only for guest physical addresses and error return values: * u64 pfn is replaced by struct page * u64 level is replaced by int level * u64 tdr and u64 tdvpr are replaced by structs that contain struct page for them as well as for tdcs and tdcx. A couple functions are also moved over from KVM to tdx.h static inline u64 mk_keyed_paddr(u16 hkid, struct page *page) static inline int pg_level_to_tdx_sept_level(enum pg_level level) The plan is to include these in kvm.git together with their first user. Thanks, Paolo Isaku Yamahata (5): x86/virt/tdx: Add SEAMCALL wrapper tdh_mem_sept_add() to add SEPT pages x86/virt/tdx: Add SEAMCALL wrappers to add TD private pages x86/virt/tdx: Add SEAMCALL wrappers to remove a TD private page x86/virt/tdx: Add SEAMCALL wrappers for TD measurement of initial contents x86/virt/tdx: Add tdx_guest_keyid_alloc/free() to alloc and free TDX guest KeyID x86/virt/tdx: Add SEAMCALL wrappers to manage TDX TLB tracking Kai Huang (1): x86/virt/tdx: Add SEAMCALL wrapper to enter/exit TDX guest x86/virt/tdx: Read essential global metadata for KVM 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 | 68 ++++ arch/x86/virt/vmx/tdx/tdx.c | 403 ++++++++++++++++++++ arch/x86/virt/vmx/tdx/tdx.h | 47 ++- arch/x86/virt/vmx/tdx/tdx_global_metadata.c | 50 +++ arch/x86/virt/vmx/tdx/tdx_global_metadata.h | 19 + 5 files changed, 580 insertions(+), 7 deletions(-)