Re: [RFC PATCH V5 15/15] x86/sev: Fix interrupt exit code paths from #HV exception

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 5/1/23 03:57, Tianyu Lan wrote:
From: Ashish Kalra <ashish.kalra@xxxxxxx>

Add checks in interrupt exit code paths in case of returns
to user mode to check if currently executing the #HV handler
then don't follow the irqentry_exit_to_user_mode path as
that can potentially cause the #HV handler to be
preempted and rescheduled on another CPU. Rescheduled #HV
handler on another cpu will cause interrupts to be handled
on a different cpu than the injected one, causing
invalid EOIs and missed/lost guest interrupts and
corresponding hangs and/or per-cpu IRQs handled on
non-intended cpu.

Signed-off-by: Ashish Kalra <ashish.kalra@xxxxxxx>

This should be merged into one of the appropriate #HV patches and just add Ashish with a Co-developed-by: tag where appropriate. This would be appropriate as a separate only if discovered after the series was merged.

---
Change since RFC v3:
        * Add check of hv_handling_events in the do_exc_hv()
        	 to avoid nested entry.
---
  arch/x86/include/asm/idtentry.h | 66 +++++++++++++++++++++++++++++++++
  arch/x86/kernel/sev.c           | 37 +++++++++++++++++-
  2 files changed, 102 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/idtentry.h b/arch/x86/include/asm/idtentry.h
index b0f3501b2767..415b7e14c227 100644
--- a/arch/x86/include/asm/idtentry.h
+++ b/arch/x86/include/asm/idtentry.h
@@ -13,6 +13,10 @@
#include <asm/irq_stack.h> +#ifdef CONFIG_AMD_MEM_ENCRYPT
+noinstr void irqentry_exit_hv_cond(struct pt_regs *regs, irqentry_state_t state);
+#endif
+
  /**
   * DECLARE_IDTENTRY - Declare functions for simple IDT entry points
   *		      No error code pushed by hardware
@@ -176,6 +180,7 @@ __visible noinstr void func(struct pt_regs *regs, unsigned long error_code)
  #define DECLARE_IDTENTRY_IRQ(vector, func)				\
  	DECLARE_IDTENTRY_ERRORCODE(vector, func)
+#ifndef CONFIG_AMD_MEM_ENCRYPT
  /**
   * DEFINE_IDTENTRY_IRQ - Emit code for device interrupt IDT entry points
   * @func:	Function name of the entry point
@@ -205,6 +210,26 @@ __visible noinstr void func(struct pt_regs *regs,			\
  }									\
  									\
  static noinline void __##func(struct pt_regs *regs, u32 vector)
+#else
+
+#define DEFINE_IDTENTRY_IRQ(func)					\
+static void __##func(struct pt_regs *regs, u32 vector);		\
+									\
+__visible noinstr void func(struct pt_regs *regs,			\
+			    unsigned long error_code)			\
+{									\
+	irqentry_state_t state = irqentry_enter(regs);			\
+	u32 vector = (u32)(u8)error_code;				\
+									\
+	instrumentation_begin();					\
+	kvm_set_cpu_l1tf_flush_l1d();					\
+	run_irq_on_irqstack_cond(__##func, regs, vector);		\
+	instrumentation_end();						\
+	irqentry_exit_hv_cond(regs, state);				\
+}									\
+									\
+static noinline void __##func(struct pt_regs *regs, u32 vector)
+#endif

This seems like a lot of code duplication. Can't the difference (which is the call to irqentry_exit() vs irqentry_exit_hv_cond() be #ifdef'd in the current defintion?

Ditto for the changes below, too?

Thanks,
Tom

/**
   * DECLARE_IDTENTRY_SYSVEC - Declare functions for system vector entry points
@@ -221,6 +246,7 @@ static noinline void __##func(struct pt_regs *regs, u32 vector)
  #define DECLARE_IDTENTRY_SYSVEC(vector, func)				\
  	DECLARE_IDTENTRY(vector, func)
+#ifndef CONFIG_AMD_MEM_ENCRYPT
  /**
   * DEFINE_IDTENTRY_SYSVEC - Emit code for system vector IDT entry points
   * @func:	Function name of the entry point
@@ -245,6 +271,26 @@ __visible noinstr void func(struct pt_regs *regs)			\
  }									\
  									\
  static noinline void __##func(struct pt_regs *regs)
+#else
+
+#define DEFINE_IDTENTRY_SYSVEC(func)					\
+static void __##func(struct pt_regs *regs);				\
+									\
+__visible noinstr void func(struct pt_regs *regs)			\
+{									\
+	irqentry_state_t state = irqentry_enter(regs);			\
+									\
+	instrumentation_begin();					\
+	kvm_set_cpu_l1tf_flush_l1d();					\
+	run_sysvec_on_irqstack_cond(__##func, regs);			\
+	instrumentation_end();						\
+	irqentry_exit_hv_cond(regs, state);				\
+}									\
+									\
+static noinline void __##func(struct pt_regs *regs)
+#endif
+
+#ifndef CONFIG_AMD_MEM_ENCRYPT
/**
   * DEFINE_IDTENTRY_SYSVEC_SIMPLE - Emit code for simple system vector IDT
@@ -274,6 +320,26 @@ __visible noinstr void func(struct pt_regs *regs)			\
  }									\
  									\
  static __always_inline void __##func(struct pt_regs *regs)
+#else
+
+#define DEFINE_IDTENTRY_SYSVEC_SIMPLE(func)				\
+static __always_inline void __##func(struct pt_regs *regs);		\
+									\
+__visible noinstr void func(struct pt_regs *regs)			\
+{									\
+	irqentry_state_t state = irqentry_enter(regs);			\
+									\
+	instrumentation_begin();					\
+	__irq_enter_raw();						\
+	kvm_set_cpu_l1tf_flush_l1d();					\
+	__##func(regs);						\
+	__irq_exit_raw();						\
+	instrumentation_end();						\
+	irqentry_exit_hv_cond(regs, state);				\
+}									\
+									\
+static __always_inline void __##func(struct pt_regs *regs)
+#endif
/**
   * DECLARE_IDTENTRY_XENCB - Declare functions for XEN HV callback entry point
diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c
index b6becf158598..69b55075ddfe 100644
--- a/arch/x86/kernel/sev.c
+++ b/arch/x86/kernel/sev.c
@@ -149,6 +149,10 @@ struct sev_hv_doorbell_page {
struct sev_snp_runtime_data {
  	struct sev_hv_doorbell_page hv_doorbell_page;
+	/*
+	 * Indication that we are currently handling #HV events.
+	 */
+	bool hv_handling_events;
  };
