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

[Dan Williams <dan.j.williams@intel.com>]

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@amd.com>
> >> ---
> >>   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?

> 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.

> > 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.