Re: [RFC PATCH v2 06/22] KVM: X86: Define tsm_get_vmid

Alexey Kardashevskiy <aik@xxxxxxx> · Fri, 14 Mar 2025 14:28:00 +1100

On 14/3/25 06:09, Dan Williams wrote:
Alexey Kardashevskiy wrote:


On 13/3/25 12:51, Dan Williams wrote:
Alexey Kardashevskiy wrote:
In order to add a PCI VF into a secure VM, the TSM module needs to
perform a "TDI bind" operation. The secure module ("PSP" for AMD)
reuqires a VM id to associate with a VM and KVM has it. Since
KVM cannot directly bind a TDI (as it does not have all necesessary
data such as host/guest PCI BDFn). QEMU and IOMMUFD do know the BDFns
but they do not have a VM id recognisable by the PSP.

Add get_vmid() hook to KVM. Implement it for AMD SEV to return a sum
of GCTX (a private page describing secure VM context) and ASID
(required on unbind for IOMMU unfencing, when needed).

Signed-off-by: Alexey Kardashevskiy <aik@xxxxxxx>
---
   arch/x86/include/asm/kvm-x86-ops.h |  1 +
   arch/x86/include/asm/kvm_host.h    |  2 ++
   include/linux/kvm_host.h           |  2 ++
   arch/x86/kvm/svm/svm.c             | 12 ++++++++++++
   virt/kvm/kvm_main.c                |  6 ++++++
   5 files changed, 23 insertions(+)

diff --git a/arch/x86/include/asm/kvm-x86-ops.h b/arch/x86/include/asm/kvm-x86-ops.h
index c35550581da0..63102a224cd7 100644
--- a/arch/x86/include/asm/kvm-x86-ops.h
+++ b/arch/x86/include/asm/kvm-x86-ops.h
@@ -144,6 +144,7 @@ KVM_X86_OP_OPTIONAL(alloc_apic_backing_page)
   KVM_X86_OP_OPTIONAL_RET0(gmem_prepare)
   KVM_X86_OP_OPTIONAL_RET0(private_max_mapping_level)
   KVM_X86_OP_OPTIONAL(gmem_invalidate)
+KVM_X86_OP_OPTIONAL(tsm_get_vmid)
   
   #undef KVM_X86_OP
   #undef KVM_X86_OP_OPTIONAL
diff --git a/arch/x86/include/asm/kvm_host.h b/arch/x86/include/asm/kvm_host.h
index b15cde0a9b5c..9330e8d4d29d 100644
--- a/arch/x86/include/asm/kvm_host.h
+++ b/arch/x86/include/asm/kvm_host.h
@@ -1875,6 +1875,8 @@ struct kvm_x86_ops {
   	int (*gmem_prepare)(struct kvm *kvm, kvm_pfn_t pfn, gfn_t gfn, int max_order);
   	void (*gmem_invalidate)(kvm_pfn_t start, kvm_pfn_t end);
   	int (*private_max_mapping_level)(struct kvm *kvm, kvm_pfn_t pfn);
+
+	u64 (*tsm_get_vmid)(struct kvm *kvm);
   };
   
   struct kvm_x86_nested_ops {
diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h
index f34f4cfaa513..6cd351edb956 100644
--- a/include/linux/kvm_host.h
+++ b/include/linux/kvm_host.h
@@ -2571,4 +2571,6 @@ long kvm_arch_vcpu_pre_fault_memory(struct kvm_vcpu *vcpu,
   				    struct kvm_pre_fault_memory *range);
   #endif
   
+u64 kvm_arch_tsm_get_vmid(struct kvm *kvm);
+
   #endif
diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
index 7640a84e554a..0276d60c61d6 100644
--- a/arch/x86/kvm/svm/svm.c
+++ b/arch/x86/kvm/svm/svm.c
@@ -4998,6 +4998,16 @@ static void *svm_alloc_apic_backing_page(struct kvm_vcpu *vcpu)
   	return page_address(page);
   }
   
+static u64 svm_tsm_get_vmid(struct kvm *kvm)
+{
+	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
+
+	if (!sev->es_active)
+		return 0;
+
+	return ((u64) sev->snp_context) | sev->asid;
+}
+

Curious why KVM needs to be bothered by a new kvm_arch_tsm_get_vmid()
and a vendor specific cookie "vmid" concept. In other words KVM never
calls kvm_arch_tsm_get_vmid(), like other kvm_arch_*() support calls.

Is this due to a restriction that something like tsm_tdi_bind() is
disallowed from doing to_kvm_svm() on an opaque @kvm pointer? Or
otherwise asking an arch/x86/kvm/svm/svm.c to do the same?

I saw someone already doing some sort of VMID thing

Reference?

Cannot find it now, RiscV and ARM have this concept but internally and 
it probably should stay this way.

and thought it is a good way of not spilling KVM details outside KVM.

...but it is not a KVM detail. It is an arch specific TSM cookie derived
from arch specific data that wraps 'struct kvm'. Now if the rationale is
some least privelege concern about what code can have a container_of()
relationship with an opaque 'struct kvm *' pointer, let's have that
discussion.  As it stands nothing in KVM cares about
kvm_arch_tsm_get_vmid(), and I expect 'vmid' does not cover all the ways
in which modular TSM drivers may interact with arch/.../kvm/ code.

For example TDX Connect needs to share some data from 'struct kvm_tdx',
and it does that with an export from arch/x86/kvm/vmx/tdx.c, not an
indirection through virt/kvm/kvm_main.c.

a) KVM-AMD uses CCP's exports now, and if I add exports to KVM-AMD for 
CCP - it is cross-reference so I'll need what, KVM-AMD-TIO module, to 
untangle this?

b) I could include arch/x86/kvm/svm/svm.h in drivers/crypto/ccp/ which 
is... meh?

c) Or move parts of struct kvm_sev_info/kvm_svm from 
arch/x86/kvm/svm/svm.h to arch/x86/include/asm/svm.h and do some trick 
to get kvm_sev_info from struct kvm.

d) In my RFC v1, I simply called tsm_tdi_bind() from KVM-AMD with this 
cookie but that assumed KVM knowledge of PCI which I dropped in this RFC 
so the bind request travels via QEMU between the guest and the PSP.

All doable though.

Effectively low level TSM drivers are extensions of arch code that
routinely performs "container_of(kvm, struct kvm_$arch, kvm)".

The arch code is CCP and so far it avoided touching KVM, KVM calls CCP
when it needs but not vice versa. Thanks,

Right, and the observation is that you don't need to touch
virt/kvm/kvm_main.c at all to meet this data sharing requirement.

These are all valid points. I like neither of a)..d) in particular and I 
am AMD-centric (as you correctly noticed :) ) and for this exercise I 
only needed kvmfd->guest_context_page, hence this proposal. Thanks,


--
Alexey








