[PATCH] xen: Disable stack protector for irq helper

[PATCH] xen: Disable stack protector for irq helper

[Bastian Blank]

The stack protector needs additional registers on x86_32, which are not
saved in calls to the small paravirt interrupt handlers. This leads to
early crashes as registers are overwritten and not saved by the caller
as instructed.

Signed-off-by: Bastian Blank <waldi@debian.org>

diff --git a/arch/x86/xen/Makefile b/arch/x86/xen/Makefile
index 3bb4fc2..ac19398 100644
--- a/arch/x86/xen/Makefile
+++ b/arch/x86/xen/Makefile
@@ -5,9 +5,9 @@
 CFLAGS_REMOVE_irq.o = -pg
 endif
 
-# Make sure early boot has no stackprotector
 nostackp := $(call cc-option, -fno-stack-protector)
 CFLAGS_enlighten.o		:= $(nostackp)
+CFLAGS_irq.o			:= $(nostackp)
 CFLAGS_mmu.o			:= $(nostackp)
 
 obj-y		:= enlighten.o setup.o multicalls.o mmu.o irq.o \
-- 
Mind your own business, Spock.  I'm sick of your halfbreed interference.

[PATCH] xen: Disable stack protector for irq helper

[Bastian Blank]

The stack protector needs additional registers on x86_32, which are not
saved in calls to the small paravirt interrupt handlers. This leads to
early crashes as registers are overwritten and not saved by the caller
as instructed.

Signed-off-by: Bastian Blank <waldi@debian.org>

diff --git a/arch/x86/xen/Makefile b/arch/x86/xen/Makefile
index 3bb4fc2..ac19398 100644
--- a/arch/x86/xen/Makefile
+++ b/arch/x86/xen/Makefile
@@ -5,9 +5,9 @@
 CFLAGS_REMOVE_irq.o = -pg
 endif
 
-# Make sure early boot has no stackprotector
 nostackp := $(call cc-option, -fno-stack-protector)
 CFLAGS_enlighten.o		:= $(nostackp)
+CFLAGS_irq.o			:= $(nostackp)
 CFLAGS_mmu.o			:= $(nostackp)
 
 obj-y		:= enlighten.o setup.o multicalls.o mmu.o irq.o \
-- 
Mind your own business, Spock.  I'm sick of your halfbreed interference.

Re: [PATCH] xen: Disable stack protector for irq helper

[Jeremy Fitzhardinge]

On 10/04/09 11:30, Bastian Blank wrote:
> The stack protector needs additional registers on x86_32, which are not
> saved in calls to the small paravirt interrupt handlers. This leads to
> early crashes as registers are overwritten and not saved by the caller
> as instructed.
>   

Thanks for the patch, but I don't think its quite right.  
PV_CALLEE_SAVE_REGS_THUNK() is responsible for generating a wrapper for
the functions to save/restore all the appropriate registers.  If it is
failing to do so, then the correct fix is to update
PV_SAVE/RESTORE_ALL_CALLER_REGS.

Or have I misunderstood you analysis?

Thanks,
    J

> Signed-off-by: Bastian Blank <waldi@debian.org>
>
> diff --git a/arch/x86/xen/Makefile b/arch/x86/xen/Makefile
> index 3bb4fc2..ac19398 100644
> --- a/arch/x86/xen/Makefile
> +++ b/arch/x86/xen/Makefile
> @@ -5,9 +5,9 @@
>  CFLAGS_REMOVE_irq.o = -pg
>  endif
>  
> -# Make sure early boot has no stackprotector
>  nostackp := $(call cc-option, -fno-stack-protector)
>  CFLAGS_enlighten.o		:= $(nostackp)
> +CFLAGS_irq.o			:= $(nostackp)
>  CFLAGS_mmu.o			:= $(nostackp)
>  
>  obj-y		:= enlighten.o setup.o multicalls.o mmu.o irq.o \
>   

Re: [PATCH] xen: Disable stack protector for irq helper

[Jeremy Fitzhardinge]

On 10/04/09 11:30, Bastian Blank wrote:
> The stack protector needs additional registers on x86_32, which are not
> saved in calls to the small paravirt interrupt handlers. This leads to
> early crashes as registers are overwritten and not saved by the caller
> as instructed.
>   

Thanks for the patch, but I don't think its quite right.  
PV_CALLEE_SAVE_REGS_THUNK() is responsible for generating a wrapper for
the functions to save/restore all the appropriate registers.  If it is
failing to do so, then the correct fix is to update
PV_SAVE/RESTORE_ALL_CALLER_REGS.

Or have I misunderstood you analysis?

Thanks,
    J

> Signed-off-by: Bastian Blank <waldi@debian.org>
>
> diff --git a/arch/x86/xen/Makefile b/arch/x86/xen/Makefile
> index 3bb4fc2..ac19398 100644
> --- a/arch/x86/xen/Makefile
> +++ b/arch/x86/xen/Makefile
> @@ -5,9 +5,9 @@
>  CFLAGS_REMOVE_irq.o = -pg
>  endif
>  
> -# Make sure early boot has no stackprotector
>  nostackp := $(call cc-option, -fno-stack-protector)
>  CFLAGS_enlighten.o		:= $(nostackp)
> +CFLAGS_irq.o			:= $(nostackp)
>  CFLAGS_mmu.o			:= $(nostackp)
>  
>  obj-y		:= enlighten.o setup.o multicalls.o mmu.o irq.o \
>   

Re: [PATCH] xen: Disable stack protector for irq helper

[Bastian Blank]

On Sun, Oct 04, 2009 at 04:06:13PM -0700, Jeremy Fitzhardinge wrote:
> On 10/04/09 11:30, Bastian Blank wrote:
> > The stack protector needs additional registers on x86_32, which are not
> > saved in calls to the small paravirt interrupt handlers. This leads to
> > early crashes as registers are overwritten and not saved by the caller
> > as instructed.
> Thanks for the patch, but I don't think its quite right.  
> PV_CALLEE_SAVE_REGS_THUNK() is responsible for generating a wrapper for
> the functions to save/restore all the appropriate registers.  If it is
> failing to do so, then the correct fix is to update
> PV_SAVE/RESTORE_ALL_CALLER_REGS.

Well, I did not understand this part of the code, but you seem right. So
lets try the following. I have not yet run tested it.

Save all caller-saved registers on x86_32 for the paravirt callee saved
registers.

Signed-off-by: Bastian Blank <waldi@debian.org>

diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
index e19ffe3..e4272f3 100644
--- a/arch/x86/include/asm/paravirt.h
+++ b/arch/x86/include/asm/paravirt.h
@@ -793,8 +793,8 @@ static __always_inline void __raw_spin_unlock(struct raw_spinlock *lock)
 #define PV_RESTORE_REGS "popl %edx; popl %ecx;"
 
 /* save and restore all caller-save registers, except return value */
-#define PV_SAVE_ALL_CALLER_REGS		"pushl %ecx;"
-#define PV_RESTORE_ALL_CALLER_REGS	"popl  %ecx;"
+#define PV_SAVE_ALL_CALLER_REGS PV_SAVE_REGS
+#define PV_RESTORE_ALL_CALLER_REGS PV_RESTORE_REGS
 
 #define PV_FLAGS_ARG "0"
 #define PV_EXTRA_CLOBBERS
-- 
In the strict scientific sense we all feed on death -- even vegetarians.
		-- Spock, "Wolf in the Fold", stardate 3615.4

Re: [PATCH] xen: Disable stack protector for irq helper

[Bastian Blank]

On Sun, Oct 04, 2009 at 04:06:13PM -0700, Jeremy Fitzhardinge wrote:
> On 10/04/09 11:30, Bastian Blank wrote:
> > The stack protector needs additional registers on x86_32, which are not
> > saved in calls to the small paravirt interrupt handlers. This leads to
> > early crashes as registers are overwritten and not saved by the caller
> > as instructed.
> Thanks for the patch, but I don't think its quite right.  
> PV_CALLEE_SAVE_REGS_THUNK() is responsible for generating a wrapper for
> the functions to save/restore all the appropriate registers.  If it is
> failing to do so, then the correct fix is to update
> PV_SAVE/RESTORE_ALL_CALLER_REGS.

Well, I did not understand this part of the code, but you seem right. So
lets try the following. I have not yet run tested it.

Save all caller-saved registers on x86_32 for the paravirt callee saved
registers.

Signed-off-by: Bastian Blank <waldi@debian.org>

diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
index e19ffe3..e4272f3 100644
--- a/arch/x86/include/asm/paravirt.h
+++ b/arch/x86/include/asm/paravirt.h
@@ -793,8 +793,8 @@ static __always_inline void __raw_spin_unlock(struct raw_spinlock *lock)
 #define PV_RESTORE_REGS "popl %edx; popl %ecx;"
 
 /* save and restore all caller-save registers, except return value */
