KVM TDX basic feature support

From: Isaku Yamahata <[email protected]>

As TDX will use posted_interrupt.c, the use of struct vcpu_vmx is a blocker. Because the members of

struct pi_desc pi_desc and struct
list_head pi_wakeup_list are only used in posted_interrupt.c, introduce common structure, struct vcpu_pi, make vcpu_vmx and vcpu_tdx has same layout in the top of structure.

To minimize the diff size, avoid code conversion like, vmx->pi_desc => vmx->common->pi_desc. Instead add compile time check if the layout is expected.

Signed-off-by Isaku Yamahata <[email protected]>

---
  arch/x86/kvm/vmx/posted_intr.c | 41 ++++++++++++++++++++++++++--------
  arch/x86/kvm/vmx/posted_intr.h | 11 +++++++++
  arch/x86/kvm/vmx/tdx.c         |  1 +
  arch/x86/kvm/vmx/tdx.h         |  8 +++++++
  arch/x86/kvm/vmx/vmx.h         | 14 +++++++-----
  5 files changed, 60 insertions(+), 15 deletions(-)

arch/x86/kvm/vmx/posted_intr.c +32 -9

index af662312fd07..b66add9da0f3 100644
@@ -11,6 +11,7 @@
  #include "posted_intr.h"
  #include "trace.h"
  #include "vmx.h"
+#include "tdx.h"
 
  /*
   * Maintain a per-CPU list of vCPUs that need to be awakened by wakeup_handler()

@@ -31,9 +32,29 @@ static DEFINE_PER_CPU(struct list_head, wakeup_vcpus_on_cpu);
   */
  static DEFINE_PER_CPU(raw_spinlock_t, wakeup_vcpus_on_cpu_lock);
 
+/*
+ * The layout of the head of struct vcpu_vmx and struct vcpu_tdx must match with
+ * struct vcpu_pi.
+ */
+static_assert(offsetof(struct vcpu_pi, pi_desc) ==
+	      offsetof(struct vcpu_vmx, pi_desc));
+static_assert(offsetof(struct vcpu_pi, pi_wakeup_list) ==
+	      offsetof(struct vcpu_vmx, pi_wakeup_list));
+#ifdef CONFIG_INTEL_TDX_HOST
+static_assert(offsetof(struct vcpu_pi, pi_desc) ==
+	      offsetof(struct vcpu_tdx, pi_desc));
+static_assert(offsetof(struct vcpu_pi, pi_wakeup_list) ==
+	      offsetof(struct vcpu_tdx, pi_wakeup_list));
+#endif
+
+static inline struct vcpu_pi *vcpu_to_pi(struct kvm_vcpu *vcpu)
+{
+	return (struct vcpu_pi *)vcpu;
+}
+
  static inline struct pi_desc *vcpu_to_pi_desc(struct kvm_vcpu *vcpu)
  {
-	return &(to_vmx(vcpu)->pi_desc);
+	return &vcpu_to_pi(vcpu)->pi_desc;
  }
 
  static int pi_try_set_control(struct pi_desc *pi_desc, u64 *pold, u64 new)

