xen: use iret directly where possible

xen: use iret directly where possible

Most of the time we can simply use the iret instruction to exit the kernel, rather than having to use the iret hypercall - the only exception is if we're returning into vm86 mode, or from delivering an NMI (which we don't support yet).

When running native, iret has the behaviour of testing for a pending interrupt atomically with re-enabling interrupts. Unfortunately there's no way to do this with Xen, so there's a window in which we could get a recursive exception after enabling events but before actually returning to userspace.

This causes three problems:

- If the interrupt causes a softirq to be queued, we will return to userspace without processing it, since its already after the point at which we look for queued softirqs. This means it could be an unbounded amount of time before it gets processed on next kernel entry.

- If the interrupt causes a signal to be delivered to the current process, the signal will be marked pending on the process, but it will not get delivered because we're past the point where pending signals are detected. Again, it could be an unbounded amount of time before the signal gets delivered.

- The recursion is, in theory, unbounded. There's a small chance that a series of unfortunate events will cause the exception frames to build up and overrun the stack. But that's very unlikely.

To deal with this, the xen event upcall handler checks to see if the EIP is within the critical section of the iret code, after events are (potentially) enabled up to the iret itself. If its within this range, it calls the iret critical section fixup, which adjusts the stack to deal with any unrestored registers, and then shifts the stack frame up to replace the previous invocation.

---
 arch/i386/kernel/asm-offsets.c |    1 
 arch/i386/kernel/entry.S       |   16 +++
 arch/i386/xen/enlighten.c      |    1 
 arch/i386/xen/xen-asm.S        |  167 +++++++++++++++++++++++++++++++++++++++-
 arch/i386/xen/xen-ops.h        |    1 
 5 files changed, 181 insertions(+), 5 deletions(-)

===================================================================

@@ -65,6 +65,7 @@ void foo(void)
 	OFFSET(TI_addr_limit, thread_info, addr_limit);
 	OFFSET(TI_restart_block, thread_info, restart_block);
 	OFFSET(TI_sysenter_return, thread_info, sysenter_return);
+	OFFSET(TI_cpu, thread_info, cpu);
 	BLANK();
 
 	OFFSET(GDS_size, Xgt_desc_struct, size);

===================================================================

@@ -1030,7 +1030,21 @@ ENTRY(xen_hypervisor_callback)
 	CFI_ADJUST_CFA_OFFSET 4
 	SAVE_ALL
 	TRACE_IRQS_OFF
-	mov %esp, %eax
+
+	/* Check to see if we got the event in the critical
+	   region in xen_iret_direct, after we've reenabled
+	   events and checked for pending events.  This simulates
+	   iret instruction's behaviour where it delivers a
+	   pending interrupt when enabling interrupts. */
+	movl PT_EIP(%esp),%eax
+	cmpl $xen_iret_start_crit,%eax
+	jb   1f
+	cmpl $xen_iret_end_crit,%eax
+	jae  1f
+
+	call xen_iret_crit_fixup
+
+1:	mov %esp, %eax
 	call xen_evtchn_do_upcall
 	jmp  ret_from_intr
 	CFI_ENDPROC

===================================================================

@@ -827,6 +827,7 @@ void __init xen_setup_vcpu_info_placemen
 		paravirt_ops.irq_disable = xen_irq_disable_direct;
 		paravirt_ops.irq_enable = xen_irq_enable_direct;
 		paravirt_ops.read_cr2 = xen_read_cr2_direct;
+		paravirt_ops.iret = xen_iret_direct;
 	}
 }
 

===================================================================

===================================================================