static DEFINE_PER_CPU(struct sev_snp_runtime_data*, snp_runtime_data);
@@ -204,6 +208,12 @@ static void do_exc_hv(struct pt_regs *regs)
  {
  	union hv_pending_events pending_events;
+ /* Avoid nested entry. */
+	if (this_cpu_read(snp_runtime_data)->hv_handling_events)
+		return;
+
+	this_cpu_read(snp_runtime_data)->hv_handling_events = true;
+
  	while (sev_hv_pending()) {
  		pending_events.events = xchg(
  			&sev_snp_current_doorbell_page()->pending_events.events,
@@ -218,7 +228,7 @@ static void do_exc_hv(struct pt_regs *regs)
  #endif
if (!pending_events.vector)
-			return;
+			goto out;
if (pending_events.vector < FIRST_EXTERNAL_VECTOR) {
  			/* Exception vectors */
@@ -238,6 +248,9 @@ static void do_exc_hv(struct pt_regs *regs)
  			common_interrupt(regs, pending_events.vector);
  		}
  	}
+
+out:
+	this_cpu_read(snp_runtime_data)->hv_handling_events = false;
  }
static __always_inline bool on_vc_stack(struct pt_regs *regs)
@@ -2542,3 +2555,25 @@ static int __init snp_init_platform_device(void)
  	return 0;
  }
  device_initcall(snp_init_platform_device);
+
+noinstr void irqentry_exit_hv_cond(struct pt_regs *regs, irqentry_state_t state)
+{
+	/*
+	 * Check whether this returns to user mode, if so and if
+	 * we are currently executing the #HV handler then we don't
+	 * want to follow the irqentry_exit_to_user_mode path as
+	 * that can potentially cause the #HV handler to be
+	 * preempted and rescheduled on another CPU. Rescheduled #HV
+	 * handler on another cpu will cause interrupts to be handled
+	 * on a different cpu than the injected one, causing
+	 * invalid EOIs and missed/lost guest interrupts and
+	 * corresponding hangs and/or per-cpu IRQs handled on
+	 * non-intended cpu.
+	 */
+	if (user_mode(regs) &&
+	    this_cpu_read(snp_runtime_data)->hv_handling_events)
+		return;
+
+	/* follow normal interrupt return/exit path */
+	irqentry_exit(regs, state);
+}



[Index of Archives]     [KVM ARM]     [KVM ia64]     [KVM ppc]     [Virtualization Tools]     [Spice Development]     [Libvirt]     [Libvirt Users]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite Questions]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux