* [PATCH 0/3] minor cleanups to EFLAGS initialisation in ret_from_fork
@ 2011-07-25 9:58 Ian Campbell
2011-07-25 10:03 ` [PATCH 1/3] x86: drop unnecessary kernel_eflags variable from 64 bit Ian Campbell
` (3 more replies)
0 siblings, 4 replies; 22+ messages in thread
From: Ian Campbell @ 2011-07-25 9:58 UTC (permalink / raw)
To: linux-kernel; +Cc: Pekka Enberg, Cyrill Gorcunov, H. Peter Anvin, Andi Kleen
The following series removes the use of a global kernel_eflags variable
from the x86_64 ret_from_fork path and (very slightly) merges the 32 and
64 bit version of that code path.
kernel_eflags could be made a __read_mostly but actually there is no
reason to prefer the value at cpu_init() time to a compile time constant
value for the initial eflags after a fork.
Ian.
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/3] x86: drop unnecessary kernel_eflags variable from 64 bit.
2011-07-25 9:58 [PATCH 0/3] minor cleanups to EFLAGS initialisation in ret_from_fork Ian Campbell
@ 2011-07-25 10:03 ` Ian Campbell
2011-07-25 10:03 ` [PATCH 2/3] x86: make 64 bit ret_from_fork a little more similar to 32 bit Ian Campbell
` (2 subsequent siblings)
3 siblings, 0 replies; 22+ messages in thread
From: Ian Campbell @ 2011-07-25 10:03 UTC (permalink / raw)
To: linux-kernel
Cc: Pekka Enberg, Cyrill Gorcunov, H. Peter Anvin, Andi Kleen,
Ian Campbell, x86
On 64 bit x86 we save the current eflags in cpu_init for use in ret_from_fork.
Strictly speaking reserved bits in EFLAGS should be read as written but in
practise it is unlikely that EFLAGS could ever be extended in this way and the
kernel alread clears any undefined flags early on
The equivalent 32 bit code simply hard codes 0x0202 as the new EFLAGS (fixed to
use symbolic constants in a subsequent patch).
This change makes 64 bit use the same mechanism to setup the initial EFLAGS on
fork. Note that 64 bit resets EFLAGS before calling schedule_tail() as opposed
to 32 bit which calls schedule_tail() first. Therefore the correct value for
EFLAGS has opposite IF bit. This will be fixed in a subsequent patch.
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Acked-by: Cyrill Gorcunov <gorcunov@gmail.com>
Cc: x86@kernel.org
---
arch/x86/include/asm/processor.h | 1 -
arch/x86/kernel/cpu/common.c | 4 ----
arch/x86/kernel/entry_64.S | 2 +-
3 files changed, 1 insertions(+), 6 deletions(-)
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 2193715..698af88 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -398,7 +398,6 @@ DECLARE_INIT_PER_CPU(irq_stack_union);
DECLARE_PER_CPU(char *, irq_stack_ptr);
DECLARE_PER_CPU(unsigned int, irq_count);
-extern unsigned long kernel_eflags;
extern asmlinkage void ignore_sysret(void);
#else /* X86_64 */
#ifdef CONFIG_CC_STACKPROTECTOR
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 22a073d..178c3b4 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1077,8 +1077,6 @@ void syscall_init(void)
X86_EFLAGS_TF|X86_EFLAGS_DF|X86_EFLAGS_IF|X86_EFLAGS_IOPL);
}
-unsigned long kernel_eflags;
-
/*
* Copies of the original ist values from the tss are only accessed during
* debugging, no special alignment required.
@@ -1231,8 +1229,6 @@ void __cpuinit cpu_init(void)
fpu_init();
xsave_init();
- raw_local_save_flags(kernel_eflags);
-
if (is_uv_system())
uv_cpu_init();
}
diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S
index e13329d..b50d142 100644
--- a/arch/x86/kernel/entry_64.S
+++ b/arch/x86/kernel/entry_64.S
@@ -396,7 +396,7 @@ ENTRY(ret_from_fork)
LOCK ; btr $TIF_FORK,TI_flags(%r8)
- pushq_cfi kernel_eflags(%rip)
+ pushq_cfi $0x0002
popfq_cfi # reset kernel eflags
call schedule_tail # rdi: 'prev' task parameter
--
1.7.2.5
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 2/3] x86: make 64 bit ret_from_fork a little more similar to 32 bit
2011-07-25 9:58 [PATCH 0/3] minor cleanups to EFLAGS initialisation in ret_from_fork Ian Campbell
2011-07-25 10:03 ` [PATCH 1/3] x86: drop unnecessary kernel_eflags variable from 64 bit Ian Campbell
@ 2011-07-25 10:03 ` Ian Campbell
2011-07-25 10:03 ` [PATCH 3/3] x86: ret_from_fork: use symbolic contants for bits in EFLAGS Ian Campbell
2011-07-25 10:19 ` [PATCH 0/3] minor cleanups to EFLAGS initialisation in ret_from_fork Cyrill Gorcunov
3 siblings, 0 replies; 22+ messages in thread
From: Ian Campbell @ 2011-07-25 10:03 UTC (permalink / raw)
To: linux-kernel
Cc: Pekka Enberg, Cyrill Gorcunov, H. Peter Anvin, Andi Kleen,
Ian Campbell, x86
The 64 bit version resets EFLAGS before calling schedule_tail() and
therefore leaves EFLAGS.IF clear. 32 bit resets EFLAGS after calling
schedule_tail() and therefore leaves EFLAGS.IF set. I don't think
there is any practical difference between the two approaches since
interrupts are actually reenabled within schedule_tail
(schedule_tail->finish_task_switch->finish_lock_switch->raw_spin_unlock_irq->...->local_irq_enable)
so arbitrarily pick the 32 bit version and make 64 bit look like that.
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Reviewed-by: Pekka Enberg <penberg@kernel.org>
Acked-by: Cyrill Gorcunov <gorcunov@gmail.com>
Cc: x86@kernel.org
---
arch/x86/kernel/entry_64.S | 7 ++++---
1 files changed, 4 insertions(+), 3 deletions(-)
diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S
index b50d142..9c28bd5 100644
--- a/arch/x86/kernel/entry_64.S
+++ b/arch/x86/kernel/entry_64.S
@@ -394,13 +394,14 @@ END(save_paranoid)
ENTRY(ret_from_fork)
DEFAULT_FRAME
- LOCK ; btr $TIF_FORK,TI_flags(%r8)
- pushq_cfi $0x0002
- popfq_cfi # reset kernel eflags
+ LOCK ; btr $TIF_FORK,TI_flags(%r8)
call schedule_tail # rdi: 'prev' task parameter
+ pushq_cfi $0x0202
+ popfq_cfi # reset kernel eflags
+
GET_THREAD_INFO(%rcx)
RESTORE_REST
--
1.7.2.5
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 3/3] x86: ret_from_fork: use symbolic contants for bits in EFLAGS
2011-07-25 9:58 [PATCH 0/3] minor cleanups to EFLAGS initialisation in ret_from_fork Ian Campbell
2011-07-25 10:03 ` [PATCH 1/3] x86: drop unnecessary kernel_eflags variable from 64 bit Ian Campbell
2011-07-25 10:03 ` [PATCH 2/3] x86: make 64 bit ret_from_fork a little more similar to 32 bit Ian Campbell
@ 2011-07-25 10:03 ` Ian Campbell
2011-07-25 10:19 ` [PATCH 0/3] minor cleanups to EFLAGS initialisation in ret_from_fork Cyrill Gorcunov
3 siblings, 0 replies; 22+ messages in thread
From: Ian Campbell @ 2011-07-25 10:03 UTC (permalink / raw)
To: linux-kernel
Cc: Pekka Enberg, Cyrill Gorcunov, H. Peter Anvin, Andi Kleen,
Ian Campbell, x86
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Reviewed-by: Pekka Enberg <penberg@kernel.org>
Acked-by: Cyrill Gorcunov <gorcunov@gmail.com>
Cc: x86@kernel.org
---
arch/x86/kernel/entry_32.S | 2 +-
arch/x86/kernel/entry_64.S | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/x86/kernel/entry_32.S b/arch/x86/kernel/entry_32.S
index 5c1a9197..f032530 100644
--- a/arch/x86/kernel/entry_32.S
+++ b/arch/x86/kernel/entry_32.S
@@ -303,7 +303,7 @@ ENTRY(ret_from_fork)
call schedule_tail
GET_THREAD_INFO(%ebp)
popl_cfi %eax
- pushl_cfi $0x0202 # Reset kernel eflags
+ pushl_cfi $(X86_EFLAGS_IF|0x2) # Reset kernel eflags
popfl_cfi
jmp syscall_exit
CFI_ENDPROC
diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S
index 9c28bd5..4c76abf 100644
--- a/arch/x86/kernel/entry_64.S
+++ b/arch/x86/kernel/entry_64.S
@@ -399,7 +399,7 @@ ENTRY(ret_from_fork)
call schedule_tail # rdi: 'prev' task parameter
- pushq_cfi $0x0202
+ pushq_cfi $(X86_EFLAGS_IF|0x2)
popfq_cfi # reset kernel eflags
GET_THREAD_INFO(%rcx)
--
1.7.2.5
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 0/3] minor cleanups to EFLAGS initialisation in ret_from_fork
2011-07-25 9:58 [PATCH 0/3] minor cleanups to EFLAGS initialisation in ret_from_fork Ian Campbell
` (2 preceding siblings ...)
2011-07-25 10:03 ` [PATCH 3/3] x86: ret_from_fork: use symbolic contants for bits in EFLAGS Ian Campbell
@ 2011-07-25 10:19 ` Cyrill Gorcunov
2011-07-25 18:20 ` Cyrill Gorcunov
3 siblings, 1 reply; 22+ messages in thread
From: Cyrill Gorcunov @ 2011-07-25 10:19 UTC (permalink / raw)
To: Ian Campbell; +Cc: linux-kernel, Pekka Enberg, H. Peter Anvin, Andi Kleen
On Mon, Jul 25, 2011 at 10:58:03AM +0100, Ian Campbell wrote:
> The following series removes the use of a global kernel_eflags variable
> from the x86_64 ret_from_fork path and (very slightly) merges the 32 and
> 64 bit version of that code path.
>
> kernel_eflags could be made a __read_mostly but actually there is no
> reason to prefer the value at cpu_init() time to a compile time constant
> value for the initial eflags after a fork.
>
> Ian.
>
Thanks, Ian! I think noone against this simplification, Peter, Andi?
Cyrill
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/3] minor cleanups to EFLAGS initialisation in ret_from_fork
2011-07-25 10:19 ` [PATCH 0/3] minor cleanups to EFLAGS initialisation in ret_from_fork Cyrill Gorcunov
@ 2011-07-25 18:20 ` Cyrill Gorcunov
2011-07-25 21:10 ` H. Peter Anvin
0 siblings, 1 reply; 22+ messages in thread
From: Cyrill Gorcunov @ 2011-07-25 18:20 UTC (permalink / raw)
To: Ian Campbell, linux-kernel, Pekka Enberg, H. Peter Anvin,
Andi Kleen, Ingo Molnar
On Mon, Jul 25, 2011 at 02:19:02PM +0400, Cyrill Gorcunov wrote:
> On Mon, Jul 25, 2011 at 10:58:03AM +0100, Ian Campbell wrote:
> > The following series removes the use of a global kernel_eflags variable
> > from the x86_64 ret_from_fork path and (very slightly) merges the 32 and
> > 64 bit version of that code path.
> >
> > kernel_eflags could be made a __read_mostly but actually there is no
> > reason to prefer the value at cpu_init() time to a compile time constant
> > value for the initial eflags after a fork.
> >
> > Ian.
> >
>
> Thanks, Ian! I think noone against this simplification, Peter, Andi?
>
> Cyrill
Ian, I've missed in first place that you've opened IRQs window _before_
schedule_tail() call, ie it's not 1:1 code mapping as it was before.
Note kernel_eflags has IF clear and what we have: the ret_from_fork on
x86-64 happens _only_ inside context_switch call, ie
schedule (sched.c)
...
raw_spin_lock_irq
...
context_switch
switch_to
"jnz ret_from_fork\n\t"
pushq_cfi kernel_eflags(%rip)
popfq_cfi # reset kernel eflags
---> irqs are still disabled
call schedule_tail # rdi: 'prev' task parameter
finish_lock_switch
raw_spin_unlock_irq
I bet raw_spin_lock_irq at the beginning of the schedule() is set
for a reason and such change is not safe. Though I may be missing
something again...
Cyrill
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/3] minor cleanups to EFLAGS initialisation in ret_from_fork
2011-07-25 18:20 ` Cyrill Gorcunov
@ 2011-07-25 21:10 ` H. Peter Anvin
2011-07-25 21:47 ` Cyrill Gorcunov
0 siblings, 1 reply; 22+ messages in thread
From: H. Peter Anvin @ 2011-07-25 21:10 UTC (permalink / raw)
To: Cyrill Gorcunov
Cc: Ian Campbell, linux-kernel, Pekka Enberg, Andi Kleen, Ingo Molnar
On 07/25/2011 11:20 AM, Cyrill Gorcunov wrote:
> On Mon, Jul 25, 2011 at 02:19:02PM +0400, Cyrill Gorcunov wrote:
>> On Mon, Jul 25, 2011 at 10:58:03AM +0100, Ian Campbell wrote:
>>> The following series removes the use of a global kernel_eflags variable
>>> from the x86_64 ret_from_fork path and (very slightly) merges the 32 and
>>> 64 bit version of that code path.
>>>
>>> kernel_eflags could be made a __read_mostly but actually there is no
>>> reason to prefer the value at cpu_init() time to a compile time constant
>>> value for the initial eflags after a fork.
>>>
>>> Ian.
>>>
>>
>> Thanks, Ian! I think noone against this simplification, Peter, Andi?
>>
>> Cyrill
>
> Ian, I've missed in first place that you've opened IRQs window _before_
> schedule_tail() call, ie it's not 1:1 code mapping as it was before.
>
> Note kernel_eflags has IF clear and what we have: the ret_from_fork on
> x86-64 happens _only_ inside context_switch call, ie
>
> schedule (sched.c)
> ...
> raw_spin_lock_irq
> ...
> context_switch
> switch_to
> "jnz ret_from_fork\n\t"
> pushq_cfi kernel_eflags(%rip)
> popfq_cfi # reset kernel eflags
>
> ---> irqs are still disabled
>
> call schedule_tail # rdi: 'prev' task parameter
> finish_lock_switch
> raw_spin_unlock_irq
>
> I bet raw_spin_lock_irq at the beginning of the schedule() is set
> for a reason and such change is not safe. Though I may be missing
> something again...
>
This definitely doesn't look "obviously safe" to me. However, does
anyone see a problem with unconditionally leaving IF disabled even on 32
bits (I haven't traced all the paths yet), i.e. doing the *opposite* of
Ian's patch #2?
-hpa
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/3] minor cleanups to EFLAGS initialisation in ret_from_fork
2011-07-25 21:10 ` H. Peter Anvin
@ 2011-07-25 21:47 ` Cyrill Gorcunov
2011-07-26 14:46 ` Peter Zijlstra
0 siblings, 1 reply; 22+ messages in thread
From: Cyrill Gorcunov @ 2011-07-25 21:47 UTC (permalink / raw)
To: H. Peter Anvin
Cc: Ian Campbell, linux-kernel, Pekka Enberg, Andi Kleen,
Ingo Molnar, Peter Zijlstra
On Mon, Jul 25, 2011 at 02:10:02PM -0700, H. Peter Anvin wrote:
> On 07/25/2011 11:20 AM, Cyrill Gorcunov wrote:
> > On Mon, Jul 25, 2011 at 02:19:02PM +0400, Cyrill Gorcunov wrote:
> >> On Mon, Jul 25, 2011 at 10:58:03AM +0100, Ian Campbell wrote:
> >>> The following series removes the use of a global kernel_eflags variable
> >>> from the x86_64 ret_from_fork path and (very slightly) merges the 32 and
> >>> 64 bit version of that code path.
> >>>
> >>> kernel_eflags could be made a __read_mostly but actually there is no
> >>> reason to prefer the value at cpu_init() time to a compile time constant
> >>> value for the initial eflags after a fork.
> >>>
> >>> Ian.
> >>>
> >>
> >> Thanks, Ian! I think noone against this simplification, Peter, Andi?
> >>
> >> Cyrill
> >
> > Ian, I've missed in first place that you've opened IRQs window _before_
> > schedule_tail() call, ie it's not 1:1 code mapping as it was before.
> >
> > Note kernel_eflags has IF clear and what we have: the ret_from_fork on
> > x86-64 happens _only_ inside context_switch call, ie
> >
> > schedule (sched.c)
> > ...
> > raw_spin_lock_irq
> > ...
> > context_switch
> > switch_to
> > "jnz ret_from_fork\n\t"
> > pushq_cfi kernel_eflags(%rip)
> > popfq_cfi # reset kernel eflags
> >
> > ---> irqs are still disabled
> >
> > call schedule_tail # rdi: 'prev' task parameter
> > finish_lock_switch
> > raw_spin_unlock_irq
> >
> > I bet raw_spin_lock_irq at the beginning of the schedule() is set
> > for a reason and such change is not safe. Though I may be missing
> > something again...
> >
>
> This definitely doesn't look "obviously safe" to me. However, does
> anyone see a problem with unconditionally leaving IF disabled even on 32
> bits (I haven't traced all the paths yet), i.e. doing the *opposite* of
> Ian's patch #2?
>
> -hpa
>
On x86-32 it seems to be similar (not identical in calls though)
copy_thread()
p->thread.ip = (unsigned long)ret_from_fork;
and the task get queued into tasks queue, but later when switch_to
happens irqs are blocked at ret_from_fork call. I better poke PeterZ
here /CC'ed/ ;)
Cyrill
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/3] minor cleanups to EFLAGS initialisation in ret_from_fork
2011-07-25 21:47 ` Cyrill Gorcunov
@ 2011-07-26 14:46 ` Peter Zijlstra
2011-07-26 15:51 ` Ian Campbell
2011-08-10 15:27 ` Ian Campbell
0 siblings, 2 replies; 22+ messages in thread
From: Peter Zijlstra @ 2011-07-26 14:46 UTC (permalink / raw)
To: Cyrill Gorcunov
Cc: H. Peter Anvin, Ian Campbell, linux-kernel, Pekka Enberg,
Andi Kleen, Ingo Molnar
On Tue, 2011-07-26 at 01:47 +0400, Cyrill Gorcunov wrote:
> > > schedule (sched.c)
> > > ...
> > > raw_spin_lock_irq
> > > ...
> > > context_switch
> > > switch_to
> > > "jnz ret_from_fork\n\t"
> > > pushq_cfi kernel_eflags(%rip)
> > > popfq_cfi # reset kernel eflags
> > >
> > > ---> irqs are still disabled
> > >
> > > call schedule_tail # rdi: 'prev' task parameter
> > > finish_lock_switch
> > > raw_spin_unlock_irq
> > >
> > > I bet raw_spin_lock_irq at the beginning of the schedule() is set
> > > for a reason and such change is not safe. Though I may be missing
> > > something again...
> > >
> >
> > This definitely doesn't look "obviously safe" to me. However, does
> > anyone see a problem with unconditionally leaving IF disabled even on 32
> > bits (I haven't traced all the paths yet), i.e. doing the *opposite* of
> > Ian's patch #2?
Right, enabling IRQs there isn't cool, currently there's still
__ARCH_WANT_INTERRUPTS_ON_CTXSW but we're working hard on getting rid of
that nightmare.
There's a number of very subtle things that can go wrong when you enable
interrupts over the context switch.
Leaving IRQs disabled should be the right thing, on x86 we should
_never_ have interrupts enabled there.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/3] minor cleanups to EFLAGS initialisation in ret_from_fork
2011-07-26 14:46 ` Peter Zijlstra
@ 2011-07-26 15:51 ` Ian Campbell
2011-08-10 15:27 ` Ian Campbell
1 sibling, 0 replies; 22+ messages in thread
From: Ian Campbell @ 2011-07-26 15:51 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Cyrill Gorcunov, H. Peter Anvin, linux-kernel, Pekka Enberg,
Andi Kleen, Ingo Molnar
On Tue, 2011-07-26 at 16:46 +0200, Peter Zijlstra wrote:
> On Tue, 2011-07-26 at 01:47 +0400, Cyrill Gorcunov wrote:
> > > > schedule (sched.c)
> > > > ...
> > > > raw_spin_lock_irq
> > > > ...
> > > > context_switch
> > > > switch_to
> > > > "jnz ret_from_fork\n\t"
> > > > pushq_cfi kernel_eflags(%rip)
> > > > popfq_cfi # reset kernel eflags
> > > >
> > > > ---> irqs are still disabled
> > > >
> > > > call schedule_tail # rdi: 'prev' task parameter
> > > > finish_lock_switch
> > > > raw_spin_unlock_irq
> > > >
> > > > I bet raw_spin_lock_irq at the beginning of the schedule() is set
> > > > for a reason and such change is not safe. Though I may be missing
> > > > something again...
> > > >
> > >
> > > This definitely doesn't look "obviously safe" to me. However, does
> > > anyone see a problem with unconditionally leaving IF disabled even on 32
> > > bits (I haven't traced all the paths yet), i.e. doing the *opposite* of
> > > Ian's patch #2?
>
> Right, enabling IRQs there isn't cool, currently there's still
> __ARCH_WANT_INTERRUPTS_ON_CTXSW but we're working hard on getting rid of
> that nightmare.
>
> There's a number of very subtle things that can go wrong when you enable
> interrupts over the context switch.
>
> Leaving IRQs disabled should be the right thing, on x86 we should
> _never_ have interrupts enabled there.
Thanks Peter. I'll flip the (not any longer) "arbitrary" decision in my
#2 patch and repost when I return from debconf/vacation.
Ian.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/3] minor cleanups to EFLAGS initialisation in ret_from_fork
2011-07-26 14:46 ` Peter Zijlstra
2011-07-26 15:51 ` Ian Campbell
@ 2011-08-10 15:27 ` Ian Campbell
2011-08-10 15:35 ` Peter Zijlstra
1 sibling, 1 reply; 22+ messages in thread
From: Ian Campbell @ 2011-08-10 15:27 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Cyrill Gorcunov, H. Peter Anvin, linux-kernel, Pekka Enberg,
Andi Kleen, Ingo Molnar
On Tue, 2011-07-26 at 15:46 +0100, Peter Zijlstra wrote:
> On Tue, 2011-07-26 at 01:47 +0400, Cyrill Gorcunov wrote:
> > > > schedule (sched.c)
> > > > ...
> > > > raw_spin_lock_irq
> > > > ...
> > > > context_switch
> > > > switch_to
> > > > "jnz ret_from_fork\n\t"
> > > > pushq_cfi kernel_eflags(%rip)
> > > > popfq_cfi # reset kernel eflags
> > > >
> > > > ---> irqs are still disabled
> > > >
> > > > call schedule_tail # rdi: 'prev' task parameter
> > > > finish_lock_switch
> > > > raw_spin_unlock_irq
> > > >
> > > > I bet raw_spin_lock_irq at the beginning of the schedule() is set
> > > > for a reason and such change is not safe. Though I may be missing
> > > > something again...
> > > >
> > >
> > > This definitely doesn't look "obviously safe" to me. However, does
> > > anyone see a problem with unconditionally leaving IF disabled even on 32
> > > bits (I haven't traced all the paths yet), i.e. doing the *opposite* of
> > > Ian's patch #2?
>
> Right, enabling IRQs there isn't cool, currently there's still
> __ARCH_WANT_INTERRUPTS_ON_CTXSW but we're working hard on getting rid of
> that nightmare.
>
> There's a number of very subtle things that can go wrong when you enable
> interrupts over the context switch.
>
> Leaving IRQs disabled should be the right thing, on x86 we should
> _never_ have interrupts enabled there.
Just getting back to this after my vac, sorry for the delay.
raw_spin_unlock_irq unconditionally re-enables interrupts so I don't
really see what I've changed since interrupts are enabled by
schedule_tail and I've moved (on 64 bit) the EFLAGS reset to after
schedule_tail, so it should have interrupts enabled at that point
already and so they should remain enabled. Or are you suggesting that
things were already wrong?
However I've switched the order of my second patch anyway, so EFLAGS is
reset to 0x0002 (interrupts disabled) on both 32- and 64-bit before the
call to schedule_tail, since it does seem like the simpler option. I'll
repost shortly.
Ian.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 0/3] minor cleanups to EFLAGS initialisation in ret_from_fork
2011-08-10 15:27 ` Ian Campbell
@ 2011-08-10 15:35 ` Peter Zijlstra
0 siblings, 0 replies; 22+ messages in thread
From: Peter Zijlstra @ 2011-08-10 15:35 UTC (permalink / raw)
To: Ian Campbell
Cc: Cyrill Gorcunov, H. Peter Anvin, linux-kernel, Pekka Enberg,
Andi Kleen, Ingo Molnar
On Wed, 2011-08-10 at 16:27 +0100, Ian Campbell wrote:
> raw_spin_unlock_irq unconditionally re-enables interrupts so I don't
> really see what I've changed since interrupts are enabled by
> schedule_tail and I've moved (on 64 bit) the EFLAGS reset to after
> schedule_tail, so it should have interrupts enabled at that point
> already and so they should remain enabled. Or are you suggesting that
> things were already wrong?
Ah, I thought this was about the EFLAGS state of new tasks which would
get into effect at switch_to(), which is well before we enable
interrupts in finish_lock_switch().
> However I've switched the order of my second patch anyway, so EFLAGS is
> reset to 0x0002 (interrupts disabled) on both 32- and 64-bit before the
> call to schedule_tail, since it does seem like the simpler option. I'll
> repost shortly.
OK.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [Q x86-64] on kernel_eflags
@ 2011-07-05 11:15 Cyrill Gorcunov
2011-07-05 13:00 ` [PATCH 1/3] x86: drop unnecessary kernel_eflags variable from 64 bit Ian Campbell
0 siblings, 1 reply; 22+ messages in thread
From: Cyrill Gorcunov @ 2011-07-05 11:15 UTC (permalink / raw)
To: Ian Campbell
Cc: LKML, Ingo Molnar, Thomas Gleixner, H. Peter Anvin, Brian Gerst,
Jan Beulich, Peter Zijlstra, Frederic Weisbecker
On Tue, Jul 05, 2011 at 12:00:56PM +0100, Ian Campbell wrote:
...
>
> I posted http://lkml.org/lkml/2010/2/9/155 to switch to the hardcoded
> version for 64 bit too. Is it worth my rebasing and reposting that?
I think so.
Cyrill
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/3] x86: drop unnecessary kernel_eflags variable from 64 bit.
2011-07-05 11:15 [Q x86-64] on kernel_eflags Cyrill Gorcunov
@ 2011-07-05 13:00 ` Ian Campbell
2011-07-05 13:28 ` Cyrill Gorcunov
2011-07-06 9:39 ` Pekka Enberg
0 siblings, 2 replies; 22+ messages in thread
From: Ian Campbell @ 2011-07-05 13:00 UTC (permalink / raw)
To: Cyrill Gorcunov
Cc: linux-kernel, Ingo Molnar, Thomas Gleixner, H. Peter Anvin,
Brian Gerst, Jan Beulich, Peter Zijlstra, Frederic Weisbecker,
Ian Campbell, x86
For no reason that I can determine 64 bit x86 saves the current eflags
in cpu_init purely for use in ret_from_fork. The equivalent 32 bit
code simply hard codes 0x0202 as the new EFLAGS which seems safer than
relying on a potentially arbitrary EFLAGS saved during cpu_init.
Original i386 changeset
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=47a5c6fa0e204a2b63309c648bb2fde36836c826
Original x86_64 changset
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=658fdbef66e5e9be79b457edc2cbbb3add840aa9
The only comment in the later indicates that it is following the
former, but not why it differs in this way.
This change makes 64 bit use the same mechanism to setup the initial
EFLAGS on fork. Note that 64 bit resets EFLAGS before calling
schedule_tail() as opposed to 32 bit which calls schedule_tail()
first. Therefore the correct value for EFLAGS has opposite IF
bit. This will be fixed in a subsequent patch.
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: x86@kernel.org
---
arch/x86/kernel/cpu/common.c | 4 ----
arch/x86/kernel/entry_64.S | 2 +-
2 files changed, 1 insertions(+), 5 deletions(-)
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 22a073d..178c3b4 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1077,8 +1077,6 @@ void syscall_init(void)
X86_EFLAGS_TF|X86_EFLAGS_DF|X86_EFLAGS_IF|X86_EFLAGS_IOPL);
}
-unsigned long kernel_eflags;
-
/*
* Copies of the original ist values from the tss are only accessed during
* debugging, no special alignment required.
@@ -1231,8 +1229,6 @@ void __cpuinit cpu_init(void)
fpu_init();
xsave_init();
- raw_local_save_flags(kernel_eflags);
-
if (is_uv_system())
uv_cpu_init();
}
diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S
index e13329d..b50d142 100644
--- a/arch/x86/kernel/entry_64.S
+++ b/arch/x86/kernel/entry_64.S
@@ -396,7 +396,7 @@ ENTRY(ret_from_fork)
LOCK ; btr $TIF_FORK,TI_flags(%r8)
- pushq_cfi kernel_eflags(%rip)
+ pushq_cfi $0x0002
popfq_cfi # reset kernel eflags
call schedule_tail # rdi: 'prev' task parameter
--
1.7.2.5
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 1/3] x86: drop unnecessary kernel_eflags variable from 64 bit.
2011-07-05 13:00 ` [PATCH 1/3] x86: drop unnecessary kernel_eflags variable from 64 bit Ian Campbell
@ 2011-07-05 13:28 ` Cyrill Gorcunov
2011-07-06 9:25 ` Ian Campbell
2011-07-06 9:39 ` Pekka Enberg
1 sibling, 1 reply; 22+ messages in thread
From: Cyrill Gorcunov @ 2011-07-05 13:28 UTC (permalink / raw)
To: Ian Campbell
Cc: linux-kernel, Ingo Molnar, Thomas Gleixner, H. Peter Anvin,
Brian Gerst, Jan Beulich, Peter Zijlstra, Frederic Weisbecker,
x86
On Tue, Jul 05, 2011 at 02:00:12PM +0100, Ian Campbell wrote:
> For no reason that I can determine 64 bit x86 saves the current eflags
> in cpu_init purely for use in ret_from_fork. The equivalent 32 bit
> code simply hard codes 0x0202 as the new EFLAGS which seems safer than
> relying on a potentially arbitrary EFLAGS saved during cpu_init.
>
> Original i386 changeset
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=47a5c6fa0e204a2b63309c648bb2fde36836c826
> Original x86_64 changset
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=658fdbef66e5e9be79b457edc2cbbb3add840aa9
>
> The only comment in the later indicates that it is following the
> former, but not why it differs in this way.
>
> This change makes 64 bit use the same mechanism to setup the initial
> EFLAGS on fork. Note that 64 bit resets EFLAGS before calling
> schedule_tail() as opposed to 32 bit which calls schedule_tail()
> first. Therefore the correct value for EFLAGS has opposite IF
> bit. This will be fixed in a subsequent patch.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: x86@kernel.org
The whole series looks good to me, thanks Ian! I hope I don't
miss anything, so lets wait for more feedback ;)
Cyrill
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/3] x86: drop unnecessary kernel_eflags variable from 64 bit.
2011-07-05 13:28 ` Cyrill Gorcunov
@ 2011-07-06 9:25 ` Ian Campbell
2011-07-06 9:46 ` Cyrill Gorcunov
0 siblings, 1 reply; 22+ messages in thread
From: Ian Campbell @ 2011-07-06 9:25 UTC (permalink / raw)
To: Cyrill Gorcunov
Cc: linux-kernel, Ingo Molnar, Thomas Gleixner, H. Peter Anvin,
Brian Gerst, Jan Beulich, Peter Zijlstra, Frederic Weisbecker,
x86
On Tue, 2011-07-05 at 17:28 +0400, Cyrill Gorcunov wrote:
> On Tue, Jul 05, 2011 at 02:00:12PM +0100, Ian Campbell wrote:
> > For no reason that I can determine 64 bit x86 saves the current eflags
> > in cpu_init purely for use in ret_from_fork. The equivalent 32 bit
> > code simply hard codes 0x0202 as the new EFLAGS which seems safer than
> > relying on a potentially arbitrary EFLAGS saved during cpu_init.
> >
> > Original i386 changeset
> > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=47a5c6fa0e204a2b63309c648bb2fde36836c826
> > Original x86_64 changset
> > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=658fdbef66e5e9be79b457edc2cbbb3add840aa9
> >
> > The only comment in the later indicates that it is following the
> > former, but not why it differs in this way.
> >
> > This change makes 64 bit use the same mechanism to setup the initial
> > EFLAGS on fork. Note that 64 bit resets EFLAGS before calling
> > schedule_tail() as opposed to 32 bit which calls schedule_tail()
> > first. Therefore the correct value for EFLAGS has opposite IF
> > bit. This will be fixed in a subsequent patch.
> >
> > Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> > Cc: x86@kernel.org
>
> The whole series looks good to me, thanks Ian! I hope I don't
> miss anything, so lets wait for more feedback ;)
Thanks. Actually I missed the extern declaration in asm/processor.h so
this updated patch should be used instead:
8<-----------------------------------------------
>From a450bf9e1c6e3f436c652b3e4d63b50c1f9848d4 Mon Sep 17 00:00:00 2001
From: Ian Campbell <ian.campbell@citrix.com>
Date: Wed, 6 Jul 2011 10:24:46 +0100
Subject: [PATCH] x86: drop unnecessary kernel_eflags variable from 64 bit.
For no reason that I can determine 64 bit x86 saves the current eflags
in cpu_init purely for use in ret_from_fork. The equivalent 32 bit
code simply hard codes 0x0202 as the new EFLAGS which seems safer than
relying on a potentially arbitrary EFLAGS saved during cpu_init.
Original i386 changeset
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=47a5c6fa0e204a2b63309c648bb2fde36836c826
Original x86_64 changset
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=658fdbef66e5e9be79b457edc2cbbb3add840aa9
The only comment in the later indicates that it is following the
former, but not why it differs in this way.
This change makes 64 bit use the same mechanism to setup the initial
EFLAGS on fork. Note that 64 bit resets EFLAGS before calling
schedule_tail() as opposed to 32 bit which calls schedule_tail()
first. Therefore the correct value for EFLAGS has opposite IF
bit. This will be fixed in a subsequent patch.
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: x86@kernel.org
---
arch/x86/include/asm/processor.h | 1 -
arch/x86/kernel/cpu/common.c | 4 ----
arch/x86/kernel/entry_64.S | 2 +-
3 files changed, 1 insertions(+), 6 deletions(-)
diff --git a/arch/x86/include/asm/processor.h b/arch/x86/include/asm/processor.h
index 2193715..698af88 100644
--- a/arch/x86/include/asm/processor.h
+++ b/arch/x86/include/asm/processor.h
@@ -398,7 +398,6 @@ DECLARE_INIT_PER_CPU(irq_stack_union);
DECLARE_PER_CPU(char *, irq_stack_ptr);
DECLARE_PER_CPU(unsigned int, irq_count);
-extern unsigned long kernel_eflags;
extern asmlinkage void ignore_sysret(void);
#else /* X86_64 */
#ifdef CONFIG_CC_STACKPROTECTOR
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 22a073d..178c3b4 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1077,8 +1077,6 @@ void syscall_init(void)
X86_EFLAGS_TF|X86_EFLAGS_DF|X86_EFLAGS_IF|X86_EFLAGS_IOPL);
}
-unsigned long kernel_eflags;
-
/*
* Copies of the original ist values from the tss are only accessed during
* debugging, no special alignment required.
@@ -1231,8 +1229,6 @@ void __cpuinit cpu_init(void)
fpu_init();
xsave_init();
- raw_local_save_flags(kernel_eflags);
-
if (is_uv_system())
uv_cpu_init();
}
diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S
index e13329d..b50d142 100644
--- a/arch/x86/kernel/entry_64.S
+++ b/arch/x86/kernel/entry_64.S
@@ -396,7 +396,7 @@ ENTRY(ret_from_fork)
LOCK ; btr $TIF_FORK,TI_flags(%r8)
- pushq_cfi kernel_eflags(%rip)
+ pushq_cfi $0x0002
popfq_cfi # reset kernel eflags
call schedule_tail # rdi: 'prev' task parameter
--
1.7.2.5
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 1/3] x86: drop unnecessary kernel_eflags variable from 64 bit.
2011-07-06 9:25 ` Ian Campbell
@ 2011-07-06 9:46 ` Cyrill Gorcunov
2011-07-06 9:57 ` Ian Campbell
0 siblings, 1 reply; 22+ messages in thread
From: Cyrill Gorcunov @ 2011-07-06 9:46 UTC (permalink / raw)
To: Ian Campbell
Cc: linux-kernel, Ingo Molnar, Thomas Gleixner, H. Peter Anvin,
Brian Gerst, Jan Beulich, Peter Zijlstra, Frederic Weisbecker,
x86
On Wed, Jul 06, 2011 at 10:25:29AM +0100, Ian Campbell wrote:
...
> >
> > The whole series looks good to me, thanks Ian! I hope I don't
> > miss anything, so lets wait for more feedback ;)
>
> Thanks. Actually I missed the extern declaration in asm/processor.h so
> this updated patch should be used instead:
>
Great, thanks Ian. Have you a chance to run this series? ;)
Cyrill
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/3] x86: drop unnecessary kernel_eflags variable from 64 bit.
2011-07-06 9:46 ` Cyrill Gorcunov
@ 2011-07-06 9:57 ` Ian Campbell
2011-07-06 10:12 ` Cyrill Gorcunov
0 siblings, 1 reply; 22+ messages in thread
From: Ian Campbell @ 2011-07-06 9:57 UTC (permalink / raw)
To: Cyrill Gorcunov
Cc: linux-kernel, Ingo Molnar, Thomas Gleixner, H. Peter Anvin,
Brian Gerst, Jan Beulich, Peter Zijlstra, Frederic Weisbecker,
x86
On Wed, 2011-07-06 at 10:46 +0100, Cyrill Gorcunov wrote:
> On Wed, Jul 06, 2011 at 10:25:29AM +0100, Ian Campbell wrote:
> ...
> > >
> > > The whole series looks good to me, thanks Ian! I hope I don't
> > > miss anything, so lets wait for more feedback ;)
> >
> > Thanks. Actually I missed the extern declaration in asm/processor.h so
> > this updated patch should be used instead:
> >
>
> Great, thanks Ian. Have you a chance to run this series? ;)
Yeah I booted it before I posted it. I figured that the initscripts etc
would be a sufficient test of fork() and that if it were wrong it would
fall over pretty quickly and obviously.
Ian.
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/3] x86: drop unnecessary kernel_eflags variable from 64 bit.
2011-07-06 9:57 ` Ian Campbell
@ 2011-07-06 10:12 ` Cyrill Gorcunov
0 siblings, 0 replies; 22+ messages in thread
From: Cyrill Gorcunov @ 2011-07-06 10:12 UTC (permalink / raw)
To: Ian Campbell
Cc: linux-kernel, Ingo Molnar, Thomas Gleixner, H. Peter Anvin,
Brian Gerst, Jan Beulich, Peter Zijlstra, Frederic Weisbecker,
x86
On Wed, Jul 06, 2011 at 10:57:05AM +0100, Ian Campbell wrote:
> On Wed, 2011-07-06 at 10:46 +0100, Cyrill Gorcunov wrote:
> > On Wed, Jul 06, 2011 at 10:25:29AM +0100, Ian Campbell wrote:
> > ...
> > > >
> > > > The whole series looks good to me, thanks Ian! I hope I don't
> > > > miss anything, so lets wait for more feedback ;)
> > >
> > > Thanks. Actually I missed the extern declaration in asm/processor.h so
> > > this updated patch should be used instead:
> > >
> >
> > Great, thanks Ian. Have you a chance to run this series? ;)
>
> Yeah I booted it before I posted it. I figured that the initscripts etc
> would be a sufficient test of fork() and that if it were wrong it would
> fall over pretty quickly and obviously.
>
> Ian.
>
OK, thanks!
Cyrill
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/3] x86: drop unnecessary kernel_eflags variable from 64 bit.
2011-07-05 13:00 ` [PATCH 1/3] x86: drop unnecessary kernel_eflags variable from 64 bit Ian Campbell
2011-07-05 13:28 ` Cyrill Gorcunov
@ 2011-07-06 9:39 ` Pekka Enberg
2011-07-06 23:17 ` Andi Kleen
1 sibling, 1 reply; 22+ messages in thread
From: Pekka Enberg @ 2011-07-06 9:39 UTC (permalink / raw)
To: Ian Campbell
Cc: Cyrill Gorcunov, linux-kernel, Ingo Molnar, Thomas Gleixner,
H. Peter Anvin, Brian Gerst, Jan Beulich, Peter Zijlstra,
Frederic Weisbecker, x86, Andi Kleen, Linus Torvalds
On Tue, Jul 5, 2011 at 4:00 PM, Ian Campbell <ian.campbell@citrix.com> wrote:
> For no reason that I can determine 64 bit x86 saves the current eflags
> in cpu_init purely for use in ret_from_fork. The equivalent 32 bit
> code simply hard codes 0x0202 as the new EFLAGS which seems safer than
> relying on a potentially arbitrary EFLAGS saved during cpu_init.
>
> Original i386 changeset
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=47a5c6fa0e204a2b63309c648bb2fde36836c826
> Original x86_64 changset
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=658fdbef66e5e9be79b457edc2cbbb3add840aa9
>
> The only comment in the later indicates that it is following the
> former, but not why it differs in this way.
Well lets ask Linus and Andi?
> This change makes 64 bit use the same mechanism to setup the initial
> EFLAGS on fork. Note that 64 bit resets EFLAGS before calling
> schedule_tail() as opposed to 32 bit which calls schedule_tail()
> first. Therefore the correct value for EFLAGS has opposite IF
> bit. This will be fixed in a subsequent patch.
>
> Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
> Cc: x86@kernel.org
> ---
> arch/x86/kernel/cpu/common.c | 4 ----
> arch/x86/kernel/entry_64.S | 2 +-
> 2 files changed, 1 insertions(+), 5 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
> index 22a073d..178c3b4 100644
> --- a/arch/x86/kernel/cpu/common.c
> +++ b/arch/x86/kernel/cpu/common.c
> @@ -1077,8 +1077,6 @@ void syscall_init(void)
> X86_EFLAGS_TF|X86_EFLAGS_DF|X86_EFLAGS_IF|X86_EFLAGS_IOPL);
> }
>
> -unsigned long kernel_eflags;
> -
> /*
> * Copies of the original ist values from the tss are only accessed during
> * debugging, no special alignment required.
> @@ -1231,8 +1229,6 @@ void __cpuinit cpu_init(void)
> fpu_init();
> xsave_init();
>
> - raw_local_save_flags(kernel_eflags);
> -
> if (is_uv_system())
> uv_cpu_init();
> }
> diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S
> index e13329d..b50d142 100644
> --- a/arch/x86/kernel/entry_64.S
> +++ b/arch/x86/kernel/entry_64.S
> @@ -396,7 +396,7 @@ ENTRY(ret_from_fork)
>
> LOCK ; btr $TIF_FORK,TI_flags(%r8)
>
> - pushq_cfi kernel_eflags(%rip)
> + pushq_cfi $0x0002
> popfq_cfi # reset kernel eflags
>
> call schedule_tail # rdi: 'prev' task parameter
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/3] x86: drop unnecessary kernel_eflags variable from 64 bit.
2011-07-06 9:39 ` Pekka Enberg
@ 2011-07-06 23:17 ` Andi Kleen
2011-07-07 0:00 ` H. Peter Anvin
0 siblings, 1 reply; 22+ messages in thread
From: Andi Kleen @ 2011-07-06 23:17 UTC (permalink / raw)
To: Pekka Enberg
Cc: Ian Campbell, Cyrill Gorcunov, linux-kernel, Ingo Molnar,
Thomas Gleixner, H. Peter Anvin, Brian Gerst, Jan Beulich,
Peter Zijlstra, Frederic Weisbecker, x86, Linus Torvalds
> Well lets ask Linus and Andi?
The original idea was that if someone extends EFLAGS with new bits
the BIOS could set them. Strictly said the Intel Manual
specifies that ("reserved bits -- always set to values previously read")
But that never really worked out and I don't think anyone
would dare extending it now. So hardcoding is fine I guess.
-Andi
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 1/3] x86: drop unnecessary kernel_eflags variable from 64 bit.
2011-07-06 23:17 ` Andi Kleen
@ 2011-07-07 0:00 ` H. Peter Anvin
0 siblings, 0 replies; 22+ messages in thread
From: H. Peter Anvin @ 2011-07-07 0:00 UTC (permalink / raw)
To: Andi Kleen
Cc: Pekka Enberg, Ian Campbell, Cyrill Gorcunov, linux-kernel,
Ingo Molnar, Thomas Gleixner, Brian Gerst, Jan Beulich,
Peter Zijlstra, Frederic Weisbecker, x86, Linus Torvalds
On 07/06/2011 04:17 PM, Andi Kleen wrote:
>> Well lets ask Linus and Andi?
>
> The original idea was that if someone extends EFLAGS with new bits
> the BIOS could set them. Strictly said the Intel Manual
> specifies that ("reserved bits -- always set to values previously read")
>
> But that never really worked out and I don't think anyone
> would dare extending it now. So hardcoding is fine I guess.
>
We already clear any undefined flags almost as soon as we enter the
kernel... and quite rightly so, because there are BIOSes which leave
random crap in the flags.
It is very different from what an *application* should do in this case,
however.
-hpa
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 0/3] x86: slightly unify ret_from_fork
@ 2010-02-09 15:56 Ian Campbell
2010-02-09 15:56 ` [PATCH 1/3] x86: drop unnecessary kernel_eflags variable from 64 bit Ian Campbell
0 siblings, 1 reply; 22+ messages in thread
From: Ian Campbell @ 2010-02-09 15:56 UTC (permalink / raw)
To: linux-kernel; +Cc: Ian Campbell, x86
I came across some minor differences between the 32 and 64 bit
implementations of ret_from_fork. Although I don't expect this is code
which can realistically be completely unified there seemed to be some
value in making the two look a bit more similar.
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: x86@kernel.org
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/3] x86: drop unnecessary kernel_eflags variable from 64 bit.
2010-02-09 15:56 [PATCH 0/3] x86: slightly unify ret_from_fork Ian Campbell
@ 2010-02-09 15:56 ` Ian Campbell
0 siblings, 0 replies; 22+ messages in thread
From: Ian Campbell @ 2010-02-09 15:56 UTC (permalink / raw)
To: linux-kernel; +Cc: Ian Campbell, x86
For no reason that I can determine 64 bit x86 saves the current eflags
in cpu_init purely for use in ret_from_fork. The equivalent 32 bit
code simply hard codes 0x0202 as the new EFLAGS which seems safer than
relying on a potentially arbitrary EFLAGS saved during cpu_init.
Original i386 changeset
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=47a5c6fa0e204a2b63309c648bb2fde36836c826
Original x86_64 changset
http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=658fdbef66e5e9be79b457edc2cbbb3add840aa9
The only comment in the later indicates that it is following the
former, but not why it differs in this way.
This change makes 64 bit use the same mechanism to setup the initial
EFLAGS on fork. Note that 64 bit resets EFLAGS before calling
schedule_tail() as opposed to 32 bit which calls schedule_tail()
first. Therefore the correct value for EFLAGS has opposite IF
bit. This will be fixed in a subsequent patch.
Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: x86@kernel.org
---
arch/x86/kernel/cpu/common.c | 4 ----
arch/x86/kernel/entry_64.S | 2 +-
2 files changed, 1 insertions(+), 5 deletions(-)
diff --git a/arch/x86/kernel/cpu/common.c b/arch/x86/kernel/cpu/common.c
index 4868e4a..ab766b2 100644
--- a/arch/x86/kernel/cpu/common.c
+++ b/arch/x86/kernel/cpu/common.c
@@ -1040,8 +1040,6 @@ void syscall_init(void)
X86_EFLAGS_TF|X86_EFLAGS_DF|X86_EFLAGS_IF|X86_EFLAGS_IOPL);
}
-unsigned long kernel_eflags;
-
/*
* Copies of the original ist values from the tss are only accessed during
* debugging, no special alignment required.
@@ -1189,8 +1187,6 @@ void __cpuinit cpu_init(void)
fpu_init();
- raw_local_save_flags(kernel_eflags);
-
if (is_uv_system())
uv_cpu_init();
}
diff --git a/arch/x86/kernel/entry_64.S b/arch/x86/kernel/entry_64.S
index 0697ff1..18e4c59 100644
--- a/arch/x86/kernel/entry_64.S
+++ b/arch/x86/kernel/entry_64.S
@@ -398,7 +398,7 @@ ENTRY(ret_from_fork)
LOCK ; btr $TIF_FORK,TI_flags(%r8)
- push kernel_eflags(%rip)
+ push $0x0002
CFI_ADJUST_CFA_OFFSET 8
popf # reset kernel eflags
CFI_ADJUST_CFA_OFFSET -8
--
1.5.6.5
^ permalink raw reply related [flat|nested] 22+ messages in thread
end of thread, other threads:[~2011-08-10 15:35 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-07-25 9:58 [PATCH 0/3] minor cleanups to EFLAGS initialisation in ret_from_fork Ian Campbell
2011-07-25 10:03 ` [PATCH 1/3] x86: drop unnecessary kernel_eflags variable from 64 bit Ian Campbell
2011-07-25 10:03 ` [PATCH 2/3] x86: make 64 bit ret_from_fork a little more similar to 32 bit Ian Campbell
2011-07-25 10:03 ` [PATCH 3/3] x86: ret_from_fork: use symbolic contants for bits in EFLAGS Ian Campbell
2011-07-25 10:19 ` [PATCH 0/3] minor cleanups to EFLAGS initialisation in ret_from_fork Cyrill Gorcunov
2011-07-25 18:20 ` Cyrill Gorcunov
2011-07-25 21:10 ` H. Peter Anvin
2011-07-25 21:47 ` Cyrill Gorcunov
2011-07-26 14:46 ` Peter Zijlstra
2011-07-26 15:51 ` Ian Campbell
2011-08-10 15:27 ` Ian Campbell
2011-08-10 15:35 ` Peter Zijlstra
-- strict thread matches above, loose matches on Subject: below --
2011-07-05 11:15 [Q x86-64] on kernel_eflags Cyrill Gorcunov
2011-07-05 13:00 ` [PATCH 1/3] x86: drop unnecessary kernel_eflags variable from 64 bit Ian Campbell
2011-07-05 13:28 ` Cyrill Gorcunov
2011-07-06 9:25 ` Ian Campbell
2011-07-06 9:46 ` Cyrill Gorcunov
2011-07-06 9:57 ` Ian Campbell
2011-07-06 10:12 ` Cyrill Gorcunov
2011-07-06 9:39 ` Pekka Enberg
2011-07-06 23:17 ` Andi Kleen
2011-07-07 0:00 ` H. Peter Anvin
2010-02-09 15:56 [PATCH 0/3] x86: slightly unify ret_from_fork Ian Campbell
2010-02-09 15:56 ` [PATCH 1/3] x86: drop unnecessary kernel_eflags variable from 64 bit Ian Campbell
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).