@@ -52,8 +73,8 @@ static int pi_try_set_control(struct pi_desc *pi_desc, u64 *pold, u64 new)
 
  void vmx_vcpu_pi_load(struct kvm_vcpu *vcpu, int cpu)
  {
-	struct pi_desc *pi_desc = vcpu_to_pi_desc(vcpu);
-	struct vcpu_vmx *vmx = to_vmx(vcpu);
+	struct vcpu_pi *vcpu_pi = vcpu_to_pi(vcpu);
+	struct pi_desc *pi_desc = &vcpu_pi->pi_desc;
  	struct pi_desc old, new;
  	unsigned long flags;
  	unsigned int dest;

@@ -90,7 +111,7 @@ void vmx_vcpu_pi_load(struct kvm_vcpu *vcpu, int cpu)
  	 */
  	if (pi_desc->nv == POSTED_INTR_WAKEUP_VECTOR) {
  		raw_spin_lock(&per_cpu(wakeup_vcpus_on_cpu_lock, vcpu->cpu));
-		list_del(&vmx->pi_wakeup_list);
+		list_del(&vcpu_pi->pi_wakeup_list);
  		raw_spin_unlock(&per_cpu(wakeup_vcpus_on_cpu_lock, vcpu->cpu));
  	}
 

@@ -145,15 +166,15 @@ static bool vmx_can_use_vtd_pi(struct kvm *kvm)
   */
  static void pi_enable_wakeup_handler(struct kvm_vcpu *vcpu)
  {
-	struct pi_desc *pi_desc = vcpu_to_pi_desc(vcpu);
-	struct vcpu_vmx *vmx = to_vmx(vcpu);
+	struct vcpu_pi *vcpu_pi = vcpu_to_pi(vcpu);
+	struct pi_desc *pi_desc = &vcpu_pi->pi_desc;
  	struct pi_desc old, new;
  	unsigned long flags;
 
  	local_irq_save(flags);
 
  	raw_spin_lock(&per_cpu(wakeup_vcpus_on_cpu_lock, vcpu->cpu));
-	list_add_tail(&vmx->pi_wakeup_list,
+	list_add_tail(&vcpu_pi->pi_wakeup_list,
  		      &per_cpu(wakeup_vcpus_on_cpu, vcpu->cpu));
  	raw_spin_unlock(&per_cpu(wakeup_vcpus_on_cpu_lock, vcpu->cpu));
 

@@ -190,7 +211,8 @@ static bool vmx_needs_pi_wakeup(struct kvm_vcpu *vcpu)
  	 * notification vector is switched to the one that calls
  	 * back to the pi_wakeup_handler() function.
  	 */
-	return vmx_can_use_ipiv(vcpu) || vmx_can_use_vtd_pi(vcpu->kvm);
+	return (vmx_can_use_ipiv(vcpu) && !is_td_vcpu(vcpu)) ||
+		vmx_can_use_vtd_pi(vcpu->kvm);
  }
 
  void vmx_vcpu_pi_put(struct kvm_vcpu *vcpu)

@@ -200,7 +222,8 @@ void vmx_vcpu_pi_put(struct kvm_vcpu *vcpu)
  	if (!vmx_needs_pi_wakeup(vcpu))
  		return;
 
-	if (kvm_vcpu_is_blocking(vcpu) && !vmx_interrupt_blocked(vcpu))
+	if (kvm_vcpu_is_blocking(vcpu) &&
+	    (is_td_vcpu(vcpu) || !vmx_interrupt_blocked(vcpu)))
  		pi_enable_wakeup_handler(vcpu);
 
  	/*

arch/x86/kvm/vmx/posted_intr.h +0 -0

index 26992076552e..2fe8222308b2 100644
@@ -94,6 +94,17 @@ static inline bool pi_test_sn(struct pi_desc *pi_desc)
  			(unsigned long *)&pi_desc->control);
  }

arch/x86/kvm/vmx/posted_intr.h +2 -0

+};
+
  void vmx_vcpu_pi_load(struct kvm_vcpu *vcpu, int cpu);
  void vmx_vcpu_pi_put(struct kvm_vcpu *vcpu);
  void pi_wakeup_handler(void);

arch/x86/kvm/vmx/tdx.c +1 -0

index a5b52aa6d153..1da58c36217c 100644
@@ -584,6 +584,7 @@ int tdx_vcpu_create(struct kvm_vcpu *vcpu)
 
  	fpstate_set_confidential(&vcpu->arch.guest_fpu);
  	vcpu->arch.apic->guest_apic_protected = true;
+	INIT_LIST_HEAD(&tdx->pi_wakeup_list);
 
  	vcpu->arch.efer = EFER_SCE | EFER_LME | EFER_LMA | EFER_NX;
 

arch/x86/kvm/vmx/tdx.h +8 -0

index 7f8c78f06508..eaffa7384725 100644
@@ -4,6 +4,7 @@
 
  #ifdef CONFIG_INTEL_TDX_HOST
 
+#include "posted_intr.h"
  #include "pmu_intel.h"
  #include "tdx_ops.h"
 

@@ -69,6 +70,13 @@ union tdx_exit_reason {
  struct vcpu_tdx {
  	struct kvm_vcpu	vcpu;
 
+	/* Posted interrupt descriptor */
+	struct pi_desc pi_desc;
+
+	/* Used if this vCPU is waiting for PI notification wakeup. */
+	struct list_head pi_wakeup_list;
+	/* Until here same layout to struct vcpu_pi. */
+
  	unsigned long tdvpr_pa;
  	unsigned long *tdvpx_pa;
  	bool td_vcpu_created;

arch/x86/kvm/vmx/vmx.h +8 -6

index 79ff54f08fee..634a9a250b95 100644
@@ -235,6 +235,14 @@ struct nested_vmx {
 
  struct vcpu_vmx {
  	struct kvm_vcpu       vcpu;
+
+	/* Posted interrupt descriptor */
+	struct pi_desc pi_desc;
+
+	/* Used if this vCPU is waiting for PI notification wakeup. */
+	struct list_head pi_wakeup_list;
+	/* Until here same layout to struct vcpu_pi. */
+
  	u8                    fail;
  	u8		      x2apic_msr_bitmap_mode;
 

@@ -304,12 +312,6 @@ struct vcpu_vmx {
 
  	union vmx_exit_reason exit_reason;
 
-	/* Posted interrupt descriptor */
-	struct pi_desc pi_desc;
-
-	/* Used if this vCPU is waiting for PI notification wakeup. */
-	struct list_head pi_wakeup_list;
-
  	/* Support for a guest hypervisor (nested VMX) */
  	struct nested_vmx nested;
 

Quoted 4 lines · On 2/26/2024 4:26 PM, [email protected] wrote:

From: Isaku Yamahata <[email protected]>

As TDX will use posted_interrupt.c, the use of struct vcpu_vmx is a blocker. Because the members of

Extra "of"

Quoted 115 lines

struct pi_desc pi_desc and struct
list_head pi_wakeup_list are only used in posted_interrupt.c, introduce common structure, struct vcpu_pi, make vcpu_vmx and vcpu_tdx has same layout in the top of structure.

To minimize the diff size, avoid code conversion like, vmx->pi_desc => vmx->common->pi_desc. Instead add compile time check if the layout is expected.

Signed-off-by Isaku Yamahata <[email protected]>

---
  arch/x86/kvm/vmx/posted_intr.c | 41 ++++++++++++++++++++++++++--------
  arch/x86/kvm/vmx/posted_intr.h | 11 +++++++++
  arch/x86/kvm/vmx/tdx.c         |  1 +
  arch/x86/kvm/vmx/tdx.h         |  8 +++++++
  arch/x86/kvm/vmx/vmx.h         | 14 +++++++-----
  5 files changed, 60 insertions(+), 15 deletions(-)

arch/x86/kvm/vmx/posted_intr.c +32 -9

index af662312fd07..b66add9da0f3 100644
@@ -11,6 +11,7 @@
  #include "posted_intr.h"
  #include "trace.h"
  #include "vmx.h"
+#include "tdx.h"
  /*
   * Maintain a per-CPU list of vCPUs that need to be awakened by wakeup_handler()

@@ -31,9 +32,29 @@ static DEFINE_PER_CPU(struct list_head, wakeup_vcpus_on_cpu);
   */
  static DEFINE_PER_CPU(raw_spinlock_t, wakeup_vcpus_on_cpu_lock);
+/*
+ * The layout of the head of struct vcpu_vmx and struct vcpu_tdx must match with
+ * struct vcpu_pi.
+ */
+static_assert(offsetof(struct vcpu_pi, pi_desc) ==
+	      offsetof(struct vcpu_vmx, pi_desc));
+static_assert(offsetof(struct vcpu_pi, pi_wakeup_list) ==
+	      offsetof(struct vcpu_vmx, pi_wakeup_list));
+#ifdef CONFIG_INTEL_TDX_HOST
+static_assert(offsetof(struct vcpu_pi, pi_desc) ==
+	      offsetof(struct vcpu_tdx, pi_desc));
+static_assert(offsetof(struct vcpu_pi, pi_wakeup_list) ==
+	      offsetof(struct vcpu_tdx, pi_wakeup_list));
+#endif
+
+static inline struct vcpu_pi *vcpu_to_pi(struct kvm_vcpu *vcpu)
+{
+	return (struct vcpu_pi *)vcpu;
+}
+
  static inline struct pi_desc *vcpu_to_pi_desc(struct kvm_vcpu *vcpu)
  {
-	return &(to_vmx(vcpu)->pi_desc);
+	return &vcpu_to_pi(vcpu)->pi_desc;
  }
  static int pi_try_set_control(struct pi_desc *pi_desc, u64 *pold, u64 new)

@@ -52,8 +73,8 @@ static int pi_try_set_control(struct pi_desc *pi_desc, u64 *pold, u64 new)
  void vmx_vcpu_pi_load(struct kvm_vcpu *vcpu, int cpu)
  {
-	struct pi_desc *pi_desc = vcpu_to_pi_desc(vcpu);
-	struct vcpu_vmx *vmx = to_vmx(vcpu);
+	struct vcpu_pi *vcpu_pi = vcpu_to_pi(vcpu);
+	struct pi_desc *pi_desc = &vcpu_pi->pi_desc;
  	struct pi_desc old, new;
  	unsigned long flags;
  	unsigned int dest;

@@ -90,7 +111,7 @@ void vmx_vcpu_pi_load(struct kvm_vcpu *vcpu, int cpu)
  	 */
  	if (pi_desc->nv == POSTED_INTR_WAKEUP_VECTOR) {
  		raw_spin_lock(&per_cpu(wakeup_vcpus_on_cpu_lock, vcpu->cpu));
-		list_del(&vmx->pi_wakeup_list);
+		list_del(&vcpu_pi->pi_wakeup_list);
  		raw_spin_unlock(&per_cpu(wakeup_vcpus_on_cpu_lock, vcpu->cpu));
  	}

@@ -145,15 +166,15 @@ static bool vmx_can_use_vtd_pi(struct kvm *kvm)
   */
  static void pi_enable_wakeup_handler(struct kvm_vcpu *vcpu)
  {
-	struct pi_desc *pi_desc = vcpu_to_pi_desc(vcpu);
-	struct vcpu_vmx *vmx = to_vmx(vcpu);
+	struct vcpu_pi *vcpu_pi = vcpu_to_pi(vcpu);
+	struct pi_desc *pi_desc = &vcpu_pi->pi_desc;
  	struct pi_desc old, new;
  	unsigned long flags;
  	local_irq_save(flags);
  	raw_spin_lock(&per_cpu(wakeup_vcpus_on_cpu_lock, vcpu->cpu));
-	list_add_tail(&vmx->pi_wakeup_list,
+	list_add_tail(&vcpu_pi->pi_wakeup_list,
  		      &per_cpu(wakeup_vcpus_on_cpu, vcpu->cpu));
  	raw_spin_unlock(&per_cpu(wakeup_vcpus_on_cpu_lock, vcpu->cpu));

@@ -190,7 +211,8 @@ static bool vmx_needs_pi_wakeup(struct kvm_vcpu *vcpu)
  	 * notification vector is switched to the one that calls
  	 * back to the pi_wakeup_handler() function.
  	 */
-	return vmx_can_use_ipiv(vcpu) || vmx_can_use_vtd_pi(vcpu->kvm);
+	return (vmx_can_use_ipiv(vcpu) && !is_td_vcpu(vcpu)) ||
+		vmx_can_use_vtd_pi(vcpu->kvm);
  }
  void vmx_vcpu_pi_put(struct kvm_vcpu *vcpu)

@@ -200,7 +222,8 @@ void vmx_vcpu_pi_put(struct kvm_vcpu *vcpu)
  	if (!vmx_needs_pi_wakeup(vcpu))
  		return;
-	if (kvm_vcpu_is_blocking(vcpu) && !vmx_interrupt_blocked(vcpu))
+	if (kvm_vcpu_is_blocking(vcpu) &&
+	    (is_td_vcpu(vcpu) || !vmx_interrupt_blocked(vcpu)))
  		pi_enable_wakeup_handler(vcpu);
  	/*

arch/x86/kvm/vmx/posted_intr.h +0 -0

index 26992076552e..2fe8222308b2 100644
@@ -94,6 +94,17 @@ static inline bool pi_test_sn(struct pi_desc *pi_desc)
  			(unsigned long *)&pi_desc->control);

arch/x86/kvm/vmx/posted_intr.h static inline bool pi_test_sn(struct pi_desc *pi_desc) · line 104

Quoted 10 lines

arch/x86/kvm/vmx/posted_intr.h +9 -0

  }
+struct vcpu_pi {
+	struct kvm_vcpu	vcpu;
+
+	/* Posted interrupt descriptor */
+	struct pi_desc pi_desc;
+
+	/* Used if this vCPU is waiting for PI notification wakeup. */
+	struct list_head pi_wakeup_list;
+	/* Until here common layout betwwn vcpu_vmx and vcpu_tdx. */

s/betwwn/between

Also, in pi_wakeup_handler(), it is still using struct vcpu_vmx, but it could
be vcpu_tdx.
Functionally it is OK, however, since you have added vcpu_pi, should it use vcpu_pi instead of vcpu_vmx in pi_wakeup_handler()?

arch/x86/kvm/vmx/posted_intr.c +0 -0

 		pi_enable_wakeup_handler(vcpu);
 

arch/x86/kvm/vmx/posted_intr.c static bool vmx_needs_pi_wakeup(struct kvm_vcpu *vcpu) · line 215

Quoted 7 lines · On Mon, Feb 26, 2024 at 12:26:33AM -0800, [email protected] wrote:

arch/x86/kvm/vmx/posted_intr.c +2 -1

@@ -190,7 +211,8 @@ static bool vmx_needs_pi_wakeup(struct kvm_vcpu *vcpu)
 	 * notification vector is switched to the one that calls
 	 * back to the pi_wakeup_handler() function.
 	 */
-	return vmx_can_use_ipiv(vcpu) || vmx_can_use_vtd_pi(vcpu->kvm);
+	return (vmx_can_use_ipiv(vcpu) && !is_td_vcpu(vcpu)) ||
+		vmx_can_use_vtd_pi(vcpu->kvm);

It is better to separate this functional change from the code refactoring.

arch/x86/kvm/vmx/posted_intr.c void vmx_vcpu_pi_put(struct kvm_vcpu *vcpu) · line 226

Quoted 10 lines

arch/x86/kvm/vmx/posted_intr.c +2 -1

 }
 
 void vmx_vcpu_pi_put(struct kvm_vcpu *vcpu)

@@ -200,7 +222,8 @@ void vmx_vcpu_pi_put(struct kvm_vcpu *vcpu)
 	if (!vmx_needs_pi_wakeup(vcpu))
 		return;
 
-	if (kvm_vcpu_is_blocking(vcpu) && !vmx_interrupt_blocked(vcpu))
+	if (kvm_vcpu_is_blocking(vcpu) &&
+	    (is_td_vcpu(vcpu) || !vmx_interrupt_blocked(vcpu)))

Ditto.

This looks incorrect to me. here we assume interrupt is always enabled for TD. But on TDVMCALL(HLT), the guest tells KVM if hlt is called with interrupt disabled. KVM can just check that interrupt status passed from the guest.

Quoted 12 lines · Chao Gao

arch/x86/kvm/vmx/posted_intr.c void vmx_vcpu_pi_put(struct kvm_vcpu *vcpu) · line 226

Quoted 10 lines

}

void vmx_vcpu_pi_put(struct kvm_vcpu *vcpu)

arch/x86/kvm/vmx/posted_intr.c +2 -1

@@ -200,7 +222,8 @@ void vmx_vcpu_pi_put(struct kvm_vcpu *vcpu)
 	if (!vmx_needs_pi_wakeup(vcpu))
 		return;
 
-	if (kvm_vcpu_is_blocking(vcpu) && !vmx_interrupt_blocked(vcpu))
+	if (kvm_vcpu_is_blocking(vcpu) &&
+	    (is_td_vcpu(vcpu) || !vmx_interrupt_blocked(vcpu)))

Ditto.

Quoted 3 lines

This looks incorrect to me. here we assume interrupt is always enabled for TD. But on TDVMCALL(HLT), the guest tells KVM if hlt is called with interrupt disabled. KVM can just check that interrupt status passed from the guest.

That's true. We can complicate this function and HLT emulation. But I don't think it's worthwhile because HLT with interrupt masked is rare. Only for CPU online.

arch/x86/kvm/vmx/tdx.c fastpath_t tdx_vcpu_run(struct kvm_vcpu *vcpu) · line 859

Quoted 8 lines

arch/x86/kvm/vmx/tdx.c +4 -0

@@ -848,6 +853,12 @@ fastpath_t tdx_vcpu_run(struct kvm_vcpu *vcpu)
 
 	trace_kvm_entry(vcpu);
 
+	if (pi_test_on(&tdx->pi_desc)) {
+		apic->send_IPI_self(POSTED_INTR_VECTOR);
+
+		kvm_wait_lapic_expire(vcpu);

it seems the APIC timer change was inadvertently included.

From: Isaku Yamahata<[email protected]>

TDX supports interrupt inject into vcpu with posted interrupt. Wire up the corresponding kvm x86 operations to posted interrupt. Move kvm_vcpu_trigger_posted_interrupt() from vmx.c to common.h to share the code.

VMX can inject interrupt by setting interrupt information field, VM_ENTRY_INTR_INFO_FIELD, of VMCS. TDX supports interrupt injection only by posted interrupt. Ignore the execution path to access VM_ENTRY_INTR_INFO_FIELD.

As cpu state is protected and apicv is enabled for the TDX guest, VMM can inject interrupt by updating posted interrupt descriptor. Treat interrupt can be injected always.

Signed-off-by Isaku Yamahata<[email protected]>

Reviewed-by Paolo Bonzini<[email protected]>

---
  arch/x86/kvm/vmx/common.h      | 71 ++++++++++++++++++++++++++
  arch/x86/kvm/vmx/main.c        | 93 ++++++++++++++++++++++++++++++----
  arch/x86/kvm/vmx/posted_intr.c |  2 +-
  arch/x86/kvm/vmx/posted_intr.h |  2 +
  arch/x86/kvm/vmx/tdx.c         | 25 +++++++++
  arch/x86/kvm/vmx/vmx.c         | 67 +-----------------------
  arch/x86/kvm/vmx/x86_ops.h     |  7 ++-
  7 files changed, 190 insertions(+), 77 deletions(-)

arch/x86/kvm/vmx/main.c +3 -0

+	vmx_set_interrupt_shadow(vcpu, mask);
+}
+

arch/x86/kvm/vmx/tdx.c +2 -0

+	}
+
  	tdx_vcpu_enter_exit(tdx);
 
  	tdx_user_return_update_cache(vcpu);

@@ -1213,6 +1224,16 @@ static int tdx_sept_remove_private_spte(struct kvm *kvm, gfn_t gfn,
  	return tdx_sept_drop_private_spte(kvm, gfn, level, pfn);
  }
 

From: Isaku Yamahata <[email protected]>

Now we are able to inject interrupts into TDX vcpu, it's ready to block TDX vcpu. Wire up kvm x86 methods for blocking/unblocking vcpu for TDX. To unblock on pending events, request immediate exit methods is also needed.

Signed-off-by Isaku Yamahata <[email protected]>

Reviewed-by Paolo Bonzini <[email protected]>

---
arch/x86/kvm/vmx/main.c | 12 +++++++++++-

1 file changed, 11 insertions(+), 1 deletion(-)

arch/x86/kvm/vmx/main.c +11 -1

index f2c9d6358f9e..ee6c04959d4c 100644
@@ -372,6 +372,16 @@ static void vt_enable_irq_window(struct kvm_vcpu *vcpu)
 	vmx_enable_irq_window(vcpu);
 }
 
+static void vt_request_immediate_exit(struct kvm_vcpu *vcpu)
+{
+	if (is_td_vcpu(vcpu)) {
+		__kvm_request_immediate_exit(vcpu);
+		return;
+	}
+
+	vmx_request_immediate_exit(vcpu);
+}
+
 static u8 vt_get_mt_mask(struct kvm_vcpu *vcpu, gfn_t gfn, bool is_mmio)
 {
 	if (is_td_vcpu(vcpu))

@@ -549,7 +559,7 @@ struct kvm_x86_ops vt_x86_ops __initdata = {
 	.check_intercept = vmx_check_intercept,
 	.handle_exit_irqoff = vmx_handle_exit_irqoff,
 
-	.request_immediate_exit = vmx_request_immediate_exit,
+	.request_immediate_exit = vt_request_immediate_exit,
 
 	.sched_in = vt_sched_in,
 

Signature · 1 line

2.25.1

Quoted 5 lines · On Mon, Feb 26, 2024 at 12:26:35AM -0800, [email protected] wrote:

From: Isaku Yamahata <[email protected]>

Now we are able to inject interrupts into TDX vcpu, it's ready to block TDX vcpu. Wire up kvm x86 methods for blocking/unblocking vcpu for TDX. To unblock on pending events, request immediate exit methods is also needed.

TDX doesn't support this immediate exit. It is considered as a potential attack to TDs. TDX module deploys 0/1-step mitigations to prevent this. Even KVM issues a self-IPI before TD-entry, TD-exit will happen after the guest runs a random number of instructions.

KVM shouldn't request immediate exits in the first place. Just emit a warning if KVM tries to do this.

arch/x86/kvm/vmx/main.c +6 -0

+static void vt_set_nmi_mask(struct kvm_vcpu *vcpu, bool masked)
+{
+	if (is_td_vcpu(vcpu))
+		return;
+
+	vmx_set_nmi_mask(vcpu, masked);

Quoted 13 lines

arch/x86/kvm/vmx/main.c +13 -0

+static void vt_set_nmi_mask(struct kvm_vcpu *vcpu, bool masked)
+{
+	if (is_td_vcpu(vcpu))
+		return;
+
+	vmx_set_nmi_mask(vcpu, masked);
+}
+
+static void vt_enable_nmi_window(struct kvm_vcpu *vcpu)
+{
+	/* Refer the comment in vt_get_nmi_mask(). */
+	if (is_td_vcpu(vcpu))
+		return;

arch/x86/kvm/vmx/main.c static void vt_flush_tlb_guest(struct kvm_vcpu *vcpu) · line 361

Quoted 3 lines

arch/x86/kvm/vmx/main.c +3 -0

+
+	vmx_enable_nmi_window(vcpu);
+}

The two actually request something to do done for the TD. But we make them nop as TDX module doesn't support VMM to configure nmi mask and nmi window. Do you think they are worth a WARN_ON_ONCE()? or adding WARN_ON_ONCE() requires a lot of code factoring in KVM's NMI injection logics?

From: Sean Christopherson <[email protected]>

TDX uses different ABI to get information about VM exit. Pass intr_info to the NMI and INTR handlers instead of pulling it from vcpu_vmx in preparation for sharing the bulk of the handlers with TDX.

When the guest TD exits to VMM, RAX holds status and exit reason, RCX holds exit qualification etc rather than the VMCS fields because VMM doesn't have access to the VMCS. The eventual code will be

VMX:
- get exit reason, intr_info, exit_qualification, and etc from VMCS
- call NMI/INTR handlers (common code)

TDX:
- get exit reason, intr_info, exit_qualification, and etc from guest registers
- call NMI/INTR handlers (common code)

Signed-off-by Sean Christopherson <[email protected]>

Signed-off-by Isaku Yamahata <[email protected]>

Reviewed-by Paolo Bonzini <[email protected]>

From: Sean Christopherson <[email protected]>

TDX mostly handles NMI/exception exit mostly the same to VMX case. The difference is how to retrieve exit qualification. To share the code with TDX, move NMI/exception to a common header, common.h.

Signed-off-by Sean Christopherson <[email protected]>

Signed-off-by Isaku Yamahata <[email protected]>

---
  arch/x86/kvm/vmx/common.h | 59 +++++++++++++++++++++++++++++++++
  arch/x86/kvm/vmx/vmx.c    | 68 +++++----------------------------------
  2 files changed, 67 insertions(+), 60 deletions(-)

arch/x86/kvm/vmx/common.h +59 -0

index 6f21d0d48809..632af7a76d0a 100644
@@ -4,8 +4,67 @@
 
  #include <linux/kvm_host.h>
 
+#include <asm/traps.h>
+
  #include "posted_intr.h"
  #include "mmu.h"
+#include "vmcs.h"
+#include "x86.h"
+
+extern unsigned long vmx_host_idt_base;
+void vmx_do_interrupt_irqoff(unsigned long entry);
+void vmx_do_nmi_irqoff(void);
+
+static inline void vmx_handle_nm_fault_irqoff(struct kvm_vcpu *vcpu)
+{
+	/*
+	 * Save xfd_err to guest_fpu before interrupt is enabled, so the
+	 * MSR value is not clobbered by the host activity before the guest
+	 * has chance to consume it.
+	 *
+	 * Do not blindly read xfd_err here, since this exception might
+	 * be caused by L1 interception on a platform which doesn't
+	 * support xfd at all.
+	 *
+	 * Do it conditionally upon guest_fpu::xfd. xfd_err matters
+	 * only when xfd contains a non-zero value.
+	 *
+	 * Queuing exception is done in vmx_handle_exit. See comment there.
+	 */
+	if (vcpu->arch.guest_fpu.fpstate->xfd)
+		rdmsrl(MSR_IA32_XFD_ERR, vcpu->arch.guest_fpu.xfd_err);
+}
+
+static inline void vmx_handle_exception_irqoff(struct kvm_vcpu *vcpu,
+					       u32 intr_info)
+{
+	/* if exit due to PF check for async PF */
+	if (is_page_fault(intr_info))
+		vcpu->arch.apf.host_apf_flags = kvm_read_and_reset_apf_flags();
+	/* if exit due to NM, handle before interrupts are enabled */
+	else if (is_nm_fault(intr_info))
+		vmx_handle_nm_fault_irqoff(vcpu);
+	/* Handle machine checks before interrupts are enabled */
+	else if (is_machine_check(intr_info))
+		kvm_machine_check();
+}
+
+static inline void vmx_handle_external_interrupt_irqoff(struct kvm_vcpu *vcpu,
+							u32 intr_info)
+{
+	unsigned int vector = intr_info & INTR_INFO_VECTOR_MASK;
+	gate_desc *desc = (gate_desc *)vmx_host_idt_base + vector;
+
+	if (KVM_BUG(!is_external_intr(intr_info), vcpu->kvm,
+	    "unexpected VM-Exit interrupt info: 0x%x", intr_info))
+		return;
+
+	kvm_before_interrupt(vcpu, KVM_HANDLING_IRQ);
+	vmx_do_interrupt_irqoff(gate_offset(desc));
+	kvm_after_interrupt(vcpu);
+
+	vcpu->arch.at_instruction_boundary = true;
+}
 
  static inline int __vmx_handle_ept_violation(struct kvm_vcpu *vcpu, gpa_t gpa,
  					     unsigned long exit_qualification)

arch/x86/kvm/vmx/vmx.c +8 -60

index 29d891e0795e..f8a00a766c40 100644
@@ -518,7 +518,7 @@ static inline void vmx_segment_cache_clear(struct vcpu_vmx *vmx)
  	vmx->segment_cache.bitmask = 0;
  }
 
-static unsigned long host_idt_base;
+unsigned long vmx_host_idt_base;
 
  #if IS_ENABLED(CONFIG_HYPERV)
  static bool __read_mostly enlightened_vmcs = true;

@@ -4273,7 +4273,7 @@ void vmx_set_constant_host_state(struct vcpu_vmx *vmx)
  	vmcs_write16(HOST_SS_SELECTOR, __KERNEL_DS);  /* 22.2.4 */
  	vmcs_write16(HOST_TR_SELECTOR, GDT_ENTRY_TSS*8);  /* 22.2.4 */
 
-	vmcs_writel(HOST_IDTR_BASE, host_idt_base);   /* 22.2.4 */
+	vmcs_writel(HOST_IDTR_BASE, vmx_host_idt_base);   /* 22.2.4 */
 
  	vmcs_writel(HOST_RIP, (unsigned long)vmx_vmexit); /* 22.2.5 */
 

@@ -5166,7 +5166,7 @@ static int handle_exception_nmi(struct kvm_vcpu *vcpu)
  	intr_info = vmx_get_intr_info(vcpu);
 
  	/*
-	 * Machine checks are handled by handle_exception_irqoff(), or by
+	 * Machine checks are handled by vmx_handle_exception_irqoff(), or by
  	 * vmx_vcpu_run() if a #MC occurs on VM-Entry.  NMIs are handled by
  	 * vmx_vcpu_enter_exit().
  	 */

@@ -5174,7 +5174,7 @@ static int handle_exception_nmi(struct kvm_vcpu *vcpu)
  		return 1;
 
  	/*
-	 * Queue the exception here instead of in handle_nm_fault_irqoff().
+	 * Queue the exception here instead of in vmx_handle_nm_fault_irqoff().
  	 * This ensures the nested_vmx check is not skipped so vmexit can
  	 * be reflected to L1 (when it intercepts #NM) before reaching this
  	 * point.

@@ -6889,59 +6889,6 @@ void vmx_load_eoi_exitmap(struct kvm_vcpu *vcpu, u64 *eoi_exit_bitmap)
  	vmcs_write64(EOI_EXIT_BITMAP3, eoi_exit_bitmap[3]);
  }
 
-void vmx_do_interrupt_irqoff(unsigned long entry);
-void vmx_do_nmi_irqoff(void);
-
-static void handle_nm_fault_irqoff(struct kvm_vcpu *vcpu)
-{
-	/*
-	 * Save xfd_err to guest_fpu before interrupt is enabled, so the
-	 * MSR value is not clobbered by the host activity before the guest
-	 * has chance to consume it.
-	 *
-	 * Do not blindly read xfd_err here, since this exception might
-	 * be caused by L1 interception on a platform which doesn't
-	 * support xfd at all.
-	 *
-	 * Do it conditionally upon guest_fpu::xfd. xfd_err matters
-	 * only when xfd contains a non-zero value.
-	 *
-	 * Queuing exception is done in vmx_handle_exit. See comment there.
-	 */
-	if (vcpu->arch.guest_fpu.fpstate->xfd)
-		rdmsrl(MSR_IA32_XFD_ERR, vcpu->arch.guest_fpu.xfd_err);
-}
-
-static void handle_exception_irqoff(struct kvm_vcpu *vcpu, u32 intr_info)
-{
-	/* if exit due to PF check for async PF */
-	if (is_page_fault(intr_info))
-		vcpu->arch.apf.host_apf_flags = kvm_read_and_reset_apf_flags();
-	/* if exit due to NM, handle before interrupts are enabled */
-	else if (is_nm_fault(intr_info))
-		handle_nm_fault_irqoff(vcpu);
-	/* Handle machine checks before interrupts are enabled */
-	else if (is_machine_check(intr_info))
-		kvm_machine_check();
-}
-
-static void handle_external_interrupt_irqoff(struct kvm_vcpu *vcpu,
-					     u32 intr_info)
-{
-	unsigned int vector = intr_info & INTR_INFO_VECTOR_MASK;
-	gate_desc *desc = (gate_desc *)host_idt_base + vector;
-
-	if (KVM_BUG(!is_external_intr(intr_info), vcpu->kvm,
-	    "unexpected VM-Exit interrupt info: 0x%x", intr_info))
-		return;
-
-	kvm_before_interrupt(vcpu, KVM_HANDLING_IRQ);
-	vmx_do_interrupt_irqoff(gate_offset(desc));
-	kvm_after_interrupt(vcpu);
-
-	vcpu->arch.at_instruction_boundary = true;
-}
-
  void vmx_handle_exit_irqoff(struct kvm_vcpu *vcpu)
  {
  	struct vcpu_vmx *vmx = to_vmx(vcpu);

@@ -6950,9 +6897,10 @@ void vmx_handle_exit_irqoff(struct kvm_vcpu *vcpu)
  		return;
 
  	if (vmx->exit_reason.basic == EXIT_REASON_EXTERNAL_INTERRUPT)
-		handle_external_interrupt_irqoff(vcpu, vmx_get_intr_info(vcpu));
+		vmx_handle_external_interrupt_irqoff(vcpu,
+						     vmx_get_intr_info(vcpu));
  	else if (vmx->exit_reason.basic == EXIT_REASON_EXCEPTION_NMI)
-		handle_exception_irqoff(vcpu, vmx_get_intr_info(vcpu));
+		vmx_handle_exception_irqoff(vcpu, vmx_get_intr_info(vcpu));
  }
 
  /*

@@ -8284,7 +8232,7 @@ __init int vmx_hardware_setup(void)
  	int r;
 
  	store_idt(&dt);
-	host_idt_base = dt.address;
+	vmx_host_idt_base = dt.address;
 
  	vmx_setup_user_return_msrs();
 

arch/x86/kvm/x86.c +1 -0

 		return 0;
 	}
 	default:
 		ret = -KVM_ENOSYS;
 		break;
 	}
+

out:

arch/x86/kvm/x86.c +18 -0

+	++vcpu->stat.hypercalls;
+	return ret;
+}
+EXPORT_SYMBOL_GPL(__kvm_emulate_hypercall);
+
+int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
+{
+	unsigned long nr, a0, a1, a2, a3, ret;
+	int op_64_bit;
+	int cpl;
+
+	if (kvm_xen_hypercall_enabled(vcpu->kvm))
+		return kvm_xen_hypercall(vcpu);
+
+	if (kvm_hv_hypercall_enabled(vcpu))
+		return kvm_hv_hypercall(vcpu);
+
+	nr = kvm_rax_read(vcpu);

arch/x86/kvm/x86.c +2 -0

+		return 0;
+

	if (!op_64_bit)
		ret = (u32)ret;
	kvm_rax_write(vcpu, ret);

arch/x86/kvm/x86.c +0 -1

-	++vcpu->stat.hypercalls;

	return kvm_skip_emulated_instruction(vcpu);
}

EXPORT_SYMBOL_GPL(kvm_emulate_hypercall);

Signature · 1 line

2.25.1

arch/x86/kvm/x86.c int kvm_emulate_hypercall(struct kvm_vcpu *vcpu) · line 10154

Quoted 5 lines · On Mon, Feb 26, 2024 at 12:26:39AM -0800, [email protected] wrote:

arch/x86/kvm/x86.c +1 -0

@@ -10162,18 +10151,49 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
 
 		WARN_ON_ONCE(vcpu->run->hypercall.flags & KVM_EXIT_HYPERCALL_MBZ);
 		vcpu->arch.complete_userspace_io = complete_hypercall_exit;
+		/* stat is incremented on completion. */

Perhaps we could use a distinct return value to signal that the request is redirected to userspace. This way, more cases can be supported, e.g., accesses to MTRR MSRs, requests to service TDs, etc. And then ...

Quoted 30 lines

arch/x86/kvm/x86.c +1 -0

 		return 0;
 	}
 	default:
 		ret = -KVM_ENOSYS;
 		break;
 	}
+

out:

arch/x86/kvm/x86.c +22 -0

+	++vcpu->stat.hypercalls;
+	return ret;
+}
+EXPORT_SYMBOL_GPL(__kvm_emulate_hypercall);
+
+int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
+{
+	unsigned long nr, a0, a1, a2, a3, ret;
+	int op_64_bit;
+	int cpl;
+
+	if (kvm_xen_hypercall_enabled(vcpu->kvm))
+		return kvm_xen_hypercall(vcpu);
+
+	if (kvm_hv_hypercall_enabled(vcpu))
+		return kvm_hv_hypercall(vcpu);
+
+	nr = kvm_rax_read(vcpu);
+	a0 = kvm_rbx_read(vcpu);
+	a1 = kvm_rcx_read(vcpu);
+	a2 = kvm_rdx_read(vcpu);
+	a3 = kvm_rsi_read(vcpu);

arch/x86/kvm/x86.c int kvm_emulate_hypercall(struct kvm_vcpu *vcpu) · line 10190

Quoted 6 lines

arch/x86/kvm/x86.c +6 -0

+	op_64_bit = is_64_bit_hypercall(vcpu);
+	cpl = static_call(kvm_x86_get_cpl)(vcpu);
+
+	ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl);
+	if (nr == KVM_HC_MAP_GPA_RANGE && !ret)
+		/* MAP_GPA tosses the request to the user space. */

no need to check what the request is. Just checking the return value will suffice.

Quoted 15 lines

arch/x86/kvm/x86.c +2 -0

+		return 0;
+

	if (!op_64_bit)
		ret = (u32)ret;
	kvm_rax_write(vcpu, ret);

arch/x86/kvm/x86.c +0 -1

-	++vcpu->stat.hypercalls;

	return kvm_skip_emulated_instruction(vcpu);
}

