[PATCH] xen: use iret directly where possible

[PATCH] xen: use iret directly where possible

[Jeremy Fitzhardinge]

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.

Signed-off-by: Jeremy Fitzhardinge <jeremy@xensource.com>

---
 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(-)

===================================================================
--- a/arch/i386/kernel/asm-offsets.c
+++ b/arch/i386/kernel/asm-offsets.c
@@ -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);
===================================================================
--- a/arch/i386/kernel/entry.S
+++ b/arch/i386/kernel/entry.S
@@ -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
===================================================================
--- a/arch/i386/xen/enlighten.c
+++ b/arch/i386/xen/enlighten.c
@@ -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;
 	}
 }
 
===================================================================
--- a/arch/i386/xen/xen-asm.S
+++ b/arch/i386/xen/xen-asm.S
@@ -12,14 +12,20 @@
  */
 
 #include <linux/linkage.h>
+
 #include <asm/asm-offsets.h>
 #include <asm/thread_info.h>
 #include <asm/percpu.h>
-#include <asm/asm-offsets.h>
 #include <asm/processor-flags.h>
+#include <asm/segment.h>
+
+#include <xen/interface/xen.h>
 
 #define RELOC(x, v)	.globl x##_reloc; x##_reloc=v
 #define ENDPATCH(x)	.globl x##_end; x##_end=.
+
+/* Pseudo-flag used for virtual NMI, which we don't implement yet */
+#define XEN_EFLAGS_NMI	0x80000000
 
 /*
 	Enable events.  This clears the event mask and tests the pending
@@ -81,13 +87,12 @@ ENDPATCH(xen_save_fl_direct)
  */
 ENTRY(xen_restore_fl_direct)
 	testb $X86_EFLAGS_IF>>8, %ah
-	setz %al
-	movb %al, PER_CPU_VAR(xen_vcpu_info)+XEN_vcpu_info_mask
+	setz PER_CPU_VAR(xen_vcpu_info)+XEN_vcpu_info_mask
 	/* Preempt here doesn't matter because that will deal with
 	   any pending interrupts.  The pending check may end up being
 	   run on the wrong CPU, but that doesn't hurt. */
 
-	/* check for pending but unmasked */
+	/* check for unmasked and pending */
 	cmpw $0x0001, PER_CPU_VAR(xen_vcpu_info)+XEN_vcpu_info_pending
 	jz 1f
 2:	call check_events
@@ -97,6 +102,160 @@ ENDPATCH(xen_restore_fl_direct)
 	ENDPROC(xen_restore_fl_direct)
 	RELOC(xen_restore_fl_direct, 2b+1)
 
