Re: [PATCH 18/25] [PATCH] turn priviled operations into macros in entry.S

Hi Andi,

Thanks for all the comments, it's greatly appreciated.

On Wed, 8 Aug 2007, Andi Kleen wrote:

quoted

+#define SYSRETQ						\
+			movq	%gs:pda_oldrsp,%rsp;	\
+			swapgs;				\
+			sysretq;

When the macro does more than sysret it should have a different
name

Noted. Do you have a better idea? Something like SETSTACK_SWAPGS_SYSRETQ?

quoted

  */
 	.globl int_ret_from_sys_call
 int_ret_from_sys_call:
-	cli
+	DISABLE_INTERRUPTS(CLBR_ANY)

ANY? There are certainly some registers alive at this point like rax

Glauber will need to address this, this is his code ;-)

quoted

 retint_restore_args:
-	cli
+	DISABLE_INTERRUPTS(CLBR_ANY)

Similar.

quoted

 	/*
 	 * The iretq could re-enable interrupts:
 	 */

@@ -566,10 +587,14 @@ retint_restore_args:
 restore_args:
 	RESTORE_ARGS 0,8,0
 iret_label:
-	iretq
+#ifdef CONFIG_PARAVIRT
+	INTERRUPT_RETURN
+ENTRY(native_iret)

ENTRY adds alignment. Why do you need that export anyways?

The paravirt ops struct points to it.

quoted

+#endif
+1:	iretq

 	.section __ex_table,"a"
-	.quad iret_label,bad_iret
+	.quad 1b, bad_iret

iret_label seems more expressive to me than 1

The reason for this change is because of the added:
#ifdef CONFIG_PARAVIRT
	INTERRUPT_RETURN
ENTRY(native_iret)
#endif

If we are paravirt ops, we need the iretq in the exception table, not the
paravit ops function call.  Since that function call may simply call the
native_iretq, and if we take a fault at the iretq, it wont be in the
exception table.

quoted

+	ENABLE_INTERRUPTS(CLBR_NONE)

In many of the CLBR_NONEs there are actually some registers free;
but it might be safer to keep it this way. But if some client can get
significantly better code with one or two free registers it might
be worthwhile to investigate.

Glauber, that comment is for you.

quoted

-	swapgs
+	SWAPGS_NOSTACK

There's still stack here

OK, bad name then. How about SWAPGS_UNTRUSTED_STACK?

From earlier in the file where SWAPGS_NOSTACK is declared we have:


/* Currently paravirt can't handle swapgs nicely when we
 * don't have a stack.  So we either find a way around these
 * or just fault and emulate if a guest tries to call swapgs
 * directly.
 *
 * Either way, this is a good way to document that we don't
 * have a reliable stack.
 */
#define SWAPGS_NOSTACK		swapgs

quoted

 paranoid_restore\trace:
 	RESTORE_ALL 8
-	iretq
+	INTERRUPT_RETURN

I suspect Xen will need much more changes anyways because of its
ring 3 guest. Are these changes sufficient for lguest?

Probably not, but this part of the code I don't fully understand. But just
doing this doesn't break lguest.  But perhaps it only did not break it yet
;-)


Thanks,

-- Steve