-#define PV_SAVE_ALL_CALLER_REGS		"pushl %ecx;"
-#define PV_RESTORE_ALL_CALLER_REGS	"popl  %ecx;"
+#define PV_SAVE_ALL_CALLER_REGS PV_SAVE_REGS
+#define PV_RESTORE_ALL_CALLER_REGS PV_RESTORE_REGS
 
 #define PV_FLAGS_ARG "0"
 #define PV_EXTRA_CLOBBERS
-- 
In the strict scientific sense we all feed on death -- even vegetarians.
		-- Spock, "Wolf in the Fold", stardate 3615.4

[PATCH] xen: fbdev frontend needs xenbus frontend

[Bastian Blank]

Make the fbdev frontend select the xenbus frontend.

Signed-off-by: Bastian Blank <waldi@debian.org>

diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 3b54b39..1b332d1 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -2070,6 +2070,7 @@ config XEN_FBDEV_FRONTEND
 	select FB_SYS_IMAGEBLIT
 	select FB_SYS_FOPS
 	select FB_DEFERRED_IO
+	select XEN_XENBUS_FRONTEND
 	default y
 	help
 	  This driver implements the front-end of the Xen virtual

Re: [PATCH] xen: Disable stack protector for irq helper

[Jeremy Fitzhardinge]

On 10/04/09 18:35, Bastian Blank wrote:
> On Sun, Oct 04, 2009 at 04:06:13PM -0700, Jeremy Fitzhardinge wrote:
>   
>> On 10/04/09 11:30, Bastian Blank wrote:
>>     
>>> The stack protector needs additional registers on x86_32, which are not
>>> saved in calls to the small paravirt interrupt handlers. This leads to
>>> early crashes as registers are overwritten and not saved by the caller
>>> as instructed.
>>>       
>> Thanks for the patch, but I don't think its quite right.  
>> PV_CALLEE_SAVE_REGS_THUNK() is responsible for generating a wrapper for
>> the functions to save/restore all the appropriate registers.  If it is
>> failing to do so, then the correct fix is to update
>> PV_SAVE/RESTORE_ALL_CALLER_REGS.
>>     
> Well, I did not understand this part of the code, but you seem right. So
> lets try the following. I have not yet run tested it.
>
> Save all caller-saved registers on x86_32 for the paravirt callee saved
> registers.
>   

That looks better, but it is still overkill.  We only need to save the
set of registers the ABI requires the callee to preserve.  What
additional register(s) gets clobbered by stack-protector that need to be
saved?

    J

Re: [PATCH] xen: Disable stack protector for irq helper

[Jeremy Fitzhardinge]

On 10/04/09 18:35, Bastian Blank wrote:
> On Sun, Oct 04, 2009 at 04:06:13PM -0700, Jeremy Fitzhardinge wrote:
>   
>> On 10/04/09 11:30, Bastian Blank wrote:
>>     
>>> The stack protector needs additional registers on x86_32, which are not
>>> saved in calls to the small paravirt interrupt handlers. This leads to
>>> early crashes as registers are overwritten and not saved by the caller
>>> as instructed.
>>>       
>> Thanks for the patch, but I don't think its quite right.  
>> PV_CALLEE_SAVE_REGS_THUNK() is responsible for generating a wrapper for
>> the functions to save/restore all the appropriate registers.  If it is
>> failing to do so, then the correct fix is to update
>> PV_SAVE/RESTORE_ALL_CALLER_REGS.
>>     
> Well, I did not understand this part of the code, but you seem right. So
> lets try the following. I have not yet run tested it.
>
> Save all caller-saved registers on x86_32 for the paravirt callee saved
> registers.
>   

That looks better, but it is still overkill.  We only need to save the
set of registers the ABI requires the callee to preserve.  What
additional register(s) gets clobbered by stack-protector that need to be
saved?

    J

Re: [Xen-devel] Re: [PATCH] xen: Disable stack protector for irq helper

[Bastian Blank]

On Mon, Oct 05, 2009 at 10:21:01AM -0700, Jeremy Fitzhardinge wrote:
> > Save all caller-saved registers on x86_32 for the paravirt callee saved
> > registers.
> That looks better, but it is still overkill.  We only need to save the
> set of registers the ABI requires the callee to preserve.  What
> additional register(s) gets clobbered by stack-protector that need to be
> saved?

Well, exactly the two, ecx and edx. eax is still clobbered by the return
value. Anyway, it works in praxis.

Bastian

-- 
Superior ability breeds superior ambition.
		-- Spock, "Space Seed", stardate 3141.9

Re: Re: [PATCH] xen: Disable stack protector for irq helper

[Bastian Blank]

On Mon, Oct 05, 2009 at 10:21:01AM -0700, Jeremy Fitzhardinge wrote:
> > Save all caller-saved registers on x86_32 for the paravirt callee saved
> > registers.
> That looks better, but it is still overkill.  We only need to save the
> set of registers the ABI requires the callee to preserve.  What
> additional register(s) gets clobbered by stack-protector that need to be
> saved?

Well, exactly the two, ecx and edx. eax is still clobbered by the return
value. Anyway, it works in praxis.

Bastian

-- 
Superior ability breeds superior ambition.
		-- Spock, "Space Seed", stardate 3141.9

Re: [Xen-devel] Re: [PATCH] xen: Disable stack protector for irq helper

[Jeremy Fitzhardinge]

On 10/05/09 15:43, Bastian Blank wrote:
> On Mon, Oct 05, 2009 at 10:21:01AM -0700, Jeremy Fitzhardinge wrote:
>   
>>> Save all caller-saved registers on x86_32 for the paravirt callee saved
>>> registers.
>>>       
>> That looks better, but it is still overkill.  We only need to save the
>> set of registers the ABI requires the callee to preserve.  What
>> additional register(s) gets clobbered by stack-protector that need to be
>> saved?
>>     
> Well, exactly the two, ecx and edx. eax is still clobbered by the return
> value. Anyway, it works in praxis.

I'm confused.  It already saves ecx, so what else needs saving?

Besides, most of the code in that file isn't used unless you're using a
very old version of Xen; it will generally prefer to use the ones in
xen-asm_X.S.

I have the feeling we haven't really found the root cause of your
problem yet.

Thanks,
    J

Re: Re: [PATCH] xen: Disable stack protector for irq helper

[Jeremy Fitzhardinge]

On 10/05/09 15:43, Bastian Blank wrote:
> On Mon, Oct 05, 2009 at 10:21:01AM -0700, Jeremy Fitzhardinge wrote:
>   
>>> Save all caller-saved registers on x86_32 for the paravirt callee saved
>>> registers.
>>>       
>> That looks better, but it is still overkill.  We only need to save the
>> set of registers the ABI requires the callee to preserve.  What
>> additional register(s) gets clobbered by stack-protector that need to be
>> saved?
>>     
> Well, exactly the two, ecx and edx. eax is still clobbered by the return
> value. Anyway, it works in praxis.

I'm confused.  It already saves ecx, so what else needs saving?

Besides, most of the code in that file isn't used unless you're using a
very old version of Xen; it will generally prefer to use the ones in
xen-asm_X.S.

I have the feeling we haven't really found the root cause of your
problem yet.

Thanks,
    J

Re: [Xen-devel] Re: [PATCH] xen: Disable stack protector for irq helper

[Bastian Blank]

On Mon, Oct 05, 2009 at 05:36:02PM -0700, Jeremy Fitzhardinge wrote:
> On 10/05/09 15:43, Bastian Blank wrote:
> > On Mon, Oct 05, 2009 at 10:21:01AM -0700, Jeremy Fitzhardinge wrote:
> >>> Save all caller-saved registers on x86_32 for the paravirt callee saved
> >>> registers.
> >> That looks better, but it is still overkill.  We only need to save the
> >> set of registers the ABI requires the callee to preserve.  What
> >> additional register(s) gets clobbered by stack-protector that need to be
> >> saved?
> > Well, exactly the two, ecx and edx. eax is still clobbered by the return
> > value. Anyway, it works in praxis.
> I'm confused.  It already saves ecx, so what else needs saving?

The original version saves ecx, but not edx. Both are official
caller-saved registers.

> Besides, most of the code in that file isn't used unless you're using a
> very old version of Xen; it will generally prefer to use the ones in
> xen-asm_X.S.

Well, my call stack say something different. It crashs during early
startup without a console. The modifications to the function pointers is
done much later.

Bastian

-- 
Vulcans never bluff.
		-- Spock, "The Doomsday Machine", stardate 4202.1

Re: Re: [PATCH] xen: Disable stack protector for irq helper

[Bastian Blank]