EXPORT_SYMBOL_GPL(kvm_emulate_hypercall);

Signature · 1 line

2.25.1

Quoted 10 lines · Chao Gao

arch/x86/kvm/x86.c int kvm_emulate_hypercall(struct kvm_vcpu *vcpu) · line 10154

Quoted 5 lines · On Mon, Feb 26, 2024 at 12:26:39AM -0800, [email protected] wrote:

arch/x86/kvm/x86.c +1 -0

@@ -10162,18 +10151,49 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
 
 		WARN_ON_ONCE(vcpu->run->hypercall.flags & KVM_EXIT_HYPERCALL_MBZ);
 		vcpu->arch.complete_userspace_io = complete_hypercall_exit;
+		/* stat is incremented on completion. */

Perhaps we could use a distinct return value to signal that the request is redirected to userspace. This way, more cases can be supported, e.g., accesses to MTRR MSRs, requests to service TDs, etc. And then ...

The convention here is the one for exit_handler vcpu_enter_guest() already uses. If we introduce something like KVM_VCPU_CONTINUE=1, KVM_VCPU_EXIT_TO_USER=0, it will touch many places. So if we will (I'm not sure it's worthwhile), the cleanup should be done as independently.

Quoted 18 lines · Chao Gao

Quoted 17 lines · On Mon, Feb 26, 2024 at 12:26:39AM -0800, [email protected] wrote:

arch/x86/kvm/x86.c +17 -0

+
+int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
+{
+	unsigned long nr, a0, a1, a2, a3, ret;
+	int op_64_bit;
+	int cpl;
+
+	if (kvm_xen_hypercall_enabled(vcpu->kvm))
+		return kvm_xen_hypercall(vcpu);
+
+	if (kvm_hv_hypercall_enabled(vcpu))
+		return kvm_hv_hypercall(vcpu);
+
+	nr = kvm_rax_read(vcpu);
+	a0 = kvm_rbx_read(vcpu);
+	a1 = kvm_rcx_read(vcpu);
+	a2 = kvm_rdx_read(vcpu);

Quoted 8 lines

arch/x86/kvm/x86.c int kvm_emulate_hypercall(struct kvm_vcpu *vcpu) · line 10190

Quoted 7 lines

arch/x86/kvm/x86.c +7 -0

+	a3 = kvm_rsi_read(vcpu);
+	op_64_bit = is_64_bit_hypercall(vcpu);
+	cpl = static_call(kvm_x86_get_cpl)(vcpu);
+
+	ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl);
+	if (nr == KVM_HC_MAP_GPA_RANGE && !ret)
+		/* MAP_GPA tosses the request to the user space. */

no need to check what the request is. Just checking the return value will suffice.