+/*
+	This is run where a normal iret would be run, with the same stack setup:
+	      8: eflags
+	      4: cs
+	esp-> 0: eip
+
+	This attempts to make sure that any pending events are dealt with
+	on return to usermode, but there is a small window in which an event
+	can happen just before entering usermode.  This has three effects:
+	 - There can be interrupt recursion on the stack, which is
+	   unbounded in theory (but very unlikely in practice)
+	 - New softirq events can be queued up, but they won't get
+	   processed until the cpu next enters and leaves the kernel.
+	 - Signals likewise.
+
+	Non-direct iret could be done in the same way, but it would
+	require an annoying amount of code duplication.  We'll assume
+	that direct mode will be the common case once the hypervisor
+	support becomes commonplace.
+ */
+ENTRY(xen_iret_direct)
+	/* test eflags for special cases */
+	testl $(X86_EFLAGS_VM | XEN_EFLAGS_NMI), 8(%esp)
+	jnz hyper_iret
+
+	/* check IF state we're restoring */
+	testb $X86_EFLAGS_IF>>8, 8+1(%esp)
+
+	push %eax
+	ESP_OFFSET=4	# bytes pushed onto stack
+
+	/* Store vcpu_info pointer for easy access.  Do it this
+	   way to avoid having to reload %fs */
+#ifdef CONFIG_SMP
+	GET_THREAD_INFO(%eax)
+	movl TI_cpu(%eax),%eax
+	movl __per_cpu_offset(,%eax,4),%eax
+	lea per_cpu__xen_vcpu_info(%eax),%eax
+#else
+	movl $per_cpu__xen_vcpu_info, %eax
+#endif
+
+	/* Maybe enable events.  Once this happens we could get a
+	   recursive event, so the critical region starts immediately
+	   afterwards.  However, if that happens we don't end up
+	   resuming the code, so we don't have to be worried about
+	   being preempted to another CPU. */
+	setz XEN_vcpu_info_mask(%eax)
+xen_iret_start_crit:
+
+	/* check for unmasked and pending */
+	cmpw $0x0001, XEN_vcpu_info_pending(%eax)
+
+	/* If there's something pending, mask events again so we
+	   can jump back into xen_hypervisor_callback */
+	sete XEN_vcpu_info_mask(%eax)
+
+	popl %eax
+
+	/* From this point on the registers are restored and the stack
+	   updated, so we don't need to worry about it if we're preempted */
+iret_restore_end:
+
+	/* Jump to hypervisor_callback after fixing up the stack.
+	   Events are masked, so jumping out of the critical
+	   region is OK. */
+	je xen_hypervisor_callback
+
+	iret
+xen_iret_end_crit:
+
+hyper_iret:
+	/* put this out of line since its very rarely used */
+	jmp hypercall_page + __HYPERVISOR_iret * 32
+
+	.globl xen_iret_start_crit, xen_iret_end_crit
+
+/*
+   This is called by xen_hypervisor_callback in entry.S when it sees
+   that the EIP at the time of interrupt was between xen_iret_start_crit
+   and xen_iret_end_crit.  We're passed the EIP in %eax so we can do
+   a more refined determination of what to do.
+
+   The stack format at this point is:
+	----------------
+	 ss		:
+	 esp		:
+	 eflags		}  outer exception info
+	 cs		}
+	 eip		}
+	---------------- <- edi (copy dest)
+	 eax		:  outer eax if it hasn't been restored
+	----------------
+	 eflags		}  nested exception info
+	 cs		}   (no ss/esp because we're nested
+	 eip		}    from the same ring)
+	 orig_eax	}<- esi (copy src)
+	 - - - - - - - -
+	 fs		}
+	 es		}
+	 ds		}  SAVE_ALL state
+	 eax		}
+	  :		:
+	 ebx		}
+	----------------
+	 return addr	 <- esp
+	----------------
+
+   In order to deliver the nested exception properly, we need to shift
+   everything from the return addr up to the error code so it
+   sits just under the outer exception info.  This means that when we
+   handle the exception, we do it in the context of the outer exception
+   rather than starting a new one.
+
+   The only caveat is that if the outer eax hasn't been
+   restored yet (ie, it's still on stack), we need to insert
+   its value into the SAVE_ALL state before going on, since
+   its usermode state which we eventually need to restore.
+ */
+ENTRY(xen_iret_crit_fixup)
+	/* offsets +4 for return address */
+
+	/* Paranoia: make sure we're really coming from userspace.
+	   Once could imagine a case where userspace jumps into
+	   the critical range address, but just before the CPU
+	   delivers a GP, it decides to deliver an interrupt
+	   instead.  Unlikely?  Definitely.  Easy to avoid?
+	   Yes. (Some virtual environments get this wrong.) */
+	movl PT_CS+4(%esp), %ecx
+	andl $SEGMENT_RPL_MASK, %ecx
+	cmpl $USER_RPL, %ecx
+	je 2f
+
+	lea PT_ORIG_EAX+4(%esp), %esi
+	lea PT_EFLAGS+4(%esp), %edi
+
+	/* If eip is before iret_restore_end then stack
+	   hasn't been restored yet. */
+	cmp $iret_restore_end, %eax
+	jae 1f
+
+	movl 0+4(%edi),%eax		/* copy EAX */
+	movl %eax, PT_EAX+4(%esp)
+
+	lea ESP_OFFSET(%edi),%edi	/* move dest up over saved regs */
+
+	/* set up the copy */
+1:	std
+	mov $(PT_EIP+4) / 4, %ecx	/* copy ret+saved regs up to orig_eax */
+	rep movsl
+	cld
+
+	lea 4(%edi),%esp		/* point esp to new frame */
+2:	ret
 
 
 /*
===================================================================
--- a/arch/i386/xen/xen-ops.h
+++ b/arch/i386/xen/xen-ops.h
@@ -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 */

Re: [PATCH] xen: use iret directly where possible

[Andi Kleen]

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

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

>  - 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.

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.

>  - 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.

It's still not clear to me why you can't do cli ; check again ; iret-equivalent
to handle this.
 
>  - 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.

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

-Andi

Re: Re: [PATCH] xen: use iret directly where possible

[Jeremy Fitzhardinge]

Andi Kleen wrote:
> Not sure what a recursive exception is. You mean the interrupt?
>   

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

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

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.

>>  - 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.
>>     
>
> 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.
>   

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.

>>  - 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.
>>     
>
> It's still not clear to me why you can't do cli ; check again ; iret-equivalent
> to handle this.
>   

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.

>>  - 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.
>>     
>
> Doesn't seem to be different to me than a normal interrupt anywhere
> else. If you're worried about overflow use interrupt stacks.
>   

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

Re: Re: [PATCH] xen: use iret directly where possible

[Andi Kleen]

On Monday 04 June 2007 22:33, Jeremy Fitzhardinge wrote:

> 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.

For interrupts it can be only signals or rescheduling.

> >>  - 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.
> >
> > It's still not clear to me why you can't do cli ; check again ;
> > iret-equivalent to handle this.
>
> 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.

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

> In your sequence, the event may become pending after "check again", even
> though it won't be delivered. 

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

Re: Re: [PATCH] xen: use iret directly where possible

[Jeremy Fitzhardinge]

Andi Kleen wrote:
> If you stay cli you don't need that. Why is it that it has to enable 
> interrupts?
>   

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.

>> In your sequence, the event may become pending after "check again", even
>> though it won't be delivered. 
>>     
>
> 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.

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

Re: [Xen-devel] Re: [PATCH] xen: use iret directly where possible

[Andi Kleen]

On Monday 04 June 2007 23:28, Jeremy Fitzhardinge wrote:

> 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.

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

Re: [Xen-devel] Re: [PATCH] xen: use iret directly where possible

[Jeremy Fitzhardinge]

Andi Kleen wrote:
> 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.
>   

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.

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

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