On Mon, Oct 05, 2009 at 05:36:02PM -0700, Jeremy Fitzhardinge wrote:
> On 10/05/09 15:43, Bastian Blank wrote:
> > On Mon, Oct 05, 2009 at 10:21:01AM -0700, Jeremy Fitzhardinge wrote:
> >>> Save all caller-saved registers on x86_32 for the paravirt callee saved
> >>> registers.
> >> That looks better, but it is still overkill.  We only need to save the
> >> set of registers the ABI requires the callee to preserve.  What
> >> additional register(s) gets clobbered by stack-protector that need to be
> >> saved?
> > Well, exactly the two, ecx and edx. eax is still clobbered by the return
> > value. Anyway, it works in praxis.
> I'm confused.  It already saves ecx, so what else needs saving?

The original version saves ecx, but not edx. Both are official
caller-saved registers.

> Besides, most of the code in that file isn't used unless you're using a
> very old version of Xen; it will generally prefer to use the ones in
> xen-asm_X.S.

Well, my call stack say something different. It crashs during early
startup without a console. The modifications to the function pointers is
done much later.

Bastian

-- 
Vulcans never bluff.
		-- Spock, "The Doomsday Machine", stardate 4202.1

Re: [Xen-devel] Re: [PATCH] xen: Disable stack protector for irq helper

[Jeremy Fitzhardinge]

On 10/05/09 20:30, Bastian Blank wrote:
> The original version saves ecx, but not edx. Both are official
> caller-saved registers.
>   

Hm.  It doesn't save edx because that can be half of a 64-bit return
value, and in general both eax and edx are marked clobbered.  Except one
place; does the patch below help?

>> Besides, most of the code in that file isn't used unless you're using a
>> very old version of Xen; it will generally prefer to use the ones in
>> xen-asm_X.S.
>>     
> Well, my call stack say something different. It crashs during early
> startup without a console. The modifications to the function pointers is
> done much later.

You're right.  But you're holding out on me; can I see your backtrace? 
And the disassembly of the troublesome code (both the Xen function and
the calling function)?

Thanks,
    J

From: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
Date: Tue, 6 Oct 2009 11:36:44 -0700
Subject: [PATCH] x86/paravirt: use normal calling sequences for irq enable/disable etc

For historical reasons irq enable/disable/save/restore had special
calling sequences to make them more efficient.  With the more
recent introduction of higher-level and more general optimisations
this is no longer necessary so we can just use the normal PVOP_
macros.

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

diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
index e19ffe3..e8420a2 100644
--- a/arch/x86/include/asm/paravirt.h
+++ b/arch/x86/include/asm/paravirt.h
@@ -863,42 +863,22 @@ static __always_inline void __raw_spin_unlock(struct raw_spinlock *lock)
 
 static inline unsigned long __raw_local_save_flags(void)
 {
-	unsigned long f;
-
-	asm volatile(paravirt_alt(PARAVIRT_CALL)
-		     : "=a"(f)
-		     : paravirt_type(pv_irq_ops.save_fl),
-		       paravirt_clobber(CLBR_EAX)
-		     : "memory", "cc");
-	return f;
+	return PVOP_CALLEE0(unsigned long, pv_irq_ops.save_fl);
 }
 
 static inline void raw_local_irq_restore(unsigned long f)
 {
-	asm volatile(paravirt_alt(PARAVIRT_CALL)
-		     : "=a"(f)
-		     : PV_FLAGS_ARG(f),
-		       paravirt_type(pv_irq_ops.restore_fl),
-		       paravirt_clobber(CLBR_EAX)
-		     : "memory", "cc");
+	PVOP_VCALLEE1(pv_irq_ops.restore_fl, f);
 }
 
 static inline void raw_local_irq_disable(void)
 {
-	asm volatile(paravirt_alt(PARAVIRT_CALL)
-		     :
-		     : paravirt_type(pv_irq_ops.irq_disable),
-		       paravirt_clobber(CLBR_EAX)
-		     : "memory", "eax", "cc");
+	PVOP_VCALLEE0(pv_irq_ops.irq_disable);
 }
 
 static inline void raw_local_irq_enable(void)
 {
-	asm volatile(paravirt_alt(PARAVIRT_CALL)
-		     :
-		     : paravirt_type(pv_irq_ops.irq_enable),
-		       paravirt_clobber(CLBR_EAX)
-		     : "memory", "eax", "cc");
+	PVOP_VCALLEE0(pv_irq_ops.irq_enable);
 }
 
 static inline unsigned long __raw_local_irq_save(void)
diff --git a/arch/x86/include/asm/paravirt_types.h b/arch/x86/include/asm/paravirt_types.h
index b9bb5e8..0b97706 100644
--- a/arch/x86/include/asm/paravirt_types.h
+++ b/arch/x86/include/asm/paravirt_types.h
@@ -612,8 +612,8 @@ int paravirt_disable_iospace(void);
 		       VEXTRA_CLOBBERS,					\
 		       pre, post, ##__VA_ARGS__)
 