This is needed to avoid updating rax etc. KVM_HC_MAP_GPA_RANGE is only an exception to go to the user space. This check is a bit weird, but I couldn't find a good way.

Quoted 15 lines

Quoted 15 lines

arch/x86/kvm/x86.c +2 -0

+		return 0;
+

	if (!op_64_bit)
		ret = (u32)ret;
	kvm_rax_write(vcpu, ret);

arch/x86/kvm/x86.c +0 -1

-	++vcpu->stat.hypercalls;

	return kvm_skip_emulated_instruction(vcpu);
}

EXPORT_SYMBOL_GPL(kvm_emulate_hypercall);

Signature · 1 line

2.25.1

From: Sean Christopherson <[email protected]>

By necessity, TDX will use a different register ABI for hypercalls. Break out the core functionality so that it may be reused for TDX.

Signed-off-by Sean Christopherson <[email protected]>

Signed-off-by Isaku Yamahata <[email protected]>

---
  arch/x86/include/asm/kvm_host.h |  4 +++
  arch/x86/kvm/x86.c              | 56 ++++++++++++++++++++++-----------
  2 files changed, 42 insertions(+), 18 deletions(-)

arch/x86/include/asm/kvm_host.h +4 -0

index e0ffef1d377d..bb8be091f996 100644
@@ -2177,6 +2177,10 @@ static inline void kvm_clear_apicv_inhibit(struct kvm *kvm,
  	kvm_set_or_clear_apicv_inhibit(kvm, reason, false);
  }
 
+unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
+				      unsigned long a0, unsigned long a1,
+				      unsigned long a2, unsigned long a3,
+				      int op_64_bit, int cpl);
  int kvm_emulate_hypercall(struct kvm_vcpu *vcpu);
 
  int kvm_mmu_page_fault(struct kvm_vcpu *vcpu, gpa_t cr2_or_gpa, u64 error_code,

arch/x86/kvm/x86.c +8 -17

index fb7597c22f31..03950368d8db 100644
@@ -10073,26 +10073,15 @@ static int complete_hypercall_exit(struct kvm_vcpu *vcpu)
  	return kvm_skip_emulated_instruction(vcpu);
  }
 
-int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
+unsigned long __kvm_emulate_hypercall(struct kvm_vcpu *vcpu, unsigned long nr,
+				      unsigned long a0, unsigned long a1,
+				      unsigned long a2, unsigned long a3,
+				      int op_64_bit, int cpl)
  {
-	unsigned long nr, a0, a1, a2, a3, ret;
-	int op_64_bit;
-
-	if (kvm_xen_hypercall_enabled(vcpu->kvm))
-		return kvm_xen_hypercall(vcpu);
-
-	if (kvm_hv_hypercall_enabled(vcpu))
-		return kvm_hv_hypercall(vcpu);
-
-	nr = kvm_rax_read(vcpu);
-	a0 = kvm_rbx_read(vcpu);
-	a1 = kvm_rcx_read(vcpu);
-	a2 = kvm_rdx_read(vcpu);
-	a3 = kvm_rsi_read(vcpu);
+	unsigned long ret;
 
  	trace_kvm_hypercall(nr, a0, a1, a2, a3);
 
-	op_64_bit = is_64_bit_hypercall(vcpu);
  	if (!op_64_bit) {
  		nr &= 0xFFFFFFFF;
  		a0 &= 0xFFFFFFFF;

@@ -10101,7 +10090,7 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
  		a3 &= 0xFFFFFFFF;
  	}
 
-	if (static_call(kvm_x86_get_cpl)(vcpu) != 0) {
+	if (cpl) {
  		ret = -KVM_EPERM;
  		goto out;
  	}

@@ -10162,18 +10151,49 @@ int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
 
  		WARN_ON_ONCE(vcpu->run->hypercall.flags & KVM_EXIT_HYPERCALL_MBZ);
  		vcpu->arch.complete_userspace_io = complete_hypercall_exit;
+		/* stat is incremented on completion. */
  		return 0;
  	}
  	default:
  		ret = -KVM_ENOSYS;
  		break;
  	}
+

arch/x86/kvm/x86.c +21 -1

+	int cpl;
+
+	if (kvm_xen_hypercall_enabled(vcpu->kvm))
+		return kvm_xen_hypercall(vcpu);
+
+	if (kvm_hv_hypercall_enabled(vcpu))
+		return kvm_hv_hypercall(vcpu);
+
+	nr = kvm_rax_read(vcpu);
+	a0 = kvm_rbx_read(vcpu);
+	a1 = kvm_rcx_read(vcpu);
+	a2 = kvm_rdx_read(vcpu);
+	a3 = kvm_rsi_read(vcpu);
+	op_64_bit = is_64_bit_hypercall(vcpu);
+	cpl = static_call(kvm_x86_get_cpl)(vcpu);
+
+	ret = __kvm_emulate_hypercall(vcpu, nr, a0, a1, a2, a3, op_64_bit, cpl);
+	if (nr == KVM_HC_MAP_GPA_RANGE && !ret)
+		/* MAP_GPA tosses the request to the user space. */
+		return 0;
+
  	if (!op_64_bit)
  		ret = (u32)ret;
  	kvm_rax_write(vcpu, ret);
 
