On 1/8/2025 10:40 PM, Sean Christopherson wrote:
On Wed, Jan 08, 2025, Binbin Wu wrote:
On 1/8/2025 3:21 PM, Xiaoyao Li wrote:
On 12/9/2024 9:07 AM, Binbin Wu wrote:
...
---
arch/x86/kvm/lapic.c | 2 +-
arch/x86/kvm/vmx/main.c | 19 ++++++++++++++++++-
2 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kvm/lapic.c b/arch/x86/kvm/lapic.c
index 474e0a7c1069..f93c382344ee 100644
--- a/arch/x86/kvm/lapic.c
+++ b/arch/x86/kvm/lapic.c
@@ -3365,7 +3365,7 @@ int kvm_apic_accept_events(struct kvm_vcpu *vcpu)
if (test_and_clear_bit(KVM_APIC_INIT, &apic->pending_events)) {
kvm_vcpu_reset(vcpu, true);
- if (kvm_vcpu_is_bsp(apic->vcpu))
+ if (kvm_vcpu_is_bsp(vcpu))
vcpu->arch.mp_state = KVM_MP_STATE_RUNNABLE;
else
vcpu->arch.mp_state = KVM_MP_STATE_INIT_RECEIVED;
diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c
index 8ec96646faec..7f933f821188 100644
--- a/arch/x86/kvm/vmx/main.c
+++ b/arch/x86/kvm/vmx/main.c
@@ -115,6 +115,11 @@ static void vt_vcpu_free(struct kvm_vcpu *vcpu)
static void vt_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
{
+ /*
+ * TDX has its own sequence to do init during TD build time (by
+ * KVM_TDX_INIT_VCPU) and it doesn't support INIT event during TD
+ * runtime.
+ */
The first half is confusing. It seems to mix up init(ialization) with INIT
event.
And this callback is about *reset*, which can be due to INIT event or not.
That's why it has a second parameter of init_event. The comment needs to
clarify why reset is not needed for both cases.
I think we can just say TDX doesn't support vcpu reset no matter due to
INIT event or not.
That's not entirely accurate either though. TDX does support KVM's version of
RESET, because KVM's RESET is "power-on", i.e. vCPU creation. Emulation of
runtime RESET is userspace's responsibility.
The real reason why KVM doesn't do anything during KVM's RESET is that what
little setup KVM does/can do needs to be defered until after guest CPUID is
configured.
It's much clear now.
KVM should also WARN if a TDX vCPU gets INIT, no?
Agreed.
Side topic, the comment about x2APIC in tdx_vcpu_init() is too specific, e.g.
calling out that x2APIC support is enumerated in CPUID.0x1.ECX isn't necessary,
and stating that userspace must use KVM_SET_CPUID2 is flat out wrong. Very
technically, KVM_SET_CPUID is also a valid option, it's just not used in practice
because it doesn't support setting non-zero indices (but in theory it could be
used to enable x2APIC).
E.g. something like this?
LGTM.
diff --git a/arch/x86/kvm/vmx/main.c b/arch/x86/kvm/vmx/main.c
index d2e78e6675b9..e36fba94fa14 100644
--- a/arch/x86/kvm/vmx/main.c
+++ b/arch/x86/kvm/vmx/main.c
@@ -115,13 +115,10 @@ static void vt_vcpu_free(struct kvm_vcpu *vcpu)
static void vt_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
{
- /*
- * TDX has its own sequence to do init during TD build time (by
- * KVM_TDX_INIT_VCPU) and it doesn't support INIT event during TD
- * runtime.
- */
- if (is_td_vcpu(vcpu))
+ if (is_td_vcpu(vcpu)) {
+ tdx_vcpu_reset(vcpu, init_event);
return;
+ }
vmx_vcpu_reset(vcpu, init_event);
}
diff --git a/arch/x86/kvm/vmx/tdx.c b/arch/x86/kvm/vmx/tdx.c
index 9e490fccf073..a587f59167a7 100644
--- a/arch/x86/kvm/vmx/tdx.c
+++ b/arch/x86/kvm/vmx/tdx.c
@@ -2806,9 +2806,8 @@ static int tdx_vcpu_init(struct kvm_vcpu *vcpu, struct kvm_tdx_cmd *cmd)
return -EINVAL;
/*
- * As TDX requires X2APIC, set local apic mode to X2APIC. User space
- * VMM, e.g. qemu, is required to set CPUID[0x1].ecx.X2APIC=1 by
- * KVM_SET_CPUID2. Otherwise kvm_apic_set_base() will fail.
+ * TDX requires x2APIC, userspace is responsible for configuring guest
+ * CPUID accordingly.
*/
apic_base = APIC_DEFAULT_PHYS_BASE | LAPIC_MODE_X2APIC |
(kvm_vcpu_is_reset_bsp(vcpu) ? MSR_IA32_APICBASE_BSP : 0);
@@ -2827,6 +2826,19 @@ static int tdx_vcpu_init(struct kvm_vcpu *vcpu, struct kvm_tdx_cmd *cmd)
return 0;
}
+void tdx_vcpu_reset(struct kvm_vcpu *vcpu, bool init_event)
+{
+ /*
+ * Yell on INIT, as TDX doesn't support INIT, i.e. KVM should drop all
+ * INIT events.
+ *
+ * Defer initializing vCPU for RESET state until KVM_TDX_INIT_VCPU, as
+ * userspace needs to define the vCPU model before KVM can initialize
+ * vCPU state, e.g. to enable x2APIC.
+ */
+ WARN_ON_ONCE(init_event);
+}
+
struct tdx_gmem_post_populate_arg {
struct kvm_vcpu *vcpu;
__u32 flags;