-#define __PVOP_VCALLEESAVE(rettype, op, pre, post, ...)			\
-	____PVOP_CALL(rettype, op.func, CLBR_RET_REG,			\
+#define __PVOP_VCALLEESAVE(op, pre, post, ...)				\
+	____PVOP_VCALL(op.func, CLBR_RET_REG,				\
 		      PVOP_VCALLEE_CLOBBERS, ,				\
 		      pre, post, ##__VA_ARGS__)
 

Re: Re: [PATCH] xen: Disable stack protector for irq helper

[Jeremy Fitzhardinge]

On 10/05/09 20:30, Bastian Blank wrote:
> The original version saves ecx, but not edx. Both are official
> caller-saved registers.
>   

Hm.  It doesn't save edx because that can be half of a 64-bit return
value, and in general both eax and edx are marked clobbered.  Except one
place; does the patch below help?

>> Besides, most of the code in that file isn't used unless you're using a
>> very old version of Xen; it will generally prefer to use the ones in
>> xen-asm_X.S.
>>     
> Well, my call stack say something different. It crashs during early
> startup without a console. The modifications to the function pointers is
> done much later.

You're right.  But you're holding out on me; can I see your backtrace? 
And the disassembly of the troublesome code (both the Xen function and
the calling function)?

Thanks,
    J

From: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
Date: Tue, 6 Oct 2009 11:36:44 -0700
Subject: [PATCH] x86/paravirt: use normal calling sequences for irq enable/disable etc

For historical reasons irq enable/disable/save/restore had special
calling sequences to make them more efficient.  With the more
recent introduction of higher-level and more general optimisations
this is no longer necessary so we can just use the normal PVOP_
macros.

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

diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
index e19ffe3..e8420a2 100644
--- a/arch/x86/include/asm/paravirt.h
+++ b/arch/x86/include/asm/paravirt.h
@@ -863,42 +863,22 @@ static __always_inline void __raw_spin_unlock(struct raw_spinlock *lock)
 
 static inline unsigned long __raw_local_save_flags(void)
 {
-	unsigned long f;
-
-	asm volatile(paravirt_alt(PARAVIRT_CALL)
-		     : "=a"(f)
-		     : paravirt_type(pv_irq_ops.save_fl),
-		       paravirt_clobber(CLBR_EAX)
-		     : "memory", "cc");
-	return f;
+	return PVOP_CALLEE0(unsigned long, pv_irq_ops.save_fl);
 }
 
 static inline void raw_local_irq_restore(unsigned long f)
 {
-	asm volatile(paravirt_alt(PARAVIRT_CALL)
-		     : "=a"(f)
-		     : PV_FLAGS_ARG(f),
-		       paravirt_type(pv_irq_ops.restore_fl),
-		       paravirt_clobber(CLBR_EAX)
-		     : "memory", "cc");
+	PVOP_VCALLEE1(pv_irq_ops.restore_fl, f);
 }
 
 static inline void raw_local_irq_disable(void)
 {
-	asm volatile(paravirt_alt(PARAVIRT_CALL)
-		     :
-		     : paravirt_type(pv_irq_ops.irq_disable),
-		       paravirt_clobber(CLBR_EAX)
-		     : "memory", "eax", "cc");
+	PVOP_VCALLEE0(pv_irq_ops.irq_disable);
 }
 
 static inline void raw_local_irq_enable(void)
 {
-	asm volatile(paravirt_alt(PARAVIRT_CALL)
-		     :
-		     : paravirt_type(pv_irq_ops.irq_enable),
-		       paravirt_clobber(CLBR_EAX)
-		     : "memory", "eax", "cc");
+	PVOP_VCALLEE0(pv_irq_ops.irq_enable);
 }
 
 static inline unsigned long __raw_local_irq_save(void)
diff --git a/arch/x86/include/asm/paravirt_types.h b/arch/x86/include/asm/paravirt_types.h
index b9bb5e8..0b97706 100644
--- a/arch/x86/include/asm/paravirt_types.h
+++ b/arch/x86/include/asm/paravirt_types.h
@@ -612,8 +612,8 @@ int paravirt_disable_iospace(void);
 		       VEXTRA_CLOBBERS,					\
 		       pre, post, ##__VA_ARGS__)
 
-#define __PVOP_VCALLEESAVE(rettype, op, pre, post, ...)			\
-	____PVOP_CALL(rettype, op.func, CLBR_RET_REG,			\
+#define __PVOP_VCALLEESAVE(op, pre, post, ...)				\
+	____PVOP_VCALL(op.func, CLBR_RET_REG,				\
 		      PVOP_VCALLEE_CLOBBERS, ,				\
 		      pre, post, ##__VA_ARGS__)

Re: [Xen-devel] Re: [PATCH] xen: Disable stack protector for irq helper

[Bastian Blank]

On Tue, Oct 06, 2009 at 12:01:12PM -0700, Jeremy Fitzhardinge wrote:
> On 10/05/09 20:30, Bastian Blank wrote:
> > The original version saves ecx, but not edx. Both are official
> > caller-saved registers.
> Hm.  It doesn't save edx because that can be half of a 64-bit return
> value, and in general both eax and edx are marked clobbered.

Then it will be also wrong for functions returning void. They may
clobber eax but never set it to something correct.

>                                                               Except one
> place; does the patch below help?

Don't you need to remove the complete wrapper setup to get a correct
result? (And type safety.)

> > Well, my call stack say something different. It crashs during early
> > startup without a console. The modifications to the function pointers is
> > done much later.
> You're right.  But you're holding out on me; can I see your backtrace? 

Well, I'm traveling and it needs some time to recreate a broken kernel.

> And the disassembly of the troublesome code (both the Xen function and
> the calling function)?

That is easy.

| c12dc725 <_spin_lock_irqsave>:
| c12dc725:       83 ec 04                sub    $0x4,%esp
| c12dc728:       89 c2                   mov    %eax,%edx
| c12dc72a:       65 a1 14 00 00 00       mov    %gs:0x14,%eax
| c12dc730:       89 04 24                mov    %eax,(%esp)
| c12dc733:       31 c0                   xor    %eax,%eax
| c12dc735:       ff 15 bc 1a 3f c1       call   *0xc13f1abc
Call to pv_irq_ops.save_fl.
| c12dc73b:       89 c1                   mov    %eax,%ecx
| c12dc73d:       ff 15 c4 1a 3f c1       call   *0xc13f1ac4
| c12dc743:       b8 00 01 00 00          mov    $0x100,%eax
| c12dc748:       f0 66 0f c1 02          lock xadd %ax,(%edx)
| c12dc74d:       38 e0                   cmp    %ah,%al
| c12dc74f:       74 06                   je     c12dc757 <_spin_lock_irqsave+0x32>
| c12dc751:       f3 90                   pause  
| c12dc753:       8a 02                   mov    (%edx),%al
Try to use (clobbered) edx.
| c12dc755:       eb f6                   jmp    c12dc74d <_spin_lock_irqsave+0x28>
| c12dc757:       8b 14 24                mov    (%esp),%edx
| c12dc75a:       65 33 15 14 00 00 00    xor    %gs:0x14,%edx
| c12dc761:       89 c8                   mov    %ecx,%eax
| c12dc763:       74 05                   je     c12dc76a <_spin_lock_irqsave+0x45>
| c12dc765:       e8 28 58 d6 ff          call   c1041f92 <__stack_chk_fail>
| c12dc76a:       5a                      pop    %edx
| c12dc76b:       c3                      ret    

| c1005dbc <xen_save_fl>:
| c1005dbc:       83 ec 04                sub    $0x4,%esp
| c1005dbf:       65 a1 14 00 00 00       mov    %gs:0x14,%eax
| c1005dc5:       89 04 24                mov    %eax,(%esp)
| c1005dc8:       31 c0                   xor    %eax,%eax
| c1005dca:       64 a1 0c 70 47 c1       mov    %fs:0xc147700c,%eax
| c1005dd0:       80 78 01 00             cmpb   $0x0,0x1(%eax)
| c1005dd4:       0f 94 c0                sete   %al
| c1005dd7:       0f b6 c0                movzbl %al,%eax
| c1005dda:       f7 d8                   neg    %eax
| c1005ddc:       25 00 02 00 00          and    $0x200,%eax
| c1005de1:       8b 14 24                mov    (%esp),%edx
| c1005de4:       65 33 15 14 00 00 00    xor    %gs:0x14,%edx
| c1005deb:       74 05                   je     c1005df2 <xen_save_fl+0x36>
| c1005ded:       e8 a0 c1 03 00          call   c1041f92 <__stack_chk_fail>
| c1005df2:       5a                      pop    %edx
Clobbers edx with the old eax.
| c1005df3:       c3                      ret    

| c13f1ab8 g     O .data  0000001c pv_irq_ops


-- 
I'm frequently appalled by the low regard you Earthmen have for life.
		-- Spock, "The Galileo Seven", stardate 2822.3

Re: Re: [PATCH] xen: Disable stack protector for irq helper

[Bastian Blank]

On Tue, Oct 06, 2009 at 12:01:12PM -0700, Jeremy Fitzhardinge wrote:
> On 10/05/09 20:30, Bastian Blank wrote:
> > The original version saves ecx, but not edx. Both are official
> > caller-saved registers.
> Hm.  It doesn't save edx because that can be half of a 64-bit return
> value, and in general both eax and edx are marked clobbered.

Then it will be also wrong for functions returning void. They may
clobber eax but never set it to something correct.

>                                                               Except one
> place; does the patch below help?

Don't you need to remove the complete wrapper setup to get a correct
result? (And type safety.)

> > Well, my call stack say something different. It crashs during early
> > startup without a console. The modifications to the function pointers is
> > done much later.
> You're right.  But you're holding out on me; can I see your backtrace? 

Well, I'm traveling and it needs some time to recreate a broken kernel.

> And the disassembly of the troublesome code (both the Xen function and
> the calling function)?

That is easy.

| c12dc725 <_spin_lock_irqsave>:
| c12dc725:       83 ec 04                sub    $0x4,%esp
| c12dc728:       89 c2                   mov    %eax,%edx
| c12dc72a:       65 a1 14 00 00 00       mov    %gs:0x14,%eax
| c12dc730:       89 04 24                mov    %eax,(%esp)
| c12dc733:       31 c0                   xor    %eax,%eax
| c12dc735:       ff 15 bc 1a 3f c1       call   *0xc13f1abc
Call to pv_irq_ops.save_fl.
| c12dc73b:       89 c1                   mov    %eax,%ecx
| c12dc73d:       ff 15 c4 1a 3f c1       call   *0xc13f1ac4
| c12dc743:       b8 00 01 00 00          mov    $0x100,%eax
| c12dc748:       f0 66 0f c1 02          lock xadd %ax,(%edx)
| c12dc74d:       38 e0                   cmp    %ah,%al
| c12dc74f:       74 06                   je     c12dc757 <_spin_lock_irqsave+0x32>
| c12dc751:       f3 90                   pause  
| c12dc753:       8a 02                   mov    (%edx),%al
Try to use (clobbered) edx.
| c12dc755:       eb f6                   jmp    c12dc74d <_spin_lock_irqsave+0x28>
| c12dc757:       8b 14 24                mov    (%esp),%edx
| c12dc75a:       65 33 15 14 00 00 00    xor    %gs:0x14,%edx
| c12dc761:       89 c8                   mov    %ecx,%eax
| c12dc763:       74 05                   je     c12dc76a <_spin_lock_irqsave+0x45>
| c12dc765:       e8 28 58 d6 ff          call   c1041f92 <__stack_chk_fail>
| c12dc76a:       5a                      pop    %edx
| c12dc76b:       c3                      ret    

| c1005dbc <xen_save_fl>:
| c1005dbc:       83 ec 04                sub    $0x4,%esp
| c1005dbf:       65 a1 14 00 00 00       mov    %gs:0x14,%eax
| c1005dc5:       89 04 24                mov    %eax,(%esp)
| c1005dc8:       31 c0                   xor    %eax,%eax
| c1005dca:       64 a1 0c 70 47 c1       mov    %fs:0xc147700c,%eax
| c1005dd0:       80 78 01 00             cmpb   $0x0,0x1(%eax)
| c1005dd4:       0f 94 c0                sete   %al
| c1005dd7:       0f b6 c0                movzbl %al,%eax
| c1005dda:       f7 d8                   neg    %eax
| c1005ddc:       25 00 02 00 00          and    $0x200,%eax
| c1005de1:       8b 14 24                mov    (%esp),%edx
| c1005de4:       65 33 15 14 00 00 00    xor    %gs:0x14,%edx
| c1005deb:       74 05                   je     c1005df2 <xen_save_fl+0x36>
| c1005ded:       e8 a0 c1 03 00          call   c1041f92 <__stack_chk_fail>
| c1005df2:       5a                      pop    %edx
Clobbers edx with the old eax.
| c1005df3:       c3                      ret    

| c13f1ab8 g     O .data  0000001c pv_irq_ops


-- 
I'm frequently appalled by the low regard you Earthmen have for life.
		-- Spock, "The Galileo Seven", stardate 2822.3

Re: [Xen-devel] Re: [PATCH] xen: Disable stack protector for irq helper

[Jeremy Fitzhardinge]

On 10/07/09 09:35, Bastian Blank wrote:
> On Tue, Oct 06, 2009 at 12:01:12PM -0700, Jeremy Fitzhardinge wrote:
>   
>> On 10/05/09 20:30, Bastian Blank wrote:
>>     
>>> The original version saves ecx, but not edx. Both are official
>>> caller-saved registers.
>>>       
>> Hm.  It doesn't save edx because that can be half of a 64-bit return
>> value, and in general both eax and edx are marked clobbered.
>>     
> Then it will be also wrong for functions returning void. They may
> clobber eax but never set it to something correct.
>   

The asm is marked as clobbering eax/edx, so the compiler knows it can't
rely on them being preserved.  void functions are not expected to
preserve them either, so it all works out (or if they are, the compiler
will do the right thing).

Or did you have something else in mind?


>>                                                               Except one
>> place; does the patch below help?
>>     
> Don't you need to remove the complete wrapper setup to get a correct
> result? (And type safety.)
>   

Which wrapper?  Do you mean the callee-save function stuff?  Or
something else?

It compiles cleanly for me and appears to work.  Does it solve the
problem for you?

>>> Well, my call stack say something different. It crashs during early
>>> startup without a console. The modifications to the function pointers is
>>> done much later.
>>>       
>> You're right.  But you're holding out on me; can I see your backtrace? 
>>     
> Well, I'm traveling and it needs some time to recreate a broken kernel.
>
>   
>> And the disassembly of the troublesome code (both the Xen function and
>> the calling function)?
>>     
> That is easy.
>   

OK, I see, thanks.

    J

Re: Re: [PATCH] xen: Disable stack protector for irq helper

[Jeremy Fitzhardinge]

On 10/07/09 09:35, Bastian Blank wrote:
> On Tue, Oct 06, 2009 at 12:01:12PM -0700, Jeremy Fitzhardinge wrote:
>   
>> On 10/05/09 20:30, Bastian Blank wrote:
>>     
>>> The original version saves ecx, but not edx. Both are official
>>> caller-saved registers.
>>>       
>> Hm.  It doesn't save edx because that can be half of a 64-bit return
>> value, and in general both eax and edx are marked clobbered.
>>     
> Then it will be also wrong for functions returning void. They may
> clobber eax but never set it to something correct.
>   

The asm is marked as clobbering eax/edx, so the compiler knows it can't
rely on them being preserved.  void functions are not expected to
preserve them either, so it all works out (or if they are, the compiler
will do the right thing).

Or did you have something else in mind?


>>                                                               Except one
>> place; does the patch below help?
>>     
> Don't you need to remove the complete wrapper setup to get a correct
> result? (And type safety.)
>   

Which wrapper?  Do you mean the callee-save function stuff?  Or
something else?

It compiles cleanly for me and appears to work.  Does it solve the
problem for you?

>>> Well, my call stack say something different. It crashs during early
>>> startup without a console. The modifications to the function pointers is
>>> done much later.
>>>       
>> You're right.  But you're holding out on me; can I see your backtrace? 
>>     
> Well, I'm traveling and it needs some time to recreate a broken kernel.
>
>   
>> And the disassembly of the troublesome code (both the Xen function and
>> the calling function)?
>>     
> That is easy.
>   

OK, I see, thanks.

    J

Re: [Xen-devel] Re: [PATCH] xen: Disable stack protector for irq helper

[Ingo Molnar]

* Jeremy Fitzhardinge <jeremy@goop.org> wrote:

> On 10/07/09 09:35, Bastian Blank wrote:
> > On Tue, Oct 06, 2009 at 12:01:12PM -0700, Jeremy Fitzhardinge wrote:
> >   
> >> On 10/05/09 20:30, Bastian Blank wrote:
> >>     
> >>> The original version saves ecx, but not edx. Both are official
> >>> caller-saved registers.
> >>>       
> >> Hm.  It doesn't save edx because that can be half of a 64-bit return
> >> value, and in general both eax and edx are marked clobbered.
> >>     
> > Then it will be also wrong for functions returning void. They may
> > clobber eax but never set it to something correct.
> >   
> 
> The asm is marked as clobbering eax/edx, so the compiler knows it can't
> rely on them being preserved.  void functions are not expected to
> preserve them either, so it all works out (or if they are, the compiler
> will do the right thing).
> 
> Or did you have something else in mind?
> 
> 
> >>                                                               Except one
> >> place; does the patch below help?
> >>     
> > Don't you need to remove the complete wrapper setup to get a correct
> > result? (And type safety.)
> >   
> 
> Which wrapper?  Do you mean the callee-save function stuff?  Or
> something else?
> 
> It compiles cleanly for me and appears to work.  Does it solve the
> problem for you?
> 
> >>> Well, my call stack say something different. It crashs during early
> >>> startup without a console. The modifications to the function pointers is
> >>> done much later.
> >>>       
> >> You're right.  But you're holding out on me; can I see your backtrace? 
> >>     
> > Well, I'm traveling and it needs some time to recreate a broken kernel.
> >
> >   
> >> And the disassembly of the troublesome code (both the Xen function and
> >> the calling function)?
> >>     
> > That is easy.
> >   
> 
> OK, I see, thanks.

ping - any update about this fix? Since it fixes a real crash it would 
be nice to fix this for .32.

	Ingo

Re: Re: [PATCH] xen: Disable stack protector for irq helper

[Ingo Molnar]

* Jeremy Fitzhardinge <jeremy@goop.org> wrote:

> On 10/07/09 09:35, Bastian Blank wrote:
> > On Tue, Oct 06, 2009 at 12:01:12PM -0700, Jeremy Fitzhardinge wrote:
> >   
> >> On 10/05/09 20:30, Bastian Blank wrote:
> >>     
> >>> The original version saves ecx, but not edx. Both are official
> >>> caller-saved registers.
> >>>       
> >> Hm.  It doesn't save edx because that can be half of a 64-bit return
> >> value, and in general both eax and edx are marked clobbered.
> >>     
> > Then it will be also wrong for functions returning void. They may
> > clobber eax but never set it to something correct.
> >   
> 
> The asm is marked as clobbering eax/edx, so the compiler knows it can't
> rely on them being preserved.  void functions are not expected to
> preserve them either, so it all works out (or if they are, the compiler
> will do the right thing).
> 
> Or did you have something else in mind?
> 
> 
> >>                                                               Except one
> >> place; does the patch below help?
> >>     
> > Don't you need to remove the complete wrapper setup to get a correct
> > result? (And type safety.)
> >   
> 
> Which wrapper?  Do you mean the callee-save function stuff?  Or
> something else?
> 
> It compiles cleanly for me and appears to work.  Does it solve the
> problem for you?
> 
> >>> Well, my call stack say something different. It crashs during early
> >>> startup without a console. The modifications to the function pointers is
> >>> done much later.
> >>>       
> >> You're right.  But you're holding out on me; can I see your backtrace? 
> >>     
> > Well, I'm traveling and it needs some time to recreate a broken kernel.
> >
> >   
> >> And the disassembly of the troublesome code (both the Xen function and
> >> the calling function)?
> >>     
> > That is easy.
> >   
> 
> OK, I see, thanks.

ping - any update about this fix? Since it fixes a real crash it would 
be nice to fix this for .32.

	Ingo

Re: [Xen-devel] Re: [PATCH] xen: Disable stack protector for irq helper

[Bastian Blank]

On Mon, Oct 12, 2009 at 10:52:08PM +0200, Ingo Molnar wrote:
> ping - any update about this fix? Since it fixes a real crash it would 
> be nice to fix this for .32.

It works nicely.

But IMHO this whole infrastructure should go for now, at least until gcc
is able to produce functions with this call convention on its own. Or it
needs to be restricted to only assembler functions. The other users of
this may only work because the stack protector is already disabled for
arch/x86/xen/mmu.o.

Bastian

-- 
Men will always be men -- no matter where they are.
		-- Harry Mudd, "Mudd's Women", stardate 1329.8

Re: Re: [PATCH] xen: Disable stack protector for irq helper

[Bastian Blank]

On Mon, Oct 12, 2009 at 10:52:08PM +0200, Ingo Molnar wrote:
> ping - any update about this fix? Since it fixes a real crash it would 
> be nice to fix this for .32.

It works nicely.

But IMHO this whole infrastructure should go for now, at least until gcc
is able to produce functions with this call convention on its own. Or it
needs to be restricted to only assembler functions. The other users of
this may only work because the stack protector is already disabled for
arch/x86/xen/mmu.o.

Bastian

-- 
Men will always be men -- no matter where they are.
		-- Harry Mudd, "Mudd's Women", stardate 1329.8

Re: [Xen-devel] Re: [PATCH] xen: Disable stack protector for irq helper

[Jeremy Fitzhardinge]

On 10/12/09 14:12, Bastian Blank wrote:
> On Mon, Oct 12, 2009 at 10:52:08PM +0200, Ingo Molnar wrote:
>   
>> ping - any update about this fix? Since it fixes a real crash it would 
>> be nice to fix this for .32.
>>     
> It works nicely.
>
> But IMHO this whole infrastructure should go for now, at least until gcc
> is able to produce functions with this call convention on its own. Or it
> needs to be restricted to only assembler functions. The other users of
> this may only work because the stack protector is already disabled for
> arch/x86/xen/mmu.o.
>   

No, the infrastructure is fine and completely compliant with the ABI
(which doesn't change with stackprotector).  But there were a couple of
interrupt-related calls which didn't use the infrastructure properly,
and failed to preserve edx properly; we'd gotten away with it until now
because the called functions were very simple and didn't end up using
edx - until stackprotector.

The fix is to use the infrastructure consistently.

I'll put together a suitable patch.

    J

Re: Re: [PATCH] xen: Disable stack protector for irq helper

[Jeremy Fitzhardinge]

On 10/12/09 14:12, Bastian Blank wrote:
> On Mon, Oct 12, 2009 at 10:52:08PM +0200, Ingo Molnar wrote:
>   
>> ping - any update about this fix? Since it fixes a real crash it would 
>> be nice to fix this for .32.
>>     
> It works nicely.
>
> But IMHO this whole infrastructure should go for now, at least until gcc
> is able to produce functions with this call convention on its own. Or it
> needs to be restricted to only assembler functions. The other users of
> this may only work because the stack protector is already disabled for
> arch/x86/xen/mmu.o.
>   

No, the infrastructure is fine and completely compliant with the ABI
(which doesn't change with stackprotector).  But there were a couple of
interrupt-related calls which didn't use the infrastructure properly,
and failed to preserve edx properly; we'd gotten away with it until now
because the called functions were very simple and didn't end up using
edx - until stackprotector.

The fix is to use the infrastructure consistently.

I'll put together a suitable patch.

    J

Re: [Xen-devel] Re: [PATCH] xen: Disable stack protector for irq helper

[Jeremy Fitzhardinge]

On 10/12/09 13:52, Ingo Molnar wrote:
> ping - any update about this fix? Since it fixes a real crash it would 
> be nice to fix this for .32.
>   

The following changes since commit 2caa731819a633bec5a56736e64c562b7e193666:
  Linus Torvalds (1):
        Merge branch 'for-linus' of git://git.kernel.org/.../jbarnes/pci-2.6

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/jeremy/xen.git mainline/x86-paravirt-fix

Jeremy Fitzhardinge (1):
      x86/paravirt: use normal calling sequences for irq enable/disable etc

 arch/x86/include/asm/paravirt.h       |   28 ++++------------------------
 arch/x86/include/asm/paravirt_types.h |   10 ++++++----
 2 files changed, 10 insertions(+), 28 deletions(-)

Subject: [PATCH] x86/paravirt: use normal calling sequences for irq enable/disable etc

For historical reasons irq enable/disable/save/restore had special
calling sequences to make them more efficient.  With the more
recent introduction of higher-level and more general optimisations
this is no longer necessary so we can just use the normal PVOP_
macros.  This fixes some residual bugs in the old implementations which
left edx liable to inadvertent clobbering.

Also, fix some bugs in __PVOP_VCALLEESAVE which were revealed by actual
use.

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

diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
index 8aebcc4..efb3899 100644
--- a/arch/x86/include/asm/paravirt.h
+++ b/arch/x86/include/asm/paravirt.h
@@ -840,42 +840,22 @@ static __always_inline void __raw_spin_unlock(struct raw_spinlock *lock)
 
 static inline unsigned long __raw_local_save_flags(void)
 {
-	unsigned long f;
-
-	asm volatile(paravirt_alt(PARAVIRT_CALL)
-		     : "=a"(f)
-		     : paravirt_type(pv_irq_ops.save_fl),
-		       paravirt_clobber(CLBR_EAX)
-		     : "memory", "cc");
-	return f;
+	return PVOP_CALLEE0(unsigned long, pv_irq_ops.save_fl);
 }
 
 static inline void raw_local_irq_restore(unsigned long f)
 {
-	asm volatile(paravirt_alt(PARAVIRT_CALL)
-		     : "=a"(f)
-		     : PV_FLAGS_ARG(f),
-		       paravirt_type(pv_irq_ops.restore_fl),
-		       paravirt_clobber(CLBR_EAX)
-		     : "memory", "cc");
+	PVOP_VCALLEE1(pv_irq_ops.restore_fl, f);
 }
 
 static inline void raw_local_irq_disable(void)
 {
-	asm volatile(paravirt_alt(PARAVIRT_CALL)
-		     :
-		     : paravirt_type(pv_irq_ops.irq_disable),
-		       paravirt_clobber(CLBR_EAX)
-		     : "memory", "eax", "cc");
+	PVOP_VCALLEE0(pv_irq_ops.irq_disable);
 }
 
 static inline void raw_local_irq_enable(void)
 {
-	asm volatile(paravirt_alt(PARAVIRT_CALL)
-		     :
-		     : paravirt_type(pv_irq_ops.irq_enable),
-		       paravirt_clobber(CLBR_EAX)
-		     : "memory", "eax", "cc");
+	PVOP_VCALLEE0(pv_irq_ops.irq_enable);
 }
 
 static inline unsigned long __raw_local_irq_save(void)
diff --git a/arch/x86/include/asm/paravirt_types.h b/arch/x86/include/asm/paravirt_types.h
index dd0f5b3..9357473 100644
--- a/arch/x86/include/asm/paravirt_types.h
+++ b/arch/x86/include/asm/paravirt_types.h
@@ -494,10 +494,11 @@ int paravirt_disable_iospace(void);
 #define EXTRA_CLOBBERS
 #define VEXTRA_CLOBBERS
 #else  /* CONFIG_X86_64 */
+/* [re]ax isn't an arg, but the return val */
 #define PVOP_VCALL_ARGS					\
 	unsigned long __edi = __edi, __esi = __esi,	\
-		__edx = __edx, __ecx = __ecx
-#define PVOP_CALL_ARGS		PVOP_VCALL_ARGS, __eax
+		__edx = __edx, __ecx = __ecx, __eax = __eax
+#define PVOP_CALL_ARGS		PVOP_VCALL_ARGS
 
 #define PVOP_CALL_ARG1(x)		"D" ((unsigned long)(x))
 #define PVOP_CALL_ARG2(x)		"S" ((unsigned long)(x))
@@ -509,6 +510,7 @@ int paravirt_disable_iospace(void);
 				"=c" (__ecx)
 #define PVOP_CALL_CLOBBERS	PVOP_VCALL_CLOBBERS, "=a" (__eax)
 
+/* void functions are still allowed [re]ax for scratch */
 #define PVOP_VCALLEE_CLOBBERS	"=a" (__eax)
 #define PVOP_CALLEE_CLOBBERS	PVOP_VCALLEE_CLOBBERS
 
@@ -583,8 +585,8 @@ int paravirt_disable_iospace(void);
 		       VEXTRA_CLOBBERS,					\
 		       pre, post, ##__VA_ARGS__)
 
-#define __PVOP_VCALLEESAVE(rettype, op, pre, post, ...)			\
-	____PVOP_CALL(rettype, op.func, CLBR_RET_REG,			\
+#define __PVOP_VCALLEESAVE(op, pre, post, ...)				\
+	____PVOP_VCALL(op.func, CLBR_RET_REG,				\
 		      PVOP_VCALLEE_CLOBBERS, ,				\
 		      pre, post, ##__VA_ARGS__)

Re: Re: [PATCH] xen: Disable stack protector for irq helper

[Jeremy Fitzhardinge]

On 10/12/09 13:52, Ingo Molnar wrote:
> ping - any update about this fix? Since it fixes a real crash it would 
> be nice to fix this for .32.
>   

The following changes since commit 2caa731819a633bec5a56736e64c562b7e193666:
  Linus Torvalds (1):
        Merge branch 'for-linus' of git://git.kernel.org/.../jbarnes/pci-2.6

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/jeremy/xen.git mainline/x86-paravirt-fix

Jeremy Fitzhardinge (1):
      x86/paravirt: use normal calling sequences for irq enable/disable etc

 arch/x86/include/asm/paravirt.h       |   28 ++++------------------------
 arch/x86/include/asm/paravirt_types.h |   10 ++++++----
 2 files changed, 10 insertions(+), 28 deletions(-)

Subject: [PATCH] x86/paravirt: use normal calling sequences for irq enable/disable etc

For historical reasons irq enable/disable/save/restore had special
calling sequences to make them more efficient.  With the more
recent introduction of higher-level and more general optimisations
this is no longer necessary so we can just use the normal PVOP_
macros.  This fixes some residual bugs in the old implementations which
left edx liable to inadvertent clobbering.

Also, fix some bugs in __PVOP_VCALLEESAVE which were revealed by actual
use.

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

diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
index 8aebcc4..efb3899 100644
--- a/arch/x86/include/asm/paravirt.h
+++ b/arch/x86/include/asm/paravirt.h
@@ -840,42 +840,22 @@ static __always_inline void __raw_spin_unlock(struct raw_spinlock *lock)
 
 static inline unsigned long __raw_local_save_flags(void)
 {
-	unsigned long f;
-
-	asm volatile(paravirt_alt(PARAVIRT_CALL)
-		     : "=a"(f)
-		     : paravirt_type(pv_irq_ops.save_fl),
-		       paravirt_clobber(CLBR_EAX)
-		     : "memory", "cc");
-	return f;
+	return PVOP_CALLEE0(unsigned long, pv_irq_ops.save_fl);
 }
 
 static inline void raw_local_irq_restore(unsigned long f)
 {
-	asm volatile(paravirt_alt(PARAVIRT_CALL)
-		     : "=a"(f)
-		     : PV_FLAGS_ARG(f),
-		       paravirt_type(pv_irq_ops.restore_fl),
-		       paravirt_clobber(CLBR_EAX)
-		     : "memory", "cc");
+	PVOP_VCALLEE1(pv_irq_ops.restore_fl, f);
 }
 
 static inline void raw_local_irq_disable(void)
 {
-	asm volatile(paravirt_alt(PARAVIRT_CALL)
-		     :
-		     : paravirt_type(pv_irq_ops.irq_disable),
-		       paravirt_clobber(CLBR_EAX)
-		     : "memory", "eax", "cc");
+	PVOP_VCALLEE0(pv_irq_ops.irq_disable);
 }
 
 static inline void raw_local_irq_enable(void)
 {
-	asm volatile(paravirt_alt(PARAVIRT_CALL)
-		     :
-		     : paravirt_type(pv_irq_ops.irq_enable),
-		       paravirt_clobber(CLBR_EAX)
-		     : "memory", "eax", "cc");
+	PVOP_VCALLEE0(pv_irq_ops.irq_enable);
 }
 
 static inline unsigned long __raw_local_irq_save(void)
diff --git a/arch/x86/include/asm/paravirt_types.h b/arch/x86/include/asm/paravirt_types.h
index dd0f5b3..9357473 100644
--- a/arch/x86/include/asm/paravirt_types.h
+++ b/arch/x86/include/asm/paravirt_types.h
@@ -494,10 +494,11 @@ int paravirt_disable_iospace(void);
 #define EXTRA_CLOBBERS
 #define VEXTRA_CLOBBERS
 #else  /* CONFIG_X86_64 */
+/* [re]ax isn't an arg, but the return val */
 #define PVOP_VCALL_ARGS					\
 	unsigned long __edi = __edi, __esi = __esi,	\
-		__edx = __edx, __ecx = __ecx
-#define PVOP_CALL_ARGS		PVOP_VCALL_ARGS, __eax
+		__edx = __edx, __ecx = __ecx, __eax = __eax
+#define PVOP_CALL_ARGS		PVOP_VCALL_ARGS
 
 #define PVOP_CALL_ARG1(x)		"D" ((unsigned long)(x))
 #define PVOP_CALL_ARG2(x)		"S" ((unsigned long)(x))
@@ -509,6 +510,7 @@ int paravirt_disable_iospace(void);
 				"=c" (__ecx)
 #define PVOP_CALL_CLOBBERS	PVOP_VCALL_CLOBBERS, "=a" (__eax)
 
+/* void functions are still allowed [re]ax for scratch */
 #define PVOP_VCALLEE_CLOBBERS	"=a" (__eax)
 #define PVOP_CALLEE_CLOBBERS	PVOP_VCALLEE_CLOBBERS
 
@@ -583,8 +585,8 @@ int paravirt_disable_iospace(void);
 		       VEXTRA_CLOBBERS,					\
 		       pre, post, ##__VA_ARGS__)
 
-#define __PVOP_VCALLEESAVE(rettype, op, pre, post, ...)			\
-	____PVOP_CALL(rettype, op.func, CLBR_RET_REG,			\
+#define __PVOP_VCALLEESAVE(op, pre, post, ...)				\
+	____PVOP_VCALL(op.func, CLBR_RET_REG,				\
 		      PVOP_VCALLEE_CLOBBERS, ,				\
 		      pre, post, ##__VA_ARGS__)

[tip:x86/urgent] x86/paravirt: Use normal calling sequences for irq enable/disable

[tip-bot for Jeremy Fitzhardinge]

Commit-ID:  71999d9862e667f1fd14f8fbfa0cce6d855bad3f
Gitweb:     http://git.kernel.org/tip/71999d9862e667f1fd14f8fbfa0cce6d855bad3f
Author:     Jeremy Fitzhardinge <jeremy@goop.org>
AuthorDate: Mon, 12 Oct 2009 16:32:43 -0700
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Tue, 13 Oct 2009 09:22:01 +0200

x86/paravirt: Use normal calling sequences for irq enable/disable

Bastian Blank reported a boot crash with stackprotector enabled,
and debugged it back to edx register corruption.

For historical reasons irq enable/disable/save/restore had special
calling sequences to make them more efficient.  With the more
recent introduction of higher-level and more general optimisations
this is no longer necessary so we can just use the normal PVOP_
macros.

This fixes some residual bugs in the old implementations which left
edx liable to inadvertent clobbering. Also, fix some bugs in
__PVOP_VCALLEESAVE which were revealed by actual use.

Reported-by: Bastian Blank <bastian@waldi.eu.org>
Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
Cc: Stable Kernel <stable@kernel.org>
Cc: Xen-devel <xen-devel@lists.xensource.com>
LKML-Reference: <4AD3BC9B.7040501@goop.org>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/include/asm/paravirt.h       |   28 ++++------------------------
 arch/x86/include/asm/paravirt_types.h |   10 ++++++----
 2 files changed, 10 insertions(+), 28 deletions(-)

diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
index 8aebcc4..efb3899 100644
--- a/arch/x86/include/asm/paravirt.h
+++ b/arch/x86/include/asm/paravirt.h
@@ -840,42 +840,22 @@ static __always_inline void __raw_spin_unlock(struct raw_spinlock *lock)
 
 static inline unsigned long __raw_local_save_flags(void)
 {
-	unsigned long f;
-
-	asm volatile(paravirt_alt(PARAVIRT_CALL)
-		     : "=a"(f)
-		     : paravirt_type(pv_irq_ops.save_fl),
-		       paravirt_clobber(CLBR_EAX)
-		     : "memory", "cc");
-	return f;
+	return PVOP_CALLEE0(unsigned long, pv_irq_ops.save_fl);
 }
 
 static inline void raw_local_irq_restore(unsigned long f)
 {
-	asm volatile(paravirt_alt(PARAVIRT_CALL)
-		     : "=a"(f)
-		     : PV_FLAGS_ARG(f),
-		       paravirt_type(pv_irq_ops.restore_fl),
-		       paravirt_clobber(CLBR_EAX)
-		     : "memory", "cc");
+	PVOP_VCALLEE1(pv_irq_ops.restore_fl, f);
 }
 
 static inline void raw_local_irq_disable(void)
 {
-	asm volatile(paravirt_alt(PARAVIRT_CALL)
-		     :
-		     : paravirt_type(pv_irq_ops.irq_disable),
-		       paravirt_clobber(CLBR_EAX)
-		     : "memory", "eax", "cc");
+	PVOP_VCALLEE0(pv_irq_ops.irq_disable);
 }
 
 static inline void raw_local_irq_enable(void)
 {
-	asm volatile(paravirt_alt(PARAVIRT_CALL)
-		     :
-		     : paravirt_type(pv_irq_ops.irq_enable),
-		       paravirt_clobber(CLBR_EAX)
-		     : "memory", "eax", "cc");
+	PVOP_VCALLEE0(pv_irq_ops.irq_enable);
 }
 
 static inline unsigned long __raw_local_irq_save(void)
diff --git a/arch/x86/include/asm/paravirt_types.h b/arch/x86/include/asm/paravirt_types.h
index dd0f5b3..9357473 100644
--- a/arch/x86/include/asm/paravirt_types.h
+++ b/arch/x86/include/asm/paravirt_types.h
@@ -494,10 +494,11 @@ int paravirt_disable_iospace(void);
 #define EXTRA_CLOBBERS
 #define VEXTRA_CLOBBERS
 #else  /* CONFIG_X86_64 */
+/* [re]ax isn't an arg, but the return val */
 #define PVOP_VCALL_ARGS					\
 	unsigned long __edi = __edi, __esi = __esi,	\
-		__edx = __edx, __ecx = __ecx
-#define PVOP_CALL_ARGS		PVOP_VCALL_ARGS, __eax
+		__edx = __edx, __ecx = __ecx, __eax = __eax
+#define PVOP_CALL_ARGS		PVOP_VCALL_ARGS
 
 #define PVOP_CALL_ARG1(x)		"D" ((unsigned long)(x))
 #define PVOP_CALL_ARG2(x)		"S" ((unsigned long)(x))
@@ -509,6 +510,7 @@ int paravirt_disable_iospace(void);
 				"=c" (__ecx)
 #define PVOP_CALL_CLOBBERS	PVOP_VCALL_CLOBBERS, "=a" (__eax)
 
+/* void functions are still allowed [re]ax for scratch */
 #define PVOP_VCALLEE_CLOBBERS	"=a" (__eax)
 #define PVOP_CALLEE_CLOBBERS	PVOP_VCALLEE_CLOBBERS
 
@@ -583,8 +585,8 @@ int paravirt_disable_iospace(void);
 		       VEXTRA_CLOBBERS,					\
 		       pre, post, ##__VA_ARGS__)
 
-#define __PVOP_VCALLEESAVE(rettype, op, pre, post, ...)			\
-	____PVOP_CALL(rettype, op.func, CLBR_RET_REG,			\
+#define __PVOP_VCALLEESAVE(op, pre, post, ...)				\
+	____PVOP_VCALL(op.func, CLBR_RET_REG,				\
 		      PVOP_VCALLEE_CLOBBERS, ,				\
 		      pre, post, ##__VA_ARGS__)
 

[tip:x86/urgent] x86/paravirt: Use normal calling sequences for irq enable/disable

[tip-bot for Jeremy Fitzhardinge]

Commit-ID:  71999d9862e667f1fd14f8fbfa0cce6d855bad3f
Gitweb:     http://git.kernel.org/tip/71999d9862e667f1fd14f8fbfa0cce6d855bad3f
Author:     Jeremy Fitzhardinge <jeremy@goop.org>
AuthorDate: Mon, 12 Oct 2009 16:32:43 -0700
Committer:  Ingo Molnar <mingo@elte.hu>
CommitDate: Tue, 13 Oct 2009 09:22:01 +0200

x86/paravirt: Use normal calling sequences for irq enable/disable

Bastian Blank reported a boot crash with stackprotector enabled,
and debugged it back to edx register corruption.

For historical reasons irq enable/disable/save/restore had special
calling sequences to make them more efficient.  With the more
recent introduction of higher-level and more general optimisations
this is no longer necessary so we can just use the normal PVOP_
macros.

This fixes some residual bugs in the old implementations which left
edx liable to inadvertent clobbering. Also, fix some bugs in
__PVOP_VCALLEESAVE which were revealed by actual use.

Reported-by: Bastian Blank <bastian@waldi.eu.org>
Signed-off-by: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
Cc: Stable Kernel <stable@kernel.org>
Cc: Xen-devel <xen-devel@lists.xensource.com>
LKML-Reference: <4AD3BC9B.7040501@goop.org>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 arch/x86/include/asm/paravirt.h       |   28 ++++------------------------
 arch/x86/include/asm/paravirt_types.h |   10 ++++++----
 2 files changed, 10 insertions(+), 28 deletions(-)

diff --git a/arch/x86/include/asm/paravirt.h b/arch/x86/include/asm/paravirt.h
index 8aebcc4..efb3899 100644
--- a/arch/x86/include/asm/paravirt.h
+++ b/arch/x86/include/asm/paravirt.h
@@ -840,42 +840,22 @@ static __always_inline void __raw_spin_unlock(struct raw_spinlock *lock)
 
 static inline unsigned long __raw_local_save_flags(void)
 {
-	unsigned long f;
-
-	asm volatile(paravirt_alt(PARAVIRT_CALL)
-		     : "=a"(f)
-		     : paravirt_type(pv_irq_ops.save_fl),
-		       paravirt_clobber(CLBR_EAX)
-		     : "memory", "cc");
-	return f;
+	return PVOP_CALLEE0(unsigned long, pv_irq_ops.save_fl);
 }
 
 static inline void raw_local_irq_restore(unsigned long f)
 {
-	asm volatile(paravirt_alt(PARAVIRT_CALL)
-		     : "=a"(f)
-		     : PV_FLAGS_ARG(f),
-		       paravirt_type(pv_irq_ops.restore_fl),
-		       paravirt_clobber(CLBR_EAX)
-		     : "memory", "cc");
+	PVOP_VCALLEE1(pv_irq_ops.restore_fl, f);
 }
 
 static inline void raw_local_irq_disable(void)
 {
-	asm volatile(paravirt_alt(PARAVIRT_CALL)
-		     :
-		     : paravirt_type(pv_irq_ops.irq_disable),
-		       paravirt_clobber(CLBR_EAX)
-		     : "memory", "eax", "cc");
+	PVOP_VCALLEE0(pv_irq_ops.irq_disable);
 }
 
 static inline void raw_local_irq_enable(void)
 {
-	asm volatile(paravirt_alt(PARAVIRT_CALL)
-		     :
-		     : paravirt_type(pv_irq_ops.irq_enable),
-		       paravirt_clobber(CLBR_EAX)
-		     : "memory", "eax", "cc");
+	PVOP_VCALLEE0(pv_irq_ops.irq_enable);
 }
 
 static inline unsigned long __raw_local_irq_save(void)
diff --git a/arch/x86/include/asm/paravirt_types.h b/arch/x86/include/asm/paravirt_types.h
index dd0f5b3..9357473 100644
--- a/arch/x86/include/asm/paravirt_types.h
+++ b/arch/x86/include/asm/paravirt_types.h
@@ -494,10 +494,11 @@ int paravirt_disable_iospace(void);
 #define EXTRA_CLOBBERS
 #define VEXTRA_CLOBBERS
 #else  /* CONFIG_X86_64 */
+/* [re]ax isn't an arg, but the return val */
 #define PVOP_VCALL_ARGS					\
 	unsigned long __edi = __edi, __esi = __esi,	\
-		__edx = __edx, __ecx = __ecx
-#define PVOP_CALL_ARGS		PVOP_VCALL_ARGS, __eax
+		__edx = __edx, __ecx = __ecx, __eax = __eax
+#define PVOP_CALL_ARGS		PVOP_VCALL_ARGS
 
 #define PVOP_CALL_ARG1(x)		"D" ((unsigned long)(x))
 #define PVOP_CALL_ARG2(x)		"S" ((unsigned long)(x))
@@ -509,6 +510,7 @@ int paravirt_disable_iospace(void);
 				"=c" (__ecx)
 #define PVOP_CALL_CLOBBERS	PVOP_VCALL_CLOBBERS, "=a" (__eax)
 
+/* void functions are still allowed [re]ax for scratch */
 #define PVOP_VCALLEE_CLOBBERS	"=a" (__eax)
 #define PVOP_CALLEE_CLOBBERS	PVOP_VCALLEE_CLOBBERS
 
@@ -583,8 +585,8 @@ int paravirt_disable_iospace(void);
 		       VEXTRA_CLOBBERS,					\
 		       pre, post, ##__VA_ARGS__)
 
-#define __PVOP_VCALLEESAVE(rettype, op, pre, post, ...)			\
-	____PVOP_CALL(rettype, op.func, CLBR_RET_REG,			\
+#define __PVOP_VCALLEESAVE(op, pre, post, ...)				\
+	____PVOP_VCALL(op.func, CLBR_RET_REG,				\
 		      PVOP_VCALLEE_CLOBBERS, ,				\
 		      pre, post, ##__VA_ARGS__)