| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <489f3c7b-db03-43dc-bb64-910a0fcba31e@intel.com>
Date: Thu, 30 Oct 2025 21:06:21 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Sagi Shahar <sagis@...gle.com>, <linux-kselftest@...r.kernel.org>, "Paolo
Bonzini" <pbonzini@...hat.com>, Shuah Khan <shuah@...nel.org>, "Sean
Christopherson" <seanjc@...gle.com>, Ackerley Tng <ackerleytng@...gle.com>,
Ryan Afranji <afranji@...gle.com>, Andrew Jones <ajones@...tanamicro.com>,
Isaku Yamahata <isaku.yamahata@...el.com>, Erdem Aktas
<erdemaktas@...gle.com>, Rick Edgecombe <rick.p.edgecombe@...el.com>, "Roger
Wang" <runanwang@...gle.com>, Binbin Wu <binbin.wu@...ux.intel.com>, "Oliver
Upton" <oliver.upton@...ux.dev>, "Pratik R. Sampat"
<pratikrajesh.sampat@....com>, Ira Weiny <ira.weiny@...el.com>, Chao Gao
<chao.gao@...el.com>, Chenyi Qiang <chenyi.qiang@...el.com>
CC: <linux-kernel@...r.kernel.org>, <kvm@...r.kernel.org>
Subject: Re: [PATCH v12 12/23] KVM: selftests: Add helper to initialize TDX VM
Hi Sagi,
On 10/28/25 2:20 PM, Sagi Shahar wrote:
> diff --git a/tools/testing/selftests/kvm/include/x86/tdx/tdx_util.h b/tools/testing/selftests/kvm/include/x86/tdx/tdx_util.h
> index dafdc7e46abe..a2509959c7ce 100644
> --- a/tools/testing/selftests/kvm/include/x86/tdx/tdx_util.h
> +++ b/tools/testing/selftests/kvm/include/x86/tdx/tdx_util.h
> @@ -11,6 +11,60 @@ static inline bool is_tdx_vm(struct kvm_vm *vm)
> return vm->type == KVM_X86_TDX_VM;
> }
>
> +/*
> + * TDX ioctls
> + */
> +
> +#define __vm_tdx_vm_ioctl(vm, cmd, metadata, arg) \
> +({ \
> + int r; \
> + \
> + union { \
> + struct kvm_tdx_cmd c; \
> + unsigned long raw; \
> + } tdx_cmd = { .c = { \
> + .id = (cmd), \
> + .flags = (uint32_t)(metadata), \
> + .data = (uint64_t)(arg), \
> + } }; \
> + \
> + r = __vm_ioctl(vm, KVM_MEMORY_ENCRYPT_OP, &tdx_cmd.raw); \
> + r ?: tdx_cmd.c.hw_error; \
> +})
> +
> +#define vm_tdx_vm_ioctl(vm, cmd, flags, arg) \
> +({ \
> + int ret = __vm_tdx_vm_ioctl(vm, cmd, flags, arg); \
> + \
> + __TEST_ASSERT_VM_VCPU_IOCTL(!ret, #cmd, ret, vm); \
> +})
> +
> +#define __vm_tdx_vcpu_ioctl(vcpu, cmd, metadata, arg) \
> +({ \
> + int r; \
> + \
> + union { \
> + struct kvm_tdx_cmd c; \
> + unsigned long raw; \
> + } tdx_cmd = { .c = { \
> + .id = (cmd), \
> + .flags = (uint32_t)(metadata), \
> + .data = (uint64_t)(arg), \
> + } }; \
> + \
> + r = __vcpu_ioctl(vcpu, KVM_MEMORY_ENCRYPT_OP, &tdx_cmd.raw); \
> + r ?: tdx_cmd.c.hw_error; \
> +})
> +
> +#define vm_tdx_vcpu_ioctl(vcpu, cmd, flags, arg) \
> +({ \
> + int ret = __vm_tdx_vcpu_ioctl(vcpu, cmd, flags, arg); \
> + \
> + __TEST_ASSERT_VM_VCPU_IOCTL(!ret, #cmd, ret, (vcpu)->vm); \
> +})
> +
> +void vm_tdx_init_vm(struct kvm_vm *vm, uint64_t attributes);
> +
> void vm_tdx_setup_boot_code_region(struct kvm_vm *vm);
> void vm_tdx_setup_boot_parameters_region(struct kvm_vm *vm, uint32_t nr_runnable_vcpus);
> void vm_tdx_load_common_boot_parameters(struct kvm_vm *vm);
For completeness to help with discussion below other patches add:
void vm_tdx_load_vcpu_boot_parameters(struct kvm_vm *vm, struct kvm_vcpu *vcpu);
void vm_tdx_set_vcpu_entry_point(struct kvm_vcpu *vcpu, void *guest_code);
void vm_tdx_finalize(struct kvm_vm *vm);
When considering the TDX functions in tdx_util.h visible above the namespace of
TDX related functions is not clear to me. I believe an intuitive namespace
makes the code easier to understand and build upon.
Almost all tdx_util.h functions appear to have the "vm_tdx" prefix even when they just operate on a vCPU scope,
for example:
void vm_tdx_set_vcpu_entry_point(struct kvm_vcpu *vcpu, void *guest_code);
and
vm_tdx_vcpu_ioctl()
Also, when operating on a VM there may be an extra "vm" added to create a function like
vm_tdx_vm_ioctl() with two "vm" in its name.
Compare with similar functions for normal VMs:
vm_ioctl() -> vm_tdx_vm_ioctl()
vcpu_ioctl() -> vm_tdx_vcpu_ioctl()
Could it not perhaps instead be:
vm_ioctl() -> tdx_vm_ioctl()
vcpu_ioctl() -> tdx_vcpu_ioctl()
The functions could still have "vm"/"vcpu" in their name to designate the scope, for example:
void tdx_vm_setup_boot_code_region(struct kvm_vm *vm);
void tdx_vm_setup_boot_parameters_region(struct kvm_vm *vm, uint32_t nr_runnable_vcpus);
void tdx_vm_load_common_boot_parameters(struct kvm_vm *vm);
void tdx_vcpu_load_boot_parameters(struct kvm_vm *vm, struct kvm_vcpu *vcpu);
void tdx_vcpu_set_entry_point(struct kvm_vcpu *vcpu, void *guest_code);
void tdx_vm_finalize(struct kvm_vm *vm);
With a namespace like above it is clear that (a) it is a TDX call and (b) what the scope of the
call is. This helps to understand what the code does while reading it and makes clear how to
name new functions when adding new features.
...
> +/*
> + * Filter CPUID based on TDX supported capabilities
> + *
> + * Input Args:
> + * vm - Virtual Machine
> + * cpuid_data - CPUID fileds to filter
fileds -> fields?
> + *
> + * Output Args: None
> + *
> + * Return: None
> + *
> + * For each CPUID leaf, filter out non-supported bits based on the capabilities reported
> + * by the TDX module
> + */
> +static void vm_tdx_filter_cpuid(struct kvm_vm *vm,
> + struct kvm_cpuid2 *cpuid_data)
> +{
> + struct kvm_tdx_capabilities *tdx_cap;
> + struct kvm_cpuid_entry2 *config;
> + struct kvm_cpuid_entry2 *e;
> + int i;
> +
> + tdx_cap = tdx_read_capabilities(vm);
> +
> + i = 0;
> + while (i < cpuid_data->nent) {
> + e = cpuid_data->entries + i;
> + config = tdx_find_cpuid_config(tdx_cap, e->function, e->index);
> +
> + if (!config) {
> + int left = cpuid_data->nent - i - 1;
> +
> + if (left > 0)
> + memmove(cpuid_data->entries + i,
> + cpuid_data->entries + i + 1,
> + sizeof(*cpuid_data->entries) * left);
> + cpuid_data->nent--;
> + continue;
> + }
> +
> + e->eax &= config->eax;
> + e->ebx &= config->ebx;
> + e->ecx &= config->ecx;
> + e->edx &= config->edx;
> +
> + i++;
> + }
> +
> + free(tdx_cap);
> +}
> +
> +void vm_tdx_init_vm(struct kvm_vm *vm, uint64_t attributes)
> +{
> + struct kvm_tdx_init_vm *init_vm;
> + const struct kvm_cpuid2 *tmp;
> + struct kvm_cpuid2 *cpuid;
> +
> + tmp = kvm_get_supported_cpuid();
> +
> + cpuid = allocate_kvm_cpuid2(MAX_NR_CPUID_ENTRIES);
Could this allocation be limited to tmp->nent?
> + memcpy(cpuid, tmp, kvm_cpuid2_size(tmp->nent));
> + vm_tdx_filter_cpuid(vm, cpuid);
> +
> + init_vm = calloc(1, sizeof(*init_vm) +
> + sizeof(init_vm->cpuid.entries[0]) * cpuid->nent);
> + TEST_ASSERT(init_vm, "init_vm allocation failed");
> +
> + memcpy(&init_vm->cpuid, cpuid, kvm_cpuid2_size(cpuid->nent));
> + free(cpuid);
> +
> + init_vm->attributes = attributes;
> +
> + vm_tdx_vm_ioctl(vm, KVM_TDX_INIT_VM, 0, init_vm);
> +
> + free(init_vm);
> +}
Reinette
Powered by blists - more mailing lists