On 26/02/2024 9:25 pm, isaku.yamahata@xxxxxxxxx wrote:
From: Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
TDX requires additional parameters for TDX VM for confidential execution to
protect the confidentiality of its memory contents and CPU state from any
other software, including VMM.
Hmm.. not only "confidentiality" but also "integrity". And the "per-VM"
TDX initializaiton here actually has nothing to do with
"crypto-protection", because the establishment of the key has already
been done before reaching here.
I would just say:
After the crypto-protection key has been configured, TDX requires a
VM-scope initialization as a step of creating the TDX guest. This
"per-VM" TDX initialization does the global configurations/features that
the TDX guest can support, such as guest's CPUIDs (emulated by the TDX
module), the maximum number of vcpus etc.
When creating a guest TD VM before creating
vcpu, the number of vcpu, TSC frequency (the values are the same among
vcpus, and it can't change.) CPUIDs which the TDX module emulates.
I cannot parse this sentence. It doesn't look like a sentence to me.
Guest
TDs can trust those CPUIDs and sha384 values for measurement.
Trustness is not about the "guest can trust", but the "people using the
guest can trust".
Just remove it.
If you want to emphasize the attestation, you can add something like:
"
It also passes the VM's measurement and hash of the signer etc and the
hardware only allows to initialize the TDX guest when that match.
"
Add a new subcommand, KVM_TDX_INIT_VM, to pass parameters for the TDX
guest.
[...]
It assigns an encryption key to the TDX guest for memory
encryption. TDX encrypts memory per guest basis.
No it doesn't. The key has been programmed already in your previous patch.
The device model, say
qemu, passes per-VM parameters for the TDX guest.
This is implied by your first sentence of this paragraph.
The maximum number of
vcpus, TSC frequency (TDX guest has fixed VM-wide TSC frequency, not per
vcpu. The TDX guest can not change it.), attributes (production or debug),
available extended features (which configure guest XCR0, IA32_XSS MSR),
CPUIDs, sha384 measurements, etc.
This is not a sentence.
Call this subcommand before creating vcpu and KVM_SET_CPUID2, i.e. CPUID
configurations aren't available yet.
"
This "per-VM" TDX initialization must be done before any "vcpu-scope"
TDX initialization. To match this better, require the KVM_TDX_INIT_VM
IOCTL() to be done before KVM creates any vcpus.
Note KVM configures the VM's CPUIDs in KVM_SET_CPUID2 via vcpu. The
downside of this approach is KVM will need to do some enforcement later
to make sure the consisntency between the CPUIDs passed here and the
CPUIDs done in KVM_SET_CPUID2.
"
So CPUIDs configuration values need
to be passed in struct kvm_tdx_init_vm. The device model's responsibility
to make this CPUID config for KVM_TDX_INIT_VM and KVM_SET_CPUID2.
And I would leave how to handle KVM_SET_CPUID2 to the patch that
actually enforces the consisntency.
Signed-off-by: Xiaoyao Li <xiaoyao.li@xxxxxxxxx>
Missing Co-developed-by tag for Xiaoyao.
Signed-off-by: Isaku Yamahata <isaku.yamahata@xxxxxxxxx>
[...]
+struct kvm_cpuid_entry2 *kvm_find_cpuid_entry2(
+ struct kvm_cpuid_entry2 *entries, int nent, u32 function, u64 index)
+{
+ return cpuid_entry2_find(entries, nent, function, index);
+}
+EXPORT_SYMBOL_GPL(kvm_find_cpuid_entry2);
Not sure whether we can export cpuid_entry2_find() directly?
No strong opinion of course.
But if we want to expose the wrapper, looks ...
+
struct kvm_cpuid_entry2 *kvm_find_cpuid_entry_index(struct kvm_vcpu *vcpu,
u32 function, u32 index)
{
diff --git a/arch/x86/kvm/cpuid.h b/arch/x86/kvm/cpuid.h
index 856e3037e74f..215d1c68c6d1 100644
--- a/arch/x86/kvm/cpuid.h
+++ b/arch/x86/kvm/cpuid.h
@@ -13,6 +13,8 @@ void kvm_set_cpu_caps(void);
void kvm_update_cpuid_runtime(struct kvm_vcpu *vcpu);
void kvm_update_pv_runtime(struct kvm_vcpu *vcpu);
+struct kvm_cpuid_entry2 *kvm_find_cpuid_entry2(struct kvm_cpuid_entry2 *entries,
+ int nent, u32 function, u64 index);
struct kvm_cpuid_entry2 *kvm_find_cpuid_entry_index(struct kvm_vcpu *vcpu,
u32 function, u32 index); > struct kvm_cpuid_entry2 *kvm_find_cpuid_entry(struct kvm_vcpu *vcpu,
... __kvm_find_cpuid_entry() would fit better?
diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index 1cf2b15da257..b11f105db3cd 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -8,7 +8,6 @@
#include "mmu.h"
#include "tdx_arch.h"
#include "tdx.h"
-#include "tdx_ops.h"
??
If it isn't needed, then it shouldn't be included in some previous patch.
#include "x86.h"
#undef pr_fmt
@@ -350,18 +349,21 @@ static int tdx_do_tdh_mng_key_config(void *param)
return 0;
}
-static int __tdx_td_init(struct kvm *kvm);
-
int tdx_vm_init(struct kvm *kvm)
{
+ /*
+ * This function initializes only KVM software construct. It doesn't
+ * initialize TDX stuff, e.g. TDCS, TDR, TDCX, HKID etc.
+ * It is handled by KVM_TDX_INIT_VM, __tdx_td_init().
+ */
+
/*
* TDX has its own limit of the number of vcpus in addition to
* KVM_MAX_VCPUS.
*/
kvm->max_vcpus = min(kvm->max_vcpus, TDX_MAX_VCPUS);
- /* Place holder for TDX specific logic. */
- return __tdx_td_init(kvm);
+ return 0;
??
I don't quite understand. What's wrong of still calling __tdx_td_init()
in tdx_vm_init()?
If there's anything preventing doing __tdx_td_init() from tdx_vm_init(),
then it's wrong to implement that in your previous patch.
[...]
