Re: [PATCH] lguest: simplify lguest_iret

[Rusty Russell]

Denys Vlasenko <dvlasenk@xxxxxxxxxx> writes:
> Signed-off-by: Denys Vlasenko <dvlasenk@xxxxxxxxxx>
> CC: lguest@xxxxxxxxxxxxxxxx
> CC: x86@xxxxxxxxxx
> CC: linux-kernel@xxxxxxxxxxxxxxx

Oh, thanks, applied!

And now it's down to one instruction, we could change
try_deliver_interrupt() to handle this case (rather than ignoring the
interrupt altogether): just jump to the handler and leave the
stack set up.

Here's a pair of inline patches which attempt to do that (untested!).

Thanks,
Rusty.

lguest: suppress interrupts for single insn, not range.

The last patch reduced our interrupt-suppression region to one address,
so simplify the code somewhat.

Also, remove the obsolete undefined instruction ranges and the comment
which refers to lguest_guest.S instead of head_32.S.

Signed-off-by: Rusty Russell <rusty@xxxxxxxxxxxxxxx>

diff --git a/arch/x86/include/asm/lguest.h b/arch/x86/include/asm/lguest.h
index e2d4a4afa8c3..3bbc07a57a31 100644
--- a/arch/x86/include/asm/lguest.h
+++ b/arch/x86/include/asm/lguest.h
@@ -20,13 +20,10 @@ extern unsigned long switcher_addr;
 /* Found in switcher.S */
 extern unsigned long default_idt_entries[];
 
-/* Declarations for definitions in lguest_guest.S */
-extern char lguest_noirq_start[], lguest_noirq_end[];
+/* Declarations for definitions in arch/x86/lguest/head_32.S */
+extern char lguest_noirq_iret[];
 extern const char lgstart_cli[], lgend_cli[];
-extern const char lgstart_sti[], lgend_sti[];
-extern const char lgstart_popf[], lgend_popf[];
 extern const char lgstart_pushf[], lgend_pushf[];
-extern const char lgstart_iret[], lgend_iret[];
 
 extern void lguest_iret(void);
 extern void lguest_init(void);
diff --git a/arch/x86/lguest/boot.c b/arch/x86/lguest/boot.c
index ac4453d8520e..4c8cf656f21f 100644
--- a/arch/x86/lguest/boot.c
+++ b/arch/x86/lguest/boot.c
@@ -87,8 +87,7 @@
 
 struct lguest_data lguest_data = {
 	.hcall_status = { [0 ... LHCALL_RING_SIZE-1] = 0xFF },
-	.noirq_start = (u32)lguest_noirq_start,
-	.noirq_end = (u32)lguest_noirq_end,
+	.noirq_iret = (u32)lguest_noirq_iret,
 	.kernel_address = PAGE_OFFSET,
 	.blocked_interrupts = { 1 }, /* Block timer interrupts */
 	.syscall_vec = SYSCALL_VECTOR,
diff --git a/arch/x86/lguest/head_32.S b/arch/x86/lguest/head_32.S
index 583732cc5d18..128fe93161b4 100644
--- a/arch/x86/lguest/head_32.S
+++ b/arch/x86/lguest/head_32.S
@@ -133,9 +133,8 @@ ENTRY(lg_restore_fl)
 	ret
 /*:*/
 
-/* These demark the EIP range where host should never deliver interrupts. */
-.global lguest_noirq_start
-.global lguest_noirq_end
+/* These demark the EIP where host should never deliver interrupts. */
+.global lguest_noirq_iret
 
 /*M:004
  * When the Host reflects a trap or injects an interrupt into the Guest, it
@@ -174,12 +173,11 @@ ENTRY(lg_restore_fl)
  *
  * The second is harder: copying eflags to lguest_data.irq_enabled will turn
  * interrupts on before we're finished, so we could be interrupted before we
- * return to userspace or wherever.  Our solution to this is to surround the
- * code with lguest_noirq_start: and lguest_noirq_end: labels.  We tell the
+ * return to userspace or wherever.  Our solution to this is to tell the
  * Host that it is *never* to interrupt us there, even if interrupts seem to be
  * enabled. (It's not necessary to protect pop instruction, since
- * data gets updated only after it completes, so we end up surrounding
- * just one instruction, iret).
+ * data gets updated only after it completes, so we only need to protect
+ * one instruction, iret).
  */
 ENTRY(lguest_iret)
 	pushl	2*4(%esp)
@@ -190,6 +188,5 @@ ENTRY(lguest_iret)
 	 * prefix makes sure we use the stack segment, which is still valid.
 	 */
 	popl	%ss:lguest_data+LGUEST_DATA_irq_enabled
-lguest_noirq_start:
+lguest_noirq_iret:
 	iret
-lguest_noirq_end:
diff --git a/drivers/lguest/hypercalls.c b/drivers/lguest/hypercalls.c
index 1219af493c0f..19a32280731d 100644
--- a/drivers/lguest/hypercalls.c
+++ b/drivers/lguest/hypercalls.c
@@ -211,10 +211,9 @@ static void initialize(struct lg_cpu *cpu)
 
 	/*
 	 * The Guest tells us where we're not to deliver interrupts by putting
-	 * the range of addresses into "struct lguest_data".
+	 * the instruction address into "struct lguest_data".
 	 */
-	if (get_user(cpu->lg->noirq_start, &cpu->lg->lguest_data->noirq_start)
-	    || get_user(cpu->lg->noirq_end, &cpu->lg->lguest_data->noirq_end))
+	if (get_user(cpu->lg->noirq_iret, &cpu->lg->lguest_data->noirq_iret))
 		kill_guest(cpu, "bad guest page %p", cpu->lg->lguest_data);
 
 	/*
diff --git a/drivers/lguest/interrupts_and_traps.c b/drivers/lguest/interrupts_and_traps.c
index 70dfcdc29f1f..6d4c072b61e1 100644
--- a/drivers/lguest/interrupts_and_traps.c
+++ b/drivers/lguest/interrupts_and_traps.c
@@ -204,8 +204,7 @@ void try_deliver_interrupt(struct lg_cpu *cpu, unsigned int irq, bool more)
 	 * They may be in the middle of an iret, where they asked us never to
 	 * deliver interrupts.
 	 */
-	if (cpu->regs->eip >= cpu->lg->noirq_start &&
-	   (cpu->regs->eip < cpu->lg->noirq_end))
+	if (cpu->regs->eip == cpu->lg->noirq_iret)
 		return;
 
 	/* If they're halted, interrupts restart them. */
@@ -395,8 +394,9 @@ static bool direct_trap(unsigned int num)
  * The Guest has the ability to turn its interrupt gates into trap gates,
  * if it is careful.  The Host will let trap gates can go directly to the
  * Guest, but the Guest needs the interrupts atomically disabled for an
- * interrupt gate.  It can do this by pointing the trap gate at instructions
- * within noirq_start and noirq_end, where it can safely disable interrupts.
+ * interrupt gate.  The Host could provide a mechanism to register more
+ * "no-interrupt" regions, and the Guest could point the trap gate at
+ * instructions within that region, where it can safely disable interrupts.
  */
 
 /*M:006
diff --git a/drivers/lguest/lg.h b/drivers/lguest/lg.h
index 307e8b39e7d1..ac8ad0461e80 100644
--- a/drivers/lguest/lg.h
+++ b/drivers/lguest/lg.h
@@ -102,7 +102,7 @@ struct lguest {
 
 	struct pgdir pgdirs[4];
 
-	unsigned long noirq_start, noirq_end;
+	unsigned long noirq_iret;
 
 	unsigned int stack_pages;
 	u32 tsc_khz;
diff --git a/include/linux/lguest.h b/include/linux/lguest.h
index 9962c6bb1311..6db19f35f7c5 100644
--- a/include/linux/lguest.h
+++ b/include/linux/lguest.h
@@ -61,8 +61,8 @@ struct lguest_data {
 	u32 tsc_khz;
 
 /* Fields initialized by the Guest at boot: */
-	/* Instruction range to suppress interrupts even if enabled */
-	unsigned long noirq_start, noirq_end;
+	/* Instruction to suppress interrupts even if enabled */
+	unsigned long noirq_iret;
 	/* Address above which page tables are all identical. */
 	unsigned long kernel_address;
 	/* The vector to try to use for system calls (0x40 or 0x80). */

lguest: handle traps on the "interrupt suppressed" iret instruction.

Lguest's "iret" is non-atomic, as it needs to restore the interrupt
state before the real iret (the guest can't actually suppress
interrupts).  For this reason, the host discards an interrupt if it
occurs in this (1-instruction) window.

We can do better, by emulating the iret execution, then immediately
setting up the interrupt handler.  In fact, we don't need to do much,
as emulating the iret and setting up th stack for the interrupt handler
basically cancel each other out.

Signed-off-by: Rusty Russell <rusty@xxxxxxxxxxxxxxx>

diff --git a/drivers/lguest/interrupts_and_traps.c b/drivers/lguest/interrupts_and_traps.c
index 70dfcdc29f1f..906aba0d762f 100644
--- a/drivers/lguest/interrupts_and_traps.c
+++ b/drivers/lguest/interrupts_and_traps.c
@@ -56,21 +56,16 @@ static void push_guest_stack(struct lg_cpu *cpu, unsigned long *gstack, u32 val)
 }
 
 /*H:210
- * The set_guest_interrupt() routine actually delivers the interrupt or
- * trap.  The mechanics of delivering traps and interrupts to the Guest are the
- * same, except some traps have an "error code" which gets pushed onto the
- * stack as well: the caller tells us if this is one.
- *
- * "lo" and "hi" are the two parts of the Interrupt Descriptor Table for this
- * interrupt or trap.  It's split into two parts for traditional reasons: gcc
- * on i386 used to be frightened by 64 bit numbers.
+ * The push_guest_interrupt_stack() routine saves Guest state on the stack for
+ * an interrupt or trap.  The mechanics of delivering traps and interrupts to
+ * the Guest are the same, except some traps have an "error code" which gets
+ * pushed onto the stack as well: the caller tells us if this is one.
  *
  * We set up the stack just like the CPU does for a real interrupt, so it's
  * identical for the Guest (and the standard "iret" instruction will undo
  * it).
  */
-static void set_guest_interrupt(struct lg_cpu *cpu, u32 lo, u32 hi,
-				bool has_err)
+static void push_guest_interrupt_stack(struct lg_cpu *cpu, bool has_err)
 {
 	unsigned long gstack, origstack;
 	u32 eflags, ss, irq_enable;
@@ -130,12 +125,28 @@ static void set_guest_interrupt(struct lg_cpu *cpu, u32 lo, u32 hi,
 	if (has_err)
 		push_guest_stack(cpu, &gstack, cpu->regs->errcode);
 
-	/*
-	 * Now we've pushed all the old state, we change the stack, the code
-	 * segment and the address to execute.
-	 */
+	/* Adjust the stack pointer and stack segment. */
 	cpu->regs->ss = ss;
 	cpu->regs->esp = virtstack + (gstack - origstack);
+}
+
+/*
+ * This actually makes the Guest start executing the given interrupt/trap
+ * handler.
+ *
+ * "lo" and "hi" are the two parts of the Interrupt Descriptor Table for this
+ * interrupt or trap.  It's split into two parts for traditional reasons: gcc
+ * on i386 used to be frightened by 64 bit numbers.
+ */
+static void guest_run_interrupt(struct lg_cpu *cpu, u32 lo, u32 hi)
+{
+	/* If we're already in the kernel, we don't change stacks. */
+	if ((cpu->regs->ss&0x3) != GUEST_PL)
+		cpu->regs->ss = cpu->esp1;
+
+	/*
+	 * Set the code segment and the address to execute.
+	 */
 	cpu->regs->cs = (__KERNEL_CS|GUEST_PL);
 	cpu->regs->eip = idt_address(lo, hi);
 
@@ -158,6 +169,24 @@ static void set_guest_interrupt(struct lg_cpu *cpu, u32 lo, u32 hi,
 			kill_guest(cpu, "Disabling interrupts");
 }
 
+/* This restores the eflags word which was pushed on the stack by a trap */
+static void restore_eflags(struct lg_cpu *cpu)
+{
+	/* This is the physical address of the stack. */
+	unsigned long stack_pa = guest_pa(cpu, cpu->regs->esp);
+
+	/*
+	 * Stack looks like this:
+	 * Address	Contents
+	 * esp		EIP
+	 * esp + 4	CS
+	 * esp + 8	EFLAGS
+	 */
+	cpu->regs->eflags = lgread(cpu, stack_pa + 8, u32);
+	cpu->regs->eflags &=
+		~(X86_EFLAGS_TF|X86_EFLAGS_VM|X86_EFLAGS_RF|X86_EFLAGS_NT);
+}
+
 /*H:205
  * Virtual Interrupts.
  *
@@ -200,14 +229,6 @@ void try_deliver_interrupt(struct lg_cpu *cpu, unsigned int irq, bool more)
 
 	BUG_ON(irq >= LGUEST_IRQS);
 
-	/*
-	 * They may be in the middle of an iret, where they asked us never to
-	 * deliver interrupts.
-	 */
-	if (cpu->regs->eip >= cpu->lg->noirq_start &&
-	   (cpu->regs->eip < cpu->lg->noirq_end))
-		return;
-
 	/* If they're halted, interrupts restart them. */
 	if (cpu->halted) {
 		/* Re-enable interrupts. */
@@ -237,12 +258,28 @@ void try_deliver_interrupt(struct lg_cpu *cpu, unsigned int irq, bool more)
 	if (idt_present(idt->a, idt->b)) {
 		/* OK, mark it no longer pending and deliver it. */
 		clear_bit(irq, cpu->irqs_pending);
+
 		/*
-		 * set_guest_interrupt() takes the interrupt descriptor and a
-		 * flag to say whether this interrupt pushes an error code onto
-		 * the stack as well: virtual interrupts never do.
+		 * They may be about to iret, where they asked us never to
+		 * deliver interrupts.  In this case, we can emulate that iret
+		 * then immediately deliver the interrupt.  This is bascially
+		 * a noop: the iret would pop the interrupt frame and restore
+		 * eflags, and then we'd set it up again.  So just restore the
+		 * eflags word and jump straight to the handler in this case.
 		 */
-		set_guest_interrupt(cpu, idt->a, idt->b, false);
+		if (cpu->regs->eip >= cpu->lg->noirq_start &&
+		    (cpu->regs->eip < cpu->lg->noirq_end)) {
+			restore_eflags(cpu);
+		} else {
+			/*
+			 * set_guest_interrupt() takes a flag to say whether
+			 * this interrupt pushes an error code onto the stack
+			 * as well: virtual interrupts never do.
+			 */
+			push_guest_interrupt_stack(cpu, false);
+		}
+		/* Actually make Guest cpu jump to handler. */
+		guest_run_interrupt(cpu, idt->a, idt->b);
 	}
 
 	/*
@@ -353,8 +390,9 @@ bool deliver_trap(struct lg_cpu *cpu, unsigned int num)
 	 */
 	if (!idt_present(cpu->arch.idt[num].a, cpu->arch.idt[num].b))
 		return false;
-	set_guest_interrupt(cpu, cpu->arch.idt[num].a,
-			    cpu->arch.idt[num].b, has_err(num));
+	push_guest_interrupt_stack(cpu, has_err(num));
+	guest_run_interrupt(cpu, cpu->arch.idt[num].a,
+			    cpu->arch.idt[num].b);
 	return true;
 }
 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/