@@ -68,4 +68,5 @@ DECL_ASM(unsigned long, xen_save_fl_dire
 DECL_ASM(unsigned long, xen_save_fl_direct, void);
 DECL_ASM(void, xen_restore_fl_direct, unsigned long);
 
+void xen_iret_direct(void);
 #endif /* XEN_OPS_H */

Not sure what a recursive exception is. You mean the interrupt?

It looks ...very... ug^w^wcomplicated.

That doesn't make sense. softirqs get processed after interrupts, not on return to user space. So the nested interrupt should handle its own softirqs because the softirq counters are already decreased.

It's still not clear to me why you can't do cli ; check again ; iret-equivalent to handle this.

Doesn't seem to be different to me than a normal interrupt anywhere else. If you're worried about overflow use interrupt stacks.

-Andi

Yeah, though Xen calls them events, so I chose a completely different third term.

I suppose, but you should look at the xen-unstable code by comparison...

But either way, the alternative is to always use the iret hypercall, which effectively doubles the cost of entering and exiting the kernel, so I think the extra code is worth it.

Hm, yes, I guess so. I'd assumed that softirq was in the WORK_NEEDED path of entry.S without checking; but anything which can set one of the WORK_NEEDED flags is an issue.

Well, we use the real iret instruction to actually transition into userspace; obviously we can't do anything after that, and there's always going to be an open window before it because we can't do anything instruction-level atomic.

In your sequence, the event may become pending after "check again", even though it won't be delivered. And either way, we need to set the proper event mask state before using "iret", so there's still a race where a recursive interrupt could happen.

In other words, consider this hypothetical sequence:

1. start with events masked
2. check for pending -> handle if so
3. unmask events
4. check for pending -> handle if so
5. iret

There's always a window after "check for pending" where a new event can become pending. If it happens between 2 & 3, then we miss it at 2; if 4 isn't done, we end up returning to userspace with a pending event. That is, 2 is insufficient on its own, and redundant when 4 is in place, so there's no point in testing for pending while events are still masked.

If we do the check at 4, then there's a window at 4-5 where we can get a
recursive event/interrupt. If that happens, then the nested interrupt
save nested register state on the stack. At it leaves the nested interrupt it will notice that its returning to kernel mode, and won't bother testing any of the WORK_NEEDED flags, and it will end up returning to just before the iret in step 5. This will simply return to usermode directly without re-testing the WORK_NEEDED flags, leaving them set until the process enters and leaves the kernel again, possibly an unbounded amount of time later.

(Despite the recursion window, the test at 4 is always necessary to mop up any previously pending events from before step 1.)

This patch short-circuits this by noticing that there's a recursive event in the 4-5 window, and just folds the recursive frame into the current one and jumps back into interrupt handling, meaning that all the work flags will be re-processed as it comes out again.

No, I don't think this is really a concern at all. But it was the only rationale listed by the original Xen implementation. I wasn't going to bother with the critical region stuff at all, but I think the signal/WORK_NEEDED issue is worth plugging. Besides, if we were in that situation, this patch just converts the stack overrun into a livelock, which isn't much of an improvement.

J

For interrupts it can be only signals or rescheduling.

If you stay cli you don't need that. Why is it that it has to enable interrupts?

sti only takes affect one instruction after it. So if you can make it that small it would also work. But that might not help you.

-Andi

Bear in mind we're talking about running paravirt under Xen in ring 1.

The cli/sti instructions don't control the event mask, so they're effectively expensive no-ops (they trap into the hypervisor, are emulated as no-ops). But if you mean cli as a general term for "events/interrupts masked", then they can't remain masked when you
return to usermode. iret normally sets the current eflags IF state
from the on-stack IF, but that's irrelevent to Xen; we need to extract eflags.IF from the on-stack eflags, and put that into the vcpu's event mask. That's inherently non-atomic with respect to iret.

No, it doesn't. sti can't be run directly in ring1, and trapping into Xen is exactly what this patch is trying to avoid. Even if it did, Xen treats it as a no-op, and it has no effect on the event mask state. Even if Xen did use it to control the event mask state, it would lose the 1-instruction shadow because it trapped into the hypervisor, and a new event may be pending as it comes out.

J

Ah I assumed the hypervisor would just check IF in ring 1 too. It would certainly make this easier, but then the additional trap of setting it would be also somewhat expensive agreed.

I must say I still hate the patch; it has all the signs of something that will be very nasty to maintain later.

-Andi

Xen doesn't do that because, while it could track sti/cli (expensively), iret and popf quietly ignore the IF state in ring 1, and so there's lots of scope for interrupt state getting lost.

Well, the corresponding xen-unstable code has been a bit of a trial to maintain. I made this as simple and self-contained as possible (with very little non-locality) to try and keep it maintainable.

I agree its all a bit subtle, but in its favour:

1. It's internal to the implementation of the iret pvop, which does have a fairly well-defined and stable interface (same as iret instruction, essentially)
2. Comments!
3. Relatively simple implementation (only one register to deal with in the slow-path handler, for example)

The annoying non-local thing is the test in the xen upcall handler, but that's unavoidable.

J