-	++vcpu->stat.hypercalls;
  	return kvm_skip_emulated_instruction(vcpu);
  }
  EXPORT_SYMBOL_GPL(kvm_emulate_hypercall);

arch/x86/kvm/x86.c int kvm_emulate_hypercall(struct kvm_vcpu *vcpu) · line 10171

Quoted 4 lines

arch/x86/kvm/x86.c +4 -0

+int kvm_emulate_hypercall(struct kvm_vcpu *vcpu)
+{
+	unsigned long nr, a0, a1, a2, a3, ret;
+	int op_64_bit;

Can it be opportunistically changed to bool type, as well as the argument type of "op_64_bit" in __kvm_emulate_hypercall()?

arch/x86/kvm/vmx/tdx.c +12 -0

+int tdx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t fastpath)
+{
+	union tdx_exit_reason exit_reason = to_tdx(vcpu)->exit_reason;
+
+	/* See the comment of tdh_sept_seamcall(). */
+	if (unlikely(exit_reason.full == (TDX_OPERAND_BUSY | TDX_OPERAND_ID_SEPT)))
+		return 1;
+
+	/*
+	 * TDH.VP.ENTRY checks TD EPOCH which contend with TDH.MEM.TRACK and
+	 * vcpu TDH.VP.ENTER.
+	 */

Quoted 12 lines · On Mon, Feb 26, 2024, [email protected] wrote:

arch/x86/kvm/vmx/tdx.c +12 -0

+int tdx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t fastpath)
+{
+	union tdx_exit_reason exit_reason = to_tdx(vcpu)->exit_reason;
+
+	/* See the comment of tdh_sept_seamcall(). */
+	if (unlikely(exit_reason.full == (TDX_OPERAND_BUSY | TDX_OPERAND_ID_SEPT)))
+		return 1;
+
+	/*
+	 * TDH.VP.ENTRY checks TD EPOCH which contend with TDH.MEM.TRACK and
+	 * vcpu TDH.VP.ENTER.
+	 */

arch/x86/kvm/vmx/tdx.c void tdx_deliver_interrupt(struct kvm_lapic *apic, int delivery_mode, · line 1309

Quoted 10 lines

arch/x86/kvm/vmx/tdx.c +10 -0

+	if (unlikely(exit_reason.full == (TDX_OPERAND_BUSY | TDX_OPERAND_ID_TD_EPOCH)))
+		return 1;
+
+	if (unlikely(exit_reason.full == TDX_SEAMCALL_UD)) {
+		kvm_spurious_fault();
+		/*
+		 * In the case of reboot or kexec, loop with TDH.VP.ENTER and
+		 * TDX_SEAMCALL_UD to avoid unnecessarily activity.
+		 */
+		return 1;

No. This is unnecessarily risky. KVM_BUG_ON() and exit to userspace. The response to "SEAMCALL faulted" should never be, "well, let's try again!".

Also, what about #GP on SEAMCALL? In general, the error handling here seems lacking.

From: Isaku Yamahata <[email protected]>

Wire up handle_exit and handle_exit_irqoff methods

and add a place holder
to handle VM exit. Add helper functions to get exit info, exit qualification, etc.

Signed-off-by Isaku Yamahata <[email protected]>

Reviewed-by Paolo Bonzini <[email protected]>

---
  arch/x86/kvm/vmx/main.c    |  37 ++++++++++++-
  arch/x86/kvm/vmx/tdx.c     | 110 +++++++++++++++++++++++++++++++++++++
  arch/x86/kvm/vmx/x86_ops.h |  10 ++++
  3 files changed, 154 insertions(+), 3 deletions(-)

arch/x86/kvm/vmx/main.c +3 -3

@@ -562,7 +593,7 @@ struct kvm_x86_ops vt_x86_ops __initdata = {
 
  	.vcpu_pre_run = vt_vcpu_pre_run,
  	.vcpu_run = vt_vcpu_run,
-	.handle_exit = vmx_handle_exit,
+	.handle_exit = vt_handle_exit,
  	.skip_emulated_instruction = vmx_skip_emulated_instruction,
  	.update_emulated_instruction = vmx_update_emulated_instruction,
  	.set_interrupt_shadow = vt_set_interrupt_shadow,

@@ -597,7 +628,7 @@ struct kvm_x86_ops vt_x86_ops __initdata = {
  	.set_identity_map_addr = vmx_set_identity_map_addr,
  	.get_mt_mask = vt_get_mt_mask,
 
-	.get_exit_info = vmx_get_exit_info,
+	.get_exit_info = vt_get_exit_info,
 
  	.vcpu_after_set_cpuid = vmx_vcpu_after_set_cpuid,
 

@@ -611,7 +642,7 @@ struct kvm_x86_ops vt_x86_ops __initdata = {
  	.load_mmu_pgd = vt_load_mmu_pgd,
 
  	.check_intercept = vmx_check_intercept,
-	.handle_exit_irqoff = vmx_handle_exit_irqoff,
+	.handle_exit_irqoff = vt_handle_exit_irqoff,
 
  	.request_immediate_exit = vt_request_immediate_exit,
 

(quoted diff) +1 -0

+	 */

arch/x86/kvm/vmx/tdx.c void tdx_deliver_interrupt(struct kvm_lapic *apic, int delivery_mode,

Quoted 4 lines

arch/x86/kvm/vmx/tdx.c +4 -0

+		return 1;
+
+	/*
+	 * TDH.VP.ENTRY

"TDH.VP.ENTRY" -> "TDH.VP.ENTER"

arch/x86/kvm/vmx/tdx.c void tdx_deliver_interrupt(struct kvm_lapic *apic, int delivery_mode, · line 1298

Quoted 2 lines

checks TD EPOCH which contend with TDH.MEM.TRACK and

arch/x86/kvm/vmx/tdx.c +1 -0

+	 * vcpu TDH.VP.ENTER.

Do you mean TDH.VP.ENTER on one vcpu can contend with TDH.MEM.TRACK and TDH.VP.ENTER on another vcpu?

From: Isaku Yamahata <[email protected]>

If the control reaches EXIT_REASON_OTHER_SMI, #SMI is delivered and handled right after returning from the TDX module to KVM

nothing needs to
be done in KVM. Continue TDX vcpu execution.

Signed-off-by Isaku Yamahata <[email protected]>

Reviewed-by Paolo Bonzini <[email protected]>

---
  arch/x86/include/uapi/asm/vmx.h | 1 +
  arch/x86/kvm/vmx/tdx.c          | 7 +++++++
  2 files changed, 8 insertions(+)

arch/x86/include/uapi/asm/vmx.h +0 -0

index a5faf6d88f1b..b3a30ef3efdd 100644

arch/x86/include/uapi/asm/vmx.h +0 -0

  #define EXIT_REASON_INTERRUPT_WINDOW    7
  #define EXIT_REASON_NMI_WINDOW          8

arch/x86/kvm/vmx/tdx.c +0 -0

index cba0fd5029be..2f68e6f2b53a 100644

arch/x86/kvm/vmx/tdx.c +3 -0

   #SMI is delivered and handled right after
+		 * SEAMRET, nothing needs to be done in KVM.
+		 */
+		return 1;
  	default:
  		break;

  	}

From: Isaku Yamahata <[email protected]>

On EPT violation, call a common function, __vmx_handle_ept_violation() to trigger x86 MMU code. On EPT misconfiguration, exit to ring 3 with KVM_EXIT_UNKNOWN. because EPT misconfiguration can't happen as MMIO is trigged by TDG.VP.VMCALL. No point to set a misconfiguration value for the

fast path.

Signed-off-by Isaku Yamahata <[email protected]>

---
v14 -> v15:
- use PFERR_GUEST_ENC_MASK to tell the fault is private

Signed-off-by Isaku Yamahata <[email protected]>

---
 arch/x86/kvm/vmx/common.h |  3 +++
 arch/x86/kvm/vmx/tdx.c    | 49 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 52 insertions(+)

arch/x86/kvm/vmx/common.h +3 -0

index 632af7a76d0a..027aa4175d2c 100644
@@ -87,6 +87,9 @@ static inline int __vmx_handle_ept_violation(struct kvm_vcpu *vcpu, gpa_t gpa,
 	error_code |= (exit_qualification & EPT_VIOLATION_GVA_TRANSLATED) != 0 ?
 	       PFERR_GUEST_FINAL_MASK : PFERR_GUEST_PAGE_MASK;
 
+	if (kvm_is_private_gpa(vcpu->kvm, gpa))
+		error_code |= PFERR_GUEST_ENC_MASK;
+
 	return kvm_mmu_page_fault(vcpu, gpa, error_code, NULL, 0);
 }
 

arch/x86/kvm/vmx/tdx.c +49 -0

index 2f68e6f2b53a..0db80fa020d2 100644
@@ -1285,6 +1285,51 @@ void tdx_deliver_interrupt(struct kvm_lapic *apic, int delivery_mode,
 	__vmx_deliver_posted_interrupt(vcpu, &tdx->pi_desc, vector);
 }
 
+static int tdx_handle_ept_violation(struct kvm_vcpu *vcpu)
+{
+	unsigned long exit_qual;
+
+	if (kvm_is_private_gpa(vcpu->kvm, tdexit_gpa(vcpu))) {
+		/*
+		 * Always treat SEPT violations as write faults.  Ignore the
+		 * EXIT_QUALIFICATION reported by TDX-SEAM for SEPT violations.
+		 * TD private pages are always RWX in the SEPT tables,
+		 * i.e. they're always mapped writable.  Just as importantly,
+		 * treating SEPT violations as write faults is necessary to
+		 * avoid COW allocations, which will cause TDAUGPAGE failures
+		 * due to aliasing a single HPA to multiple GPAs.
+		 */
+#define TDX_SEPT_VIOLATION_EXIT_QUAL	EPT_VIOLATION_ACC_WRITE
+		exit_qual = TDX_SEPT_VIOLATION_EXIT_QUAL;
+	} else {
+		exit_qual = tdexit_exit_qual(vcpu);
+		if (exit_qual & EPT_VIOLATION_ACC_INSTR) {
+			pr_warn("kvm: TDX instr fetch to shared GPA = 0x%lx @ RIP = 0x%lx\n",
+				tdexit_gpa(vcpu), kvm_rip_read(vcpu));
+			vcpu->run->exit_reason = KVM_EXIT_EXCEPTION;
+			vcpu->run->ex.exception = PF_VECTOR;
+			vcpu->run->ex.error_code = exit_qual;
+			return 0;
+		}
+	}
+
+	trace_kvm_page_fault(vcpu, tdexit_gpa(vcpu), exit_qual);
+	return __vmx_handle_ept_violation(vcpu, tdexit_gpa(vcpu), exit_qual);
+}
+
+static int tdx_handle_ept_misconfig(struct kvm_vcpu *vcpu)
+{
+	WARN_ON_ONCE(1);
+
+	vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
+	vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON;
+	vcpu->run->internal.ndata = 2;
+	vcpu->run->internal.data[0] = EXIT_REASON_EPT_MISCONFIG;
+	vcpu->run->internal.data[1] = vcpu->arch.last_vmentry_cpu;
+
+	return 0;
+}
+
 int tdx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t fastpath)
 {
 	union tdx_exit_reason exit_reason = to_tdx(vcpu)->exit_reason;

@@ -1345,6 +1390,10 @@ int tdx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t fastpath)
 	WARN_ON_ONCE(fastpath != EXIT_FASTPATH_NONE);
 
 	switch (exit_reason.basic) {
+	case EXIT_REASON_EPT_VIOLATION:
+		return tdx_handle_ept_violation(vcpu);
+	case EXIT_REASON_EPT_MISCONFIG:
+		return tdx_handle_ept_misconfig(vcpu);
 	case EXIT_REASON_OTHER_SMI:
 		/*
 		 * If reach here, it's not a Machine Check System Management

arch/x86/kvm/vmx/tdx.c +9 -0

+static int tdx_handle_ept_violation(struct kvm_vcpu *vcpu)
+{
+	unsigned long exit_qual;
+
+	if (kvm_is_private_gpa(vcpu->kvm, tdexit_gpa(vcpu))) {
+		/*
+		 * Always treat SEPT violations as write faults.  Ignore the
+		 * EXIT_QUALIFICATION reported by TDX-SEAM for SEPT violations.
+		 * TD private pages are always RWX in the SEPT tables,

arch/x86/kvm/vmx/tdx.c +26 -0

+			pr_warn("kvm: TDX instr fetch to shared GPA = 0x%lx @ RIP = 0x%lx\n",
+				tdexit_gpa(vcpu), kvm_rip_read(vcpu));
+			vcpu->run->exit_reason = KVM_EXIT_EXCEPTION;
+			vcpu->run->ex.exception = PF_VECTOR;
+			vcpu->run->ex.error_code = exit_qual;
+			return 0;
+		}
+	}
+
+	trace_kvm_page_fault(vcpu, tdexit_gpa(vcpu), exit_qual);
+	return __vmx_handle_ept_violation(vcpu, tdexit_gpa(vcpu), exit_qual);
+}
+
+static int tdx_handle_ept_misconfig(struct kvm_vcpu *vcpu)
+{
+	WARN_ON_ONCE(1);
+
+	vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
+	vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON;
+	vcpu->run->internal.ndata = 2;
+	vcpu->run->internal.data[0] = EXIT_REASON_EPT_MISCONFIG;
+	vcpu->run->internal.data[1] = vcpu->arch.last_vmentry_cpu;
+
+	return 0;
+}
+

int tdx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t fastpath)

{
	union tdx_exit_reason exit_reason = to_tdx(vcpu)->exit_reason;

arch/x86/kvm/vmx/tdx.c +0 -0

 	case EXIT_REASON_OTHER_SMI:
 		/*
 		 * If reach here, it's not a Machine Check System Management

Signature · 1 line

2.25.1

Quoted 14 lines

arch/x86/kvm/vmx/tdx.c +14 -0

+static int tdx_handle_ept_violation(struct kvm_vcpu *vcpu)
+{
+	unsigned long exit_qual;
+
+	if (kvm_is_private_gpa(vcpu->kvm, tdexit_gpa(vcpu))) {
+		/*
+		 * Always treat SEPT violations as write faults.  Ignore the
+		 * EXIT_QUALIFICATION reported by TDX-SEAM for SEPT violations.
+		 * TD private pages are always RWX in the SEPT tables,
+		 * i.e. they're always mapped writable.  Just as importantly,
+		 * treating SEPT violations as write faults is necessary to
+		 * avoid COW allocations, which will cause TDAUGPAGE failures
+		 * due to aliasing a single HPA to multiple GPAs.
+		 */

arch/x86/kvm/vmx/tdx.c void tdx_deliver_interrupt(struct kvm_lapic *apic, int delivery_mode, · line 1306

Quoted 5 lines

arch/x86/kvm/vmx/tdx.c +5 -0

+#define TDX_SEPT_VIOLATION_EXIT_QUAL	EPT_VIOLATION_ACC_WRITE
+		exit_qual = TDX_SEPT_VIOLATION_EXIT_QUAL;
+	} else {
+		exit_qual = tdexit_exit_qual(vcpu);
+		if (exit_qual & EPT_VIOLATION_ACC_INSTR) {

Unless the CPU has a bug, instruction fetch in TD from shared memory causes a #PF. I think you can add a comment for this.

Maybe KVM_BUG_ON() is more appropriate as it signifies a potential bug.

Quoted 32 lines

arch/x86/kvm/vmx/tdx.c +26 -0

+			pr_warn("kvm: TDX instr fetch to shared GPA = 0x%lx @ RIP = 0x%lx\n",
+				tdexit_gpa(vcpu), kvm_rip_read(vcpu));
+			vcpu->run->exit_reason = KVM_EXIT_EXCEPTION;
+			vcpu->run->ex.exception = PF_VECTOR;
+			vcpu->run->ex.error_code = exit_qual;
+			return 0;
+		}
+	}
+
+	trace_kvm_page_fault(vcpu, tdexit_gpa(vcpu), exit_qual);
+	return __vmx_handle_ept_violation(vcpu, tdexit_gpa(vcpu), exit_qual);
+}
+
+static int tdx_handle_ept_misconfig(struct kvm_vcpu *vcpu)
+{
+	WARN_ON_ONCE(1);
+
+	vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
+	vcpu->run->internal.suberror = KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON;
+	vcpu->run->internal.ndata = 2;
+	vcpu->run->internal.data[0] = EXIT_REASON_EPT_MISCONFIG;
+	vcpu->run->internal.data[1] = vcpu->arch.last_vmentry_cpu;
+
+	return 0;
+}
+

int tdx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t fastpath)

{
	union tdx_exit_reason exit_reason = to_tdx(vcpu)->exit_reason;

arch/x86/kvm/vmx/tdx.c +0 -0

@@ -1345,6 +1390,10 @@ int tdx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t fastpath)
 	WARN_ON_ONCE(fastpath != EXIT_FASTPATH_NONE);
 

arch/x86/kvm/vmx/tdx.c int tdx_handle_exit(struct kvm_vcpu *vcpu, fastpath_t fastpath) · line 1396

Quoted 5 lines

arch/x86/kvm/vmx/tdx.c +4 -0

 	switch (exit_reason.basic) {
+	case EXIT_REASON_EPT_VIOLATION:
+		return tdx_handle_ept_violation(vcpu);
+	case EXIT_REASON_EPT_MISCONFIG:
+		return tdx_handle_ept_misconfig(vcpu);

Handling EPT misconfiguration can be dropped because the "default" case handles all unexpected exits in the same way

Quoted 14 lines · Chao Gao

Quoted 14 lines

(quoted diff) +14 -0

+static int tdx_handle_ept_violation(struct kvm_vcpu *vcpu)
+{
+	unsigned long exit_qual;
+
+	if (kvm_is_private_gpa(vcpu->kvm, tdexit_gpa(vcpu))) {
+		/*
+		 * Always treat SEPT violations as write faults.  Ignore the
+		 * EXIT_QUALIFICATION reported by TDX-SEAM for SEPT violations.
+		 * TD private pages are always RWX in the SEPT tables,
+		 * i.e. they're always mapped writable.  Just as importantly,
+		 * treating SEPT violations as write faults is necessary to
+		 * avoid COW allocations, which will cause TDAUGPAGE failures
+		 * due to aliasing a single HPA to multiple GPAs.
+		 */

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+#define TDX_SEPT_VIOLATION_EXIT_QUAL	EPT_VIOLATION_ACC_WRITE
+		exit_qual = TDX_SEPT_VIOLATION_EXIT_QUAL;
+	} else {
+		exit_qual = tdexit_exit_qual(vcpu);
+		if (exit_qual & EPT_VIOLATION_ACC_INSTR) {

Unless the CPU has a bug, instruction fetch in TD from shared memory causes a #PF. I think you can add a comment for this.

Yes.

Quoted 1 line

Maybe KVM_BUG_ON() is more appropriate as it signifies a potential bug.

Bug of what component? CPU. If so, I think KVM_EXIT_INTERNAL_ERROR + KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON is more appropriate.

Hi Isaku,

Quoted 29 lines · Isaku Yamahata

Quoted 14 lines · Chao Gao

Quoted 14 lines

(quoted diff) +14 -0

+static int tdx_handle_ept_violation(struct kvm_vcpu *vcpu)
+{
+	unsigned long exit_qual;
+
+	if (kvm_is_private_gpa(vcpu->kvm, tdexit_gpa(vcpu))) {
+		/*
+		 * Always treat SEPT violations as write faults.  Ignore the
+		 * EXIT_QUALIFICATION reported by TDX-SEAM for SEPT violations.
+		 * TD private pages are always RWX in the SEPT tables,
+		 * i.e. they're always mapped writable.  Just as importantly,
+		 * treating SEPT violations as write faults is necessary to
+		 * avoid COW allocations, which will cause TDAUGPAGE failures
+		 * due to aliasing a single HPA to multiple GPAs.
+		 */

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+#define TDX_SEPT_VIOLATION_EXIT_QUAL	EPT_VIOLATION_ACC_WRITE
+		exit_qual = TDX_SEPT_VIOLATION_EXIT_QUAL;
+	} else {
+		exit_qual = tdexit_exit_qual(vcpu);
+		if (exit_qual & EPT_VIOLATION_ACC_INSTR) {

Unless the CPU has a bug, instruction fetch in TD from shared memory causes a #PF. I think you can add a comment for this.

Yes.

Quoted 5 lines

Quoted 1 line

Maybe KVM_BUG_ON() is more appropriate as it signifies a potential bug.

Bug of what component? CPU. If so, I think KVM_EXIT_INTERNAL_ERROR + KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON is more appropriate.

Is below what you have in mind?

arch/x86/kvm/vmx/tdx.c +10 -3

index 499c6cd9633f..bd30b4c4d710 100644
@@ -1305,11 +1305,18 @@ static int tdx_handle_ept_violation(struct kvm_vcpu *vcpu)
 	} else {
 		exit_qual = tdexit_exit_qual(vcpu);
 		if (exit_qual & EPT_VIOLATION_ACC_INSTR) {
+			/*
+			 * Instruction fetch in TD from shared memory
+			 * causes a #PF.
+			 */
 			pr_warn("kvm: TDX instr fetch to shared GPA = 0x%lx @ RIP = 0x%lx\n",
 				tdexit_gpa(vcpu), kvm_rip_read(vcpu));
-			vcpu->run->exit_reason = KVM_EXIT_EXCEPTION;
-			vcpu->run->ex.exception = PF_VECTOR;
-			vcpu->run->ex.error_code = exit_qual;
+			vcpu->run->exit_reason = KVM_EXIT_INTERNAL_ERROR;
+			vcpu->run->internal.suberror =
+				KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON;
+			vcpu->run->internal.ndata = 2;
+			vcpu->run->internal.data[0] = EXIT_REASON_EPT_VIOLATION;
+			vcpu->run->internal.data[1] = vcpu->arch.last_vmentry_cpu;
 			return 0;
 		}
 	}

Thank you

Reinette

Quoted 32 lines · Reinette Chatre

Quoted 31 lines · Isaku Yamahata

Quoted 14 lines · Chao Gao

Quoted 14 lines

(quoted diff) +14 -0

+static int tdx_handle_ept_violation(struct kvm_vcpu *vcpu)
+{
+	unsigned long exit_qual;
+
+	if (kvm_is_private_gpa(vcpu->kvm, tdexit_gpa(vcpu))) {
+		/*
+		 * Always treat SEPT violations as write faults.  Ignore the
+		 * EXIT_QUALIFICATION reported by TDX-SEAM for SEPT violations.
+		 * TD private pages are always RWX in the SEPT tables,
+		 * i.e. they're always mapped writable.  Just as importantly,
+		 * treating SEPT violations as write faults is necessary to
+		 * avoid COW allocations, which will cause TDAUGPAGE failures
+		 * due to aliasing a single HPA to multiple GPAs.
+		 */

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+#define TDX_SEPT_VIOLATION_EXIT_QUAL	EPT_VIOLATION_ACC_WRITE
+		exit_qual = TDX_SEPT_VIOLATION_EXIT_QUAL;
+	} else {
+		exit_qual = tdexit_exit_qual(vcpu);
+		if (exit_qual & EPT_VIOLATION_ACC_INSTR) {

Unless the CPU has a bug, instruction fetch in TD from shared memory causes a #PF. I think you can add a comment for this.

Yes.

Quoted 1 line

Maybe KVM_BUG_ON() is more appropriate as it signifies a potential bug.

Quoted 5 lines

Quoted 3 lines

Bug of what component? CPU. If so, I think KVM_EXIT_INTERNAL_ERROR + KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON is more appropriate.

Is below what you have in mind?

Yes. data[0] should be the raw value of exit reason if possible. data[2] should be exit_qual. Hmm, I don't find document on data[] for

KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON.
Qemu doesn't assumt ndata = 2. Just report all data within ndata.

Hi Isaku,

Quoted 34 lines · Isaku Yamahata

Quoted 32 lines · Reinette Chatre

Quoted 31 lines · Isaku Yamahata

Quoted 14 lines · Chao Gao

Quoted 14 lines

(quoted diff) +14 -0

+static int tdx_handle_ept_violation(struct kvm_vcpu *vcpu)
+{
+	unsigned long exit_qual;
+
+	if (kvm_is_private_gpa(vcpu->kvm, tdexit_gpa(vcpu))) {
+		/*
+		 * Always treat SEPT violations as write faults.  Ignore the
+		 * EXIT_QUALIFICATION reported by TDX-SEAM for SEPT violations.
+		 * TD private pages are always RWX in the SEPT tables,
+		 * i.e. they're always mapped writable.  Just as importantly,
+		 * treating SEPT violations as write faults is necessary to
+		 * avoid COW allocations, which will cause TDAUGPAGE failures
+		 * due to aliasing a single HPA to multiple GPAs.
+		 */

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+#define TDX_SEPT_VIOLATION_EXIT_QUAL	EPT_VIOLATION_ACC_WRITE
+		exit_qual = TDX_SEPT_VIOLATION_EXIT_QUAL;
+	} else {
+		exit_qual = tdexit_exit_qual(vcpu);
+		if (exit_qual & EPT_VIOLATION_ACC_INSTR) {

Unless the CPU has a bug, instruction fetch in TD from shared memory causes a #PF. I think you can add a comment for this.

Yes.

Quoted 1 line

Maybe KVM_BUG_ON() is more appropriate as it signifies a potential bug.

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

Bug of what component? CPU. If so, I think KVM_EXIT_INTERNAL_ERROR + KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON is more appropriate.

Is below what you have in mind?

Yes. data[0] should be the raw value of exit reason if possible. data[2] should be exit_qual. Hmm, I don't find document on data[] for

Did you perhaps intend to write "data[1] should be exit_qual" or would you like to see ndata = 3? I followed existing usages, for example [1] and [2], that have ndata = 2 with "data[1] = vcpu->arch.last_vmentry_cpu".

Quoted 2 lines

KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON.
Qemu doesn't assumt ndata = 2. Just report all data within ndata.

I am not sure I interpreted your response correctly so I share one possible snippet below as I interpret it. Could you please check where I misinterpreted you? I could also make ndata = 3 to break the existing custom and add "data[2] = vcpu->arch.last_vmentry_cpu" to match existing pattern. What do you think?

Quoted 50 lines · Reinette Chatre

Hi Isaku,

Quoted 34 lines · Isaku Yamahata

Quoted 32 lines · Reinette Chatre

Quoted 31 lines · Isaku Yamahata

Quoted 14 lines · Chao Gao

Quoted 14 lines

(quoted diff) +14 -0

+static int tdx_handle_ept_violation(struct kvm_vcpu *vcpu)
+{
+	unsigned long exit_qual;
+
+	if (kvm_is_private_gpa(vcpu->kvm, tdexit_gpa(vcpu))) {
+		/*
+		 * Always treat SEPT violations as write faults.  Ignore the
+		 * EXIT_QUALIFICATION reported by TDX-SEAM for SEPT violations.
+		 * TD private pages are always RWX in the SEPT tables,
+		 * i.e. they're always mapped writable.  Just as importantly,
+		 * treating SEPT violations as write faults is necessary to
+		 * avoid COW allocations, which will cause TDAUGPAGE failures
+		 * due to aliasing a single HPA to multiple GPAs.
+		 */

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +5 -0

+#define TDX_SEPT_VIOLATION_EXIT_QUAL	EPT_VIOLATION_ACC_WRITE
+		exit_qual = TDX_SEPT_VIOLATION_EXIT_QUAL;
+	} else {
+		exit_qual = tdexit_exit_qual(vcpu);
+		if (exit_qual & EPT_VIOLATION_ACC_INSTR) {

Unless the CPU has a bug, instruction fetch in TD from shared memory causes a #PF. I think you can add a comment for this.

Yes.

Quoted 1 line

Maybe KVM_BUG_ON() is more appropriate as it signifies a potential bug.

Quoted 8 lines

Quoted 5 lines

Quoted 3 lines

Bug of what component? CPU. If so, I think KVM_EXIT_INTERNAL_ERROR + KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON is more appropriate.

Is below what you have in mind?

Yes. data[0] should be the raw value of exit reason if possible. data[2] should be exit_qual. Hmm, I don't find document on data[] for

Did you perhaps intend to write "data[1] should be exit_qual" or would you like to see ndata = 3? I followed existing usages, for example [1] and [2], that have ndata = 2 with "data[1] = vcpu->arch.last_vmentry_cpu".

Quoted 9 lines

Quoted 2 lines

KVM_INTERNAL_ERROR_UNEXPECTED_EXIT_REASON.
Qemu doesn't assumt ndata = 2. Just report all data within ndata.

I am not sure I interpreted your response correctly so I share one possible snippet below as I interpret it. Could you please check where I misinterpreted you? I could also make ndata = 3 to break the existing custom and add "data[2] = vcpu->arch.last_vmentry_cpu" to match existing pattern. What do you think?

Sorry, I wasn't clear enough. I meant
ndata = 3;
data[0] = exit_reason.full;
data[1] = vcpu->arch.last_vmentry_cpu;
data[2] = exit_qual;

Because I hesitate to change the meaning of data[1] from other usage, I appended exit_qual as data[2].