Function machine_halt (resp. native_machine_halt) is empty for x86
architectures. When command 'halt -f' is invoked, the message
"System halted." is displayed but this is not really true because
all CPUs are still running.
There are also similar inconsistencies for other arches (some uses
power-off for halt or forever-loop with IRQs enabled/disabled).
IMO there should be used the same approach for all architectures
OR what does the message "System halted" really mean?
Signed-off-by: Ivan Vecera <[email protected]>
---
arch/x86/kernel/reboot.c | 8 ++++++++
1 files changed, 8 insertions(+), 0 deletions(-)
diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
index f4c93f1..15ad949 100644
--- a/arch/x86/kernel/reboot.c
+++ b/arch/x86/kernel/reboot.c
@@ -465,6 +465,14 @@ static void native_machine_restart(char *__unused)
static void native_machine_halt(void)
{
+ /* stop other cpus and apics */
+ machine_shutdown();
+
+ /* stop this cpu */
+ local_irq_disable();
+ if (hlt_works(smp_processor_id()))
+ for (;;) halt();
+ for (;;);
}
static void native_machine_power_off(void)
--
1.5.6.3
On Mon, Oct 20, 2008 at 02:13:07PM +0200, Ivan Vecera wrote:
> Function machine_halt (resp. native_machine_halt) is empty for x86
> architectures. When command 'halt -f' is invoked, the message
> "System halted." is displayed but this is not really true because
> all CPUs are still running.
> There are also similar inconsistencies for other arches (some uses
> power-off for halt or forever-loop with IRQs enabled/disabled).
> IMO there should be used the same approach for all architectures
> OR what does the message "System halted" really mean?
>
> Signed-off-by: Ivan Vecera <[email protected]>
> ---
> arch/x86/kernel/reboot.c | 8 ++++++++
> 1 files changed, 8 insertions(+), 0 deletions(-)
>
> diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
> index f4c93f1..15ad949 100644
> --- a/arch/x86/kernel/reboot.c
> +++ b/arch/x86/kernel/reboot.c
> @@ -465,6 +465,14 @@ static void native_machine_restart(char *__unused)
>
> static void native_machine_halt(void)
> {
> + /* stop other cpus and apics */
> + machine_shutdown();
> +
> + /* stop this cpu */
> + local_irq_disable();
> + if (hlt_works(smp_processor_id()))
> + for (;;) halt();
> + for (;;);
> }
>
> static void native_machine_power_off(void)
> --
> 1.5.6.3
>
Acked-by: Neil Horman <[email protected]>
--
/***************************************************
*Neil Horman
*Senior Software Engineer
*Red Hat, Inc.
*[email protected]
*gpg keyid: 1024D / 0x92A74FA1
*http://pgp.mit.edu
***************************************************/
* Ivan Vecera <[email protected]> wrote:
> Function machine_halt (resp. native_machine_halt) is empty for x86
> architectures. When command 'halt -f' is invoked, the message "System
> halted." is displayed but this is not really true because all CPUs are
> still running. There are also similar inconsistencies for other arches
> (some uses power-off for halt or forever-loop with IRQs
> enabled/disabled). IMO there should be used the same approach for all
> architectures OR what does the message "System halted" really mean?
no fundamental objections, but could you please do it a bit cleaner:
> static void native_machine_halt(void)
> {
> + /* stop other cpus and apics */
> + machine_shutdown();
> +
> + /* stop this cpu */
> + local_irq_disable();
> + if (hlt_works(smp_processor_id()))
> + for (;;) halt();
> + for (;;);
> }
the code in arch/x86/kernel/smp.c::stop_this_cpu() is very similar to
this and could be shared. You could move the stop_this_cpu() function to
arch/x86/kernel/process.c (out of smp.c), so that it can be used by
reboot.c.
furthermore, native_machine_power_off() should probably fall back to
native_machine_halt() as well - should pm_power_off() be disabled (or if
it fails to stop the machine).
hm?
Ingo
Ingo Molnar wrote:
> the code in arch/x86/kernel/smp.c::stop_this_cpu() is very similar to
> this and could be shared. You could move the stop_this_cpu() function to
> arch/x86/kernel/process.c (out of smp.c), so that it can be used by
> reboot.c.
>
Yes, this make sense. Here is the patch.
Ivan
---
arch/x86/kernel/process.c | 16 ++++++++++++++++
arch/x86/kernel/reboot.c | 5 +++++
arch/x86/kernel/smp.c | 13 -------------
include/asm-x86/system.h | 2 ++
4 files changed, 23 insertions(+), 13 deletions(-)
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index c622772..af6904e 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -8,6 +8,7 @@
#include <linux/pm.h>
#include <linux/clockchips.h>
#include <asm/system.h>
+#include <asm/apic.h>
unsigned long idle_halt;
EXPORT_SYMBOL(idle_halt);
@@ -122,6 +123,21 @@ void default_idle(void)
EXPORT_SYMBOL(default_idle);
#endif
+void stop_this_cpu(void *dummy)
+{
+ local_irq_disable();
+ /*
+ * Remove this CPU:
+ */
+ cpu_clear(smp_processor_id(), cpu_online_map);
+#ifdef CONFIG_X86_LOCAL_APIC
+ disable_local_APIC();
+#endif
+ if (hlt_works(smp_processor_id()))
+ for (;;) halt();
+ for (;;);
+}
+
static void do_nothing(void *unused)
{
}
diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
index f4c93f1..3113d9e 100644
--- a/arch/x86/kernel/reboot.c
+++ b/arch/x86/kernel/reboot.c
@@ -465,6 +465,11 @@ static void native_machine_restart(char *__unused)
static void native_machine_halt(void)
{
+ /* stop other cpus and apics */
+ machine_shutdown();
+
+ /* stop this cpu */
+ stop_this_cpu(NULL);
}
static void native_machine_power_off(void)
diff --git a/arch/x86/kernel/smp.c b/arch/x86/kernel/smp.c
index 18f9b19..3f92b13 100644
--- a/arch/x86/kernel/smp.c
+++ b/arch/x86/kernel/smp.c
@@ -140,19 +140,6 @@ void native_send_call_func_ipi(cpumask_t mask)
send_IPI_mask(mask, CALL_FUNCTION_VECTOR);
}
-static void stop_this_cpu(void *dummy)
-{
- local_irq_disable();
- /*
- * Remove this CPU:
- */
- cpu_clear(smp_processor_id(), cpu_online_map);
- disable_local_APIC();
- if (hlt_works(smp_processor_id()))
- for (;;) halt();
- for (;;);
-}
-
/*
* this function calls the 'stop' function on all other CPUs in the system.
*/
diff --git a/include/asm-x86/system.h b/include/asm-x86/system.h
index b20c894..17ac753 100644
--- a/include/asm-x86/system.h
+++ b/include/asm-x86/system.h
@@ -314,6 +314,8 @@ extern void free_init_pages(char *what, unsigned long begin, unsigned long end);
void default_idle(void);
+void stop_this_cpu(void *dummy);
+
/*
* Force strict CPU ordering.
* And yes, this is required on UP too when we're talking
--
1.5.6.3
Any comments?
Ivan
---
Ivan Vecera wrote:
> Ingo Molnar wrote:
>> the code in arch/x86/kernel/smp.c::stop_this_cpu() is very similar to
>> this and could be shared. You could move the stop_this_cpu() function to
>> arch/x86/kernel/process.c (out of smp.c), so that it can be used by
>> reboot.c.
>>
> Yes, this make sense. Here is the patch.
>
> Ivan
>
> ---
> arch/x86/kernel/process.c | 16 ++++++++++++++++
> arch/x86/kernel/reboot.c | 5 +++++
> arch/x86/kernel/smp.c | 13 -------------
> include/asm-x86/system.h | 2 ++
> 4 files changed, 23 insertions(+), 13 deletions(-)
>
> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> index c622772..af6904e 100644
> --- a/arch/x86/kernel/process.c
> +++ b/arch/x86/kernel/process.c
> @@ -8,6 +8,7 @@
> #include <linux/pm.h>
> #include <linux/clockchips.h>
> #include <asm/system.h>
> +#include <asm/apic.h>
>
> unsigned long idle_halt;
> EXPORT_SYMBOL(idle_halt);
> @@ -122,6 +123,21 @@ void default_idle(void)
> EXPORT_SYMBOL(default_idle);
> #endif
>
> +void stop_this_cpu(void *dummy)
> +{
> + local_irq_disable();
> + /*
> + * Remove this CPU:
> + */
> + cpu_clear(smp_processor_id(), cpu_online_map);
> +#ifdef CONFIG_X86_LOCAL_APIC
> + disable_local_APIC();
> +#endif
> + if (hlt_works(smp_processor_id()))
> + for (;;) halt();
> + for (;;);
> +}
> +
> static void do_nothing(void *unused)
> {
> }
> diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
> index f4c93f1..3113d9e 100644
> --- a/arch/x86/kernel/reboot.c
> +++ b/arch/x86/kernel/reboot.c
> @@ -465,6 +465,11 @@ static void native_machine_restart(char *__unused)
>
> static void native_machine_halt(void)
> {
> + /* stop other cpus and apics */
> + machine_shutdown();
> +
> + /* stop this cpu */
> + stop_this_cpu(NULL);
> }
>
> static void native_machine_power_off(void)
> diff --git a/arch/x86/kernel/smp.c b/arch/x86/kernel/smp.c
> index 18f9b19..3f92b13 100644
> --- a/arch/x86/kernel/smp.c
> +++ b/arch/x86/kernel/smp.c
> @@ -140,19 +140,6 @@ void native_send_call_func_ipi(cpumask_t mask)
> send_IPI_mask(mask, CALL_FUNCTION_VECTOR);
> }
>
> -static void stop_this_cpu(void *dummy)
> -{
> - local_irq_disable();
> - /*
> - * Remove this CPU:
> - */
> - cpu_clear(smp_processor_id(), cpu_online_map);
> - disable_local_APIC();
> - if (hlt_works(smp_processor_id()))
> - for (;;) halt();
> - for (;;);
> -}
> -
> /*
> * this function calls the 'stop' function on all other CPUs in the system.
> */
> diff --git a/include/asm-x86/system.h b/include/asm-x86/system.h
> index b20c894..17ac753 100644
> --- a/include/asm-x86/system.h
> +++ b/include/asm-x86/system.h
> @@ -314,6 +314,8 @@ extern void free_init_pages(char *what, unsigned long begin, unsigned long end);
>
> void default_idle(void);
>
> +void stop_this_cpu(void *dummy);
> +
> /*
> * Force strict CPU ordering.
> * And yes, this is required on UP too when we're talking
On Thu, Nov 06, 2008 at 05:10:01PM +0100, Ivan Vecera wrote:
> Any comments?
>
> Ivan
>
Acked-by: Neil Horman <[email protected]>
> ---
> Ivan Vecera wrote:
> > Ingo Molnar wrote:
> >> the code in arch/x86/kernel/smp.c::stop_this_cpu() is very similar to
> >> this and could be shared. You could move the stop_this_cpu() function to
> >> arch/x86/kernel/process.c (out of smp.c), so that it can be used by
> >> reboot.c.
> >>
> > Yes, this make sense. Here is the patch.
> >
> > Ivan
> >
> > ---
> > arch/x86/kernel/process.c | 16 ++++++++++++++++
> > arch/x86/kernel/reboot.c | 5 +++++
> > arch/x86/kernel/smp.c | 13 -------------
> > include/asm-x86/system.h | 2 ++
> > 4 files changed, 23 insertions(+), 13 deletions(-)
> >
> > diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> > index c622772..af6904e 100644
> > --- a/arch/x86/kernel/process.c
> > +++ b/arch/x86/kernel/process.c
> > @@ -8,6 +8,7 @@
> > #include <linux/pm.h>
> > #include <linux/clockchips.h>
> > #include <asm/system.h>
> > +#include <asm/apic.h>
> >
> > unsigned long idle_halt;
> > EXPORT_SYMBOL(idle_halt);
> > @@ -122,6 +123,21 @@ void default_idle(void)
> > EXPORT_SYMBOL(default_idle);
> > #endif
> >
> > +void stop_this_cpu(void *dummy)
> > +{
> > + local_irq_disable();
> > + /*
> > + * Remove this CPU:
> > + */
> > + cpu_clear(smp_processor_id(), cpu_online_map);
> > +#ifdef CONFIG_X86_LOCAL_APIC
> > + disable_local_APIC();
> > +#endif
> > + if (hlt_works(smp_processor_id()))
> > + for (;;) halt();
> > + for (;;);
> > +}
> > +
> > static void do_nothing(void *unused)
> > {
> > }
> > diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
> > index f4c93f1..3113d9e 100644
> > --- a/arch/x86/kernel/reboot.c
> > +++ b/arch/x86/kernel/reboot.c
> > @@ -465,6 +465,11 @@ static void native_machine_restart(char *__unused)
> >
> > static void native_machine_halt(void)
> > {
> > + /* stop other cpus and apics */
> > + machine_shutdown();
> > +
> > + /* stop this cpu */
> > + stop_this_cpu(NULL);
> > }
> >
> > static void native_machine_power_off(void)
> > diff --git a/arch/x86/kernel/smp.c b/arch/x86/kernel/smp.c
> > index 18f9b19..3f92b13 100644
> > --- a/arch/x86/kernel/smp.c
> > +++ b/arch/x86/kernel/smp.c
> > @@ -140,19 +140,6 @@ void native_send_call_func_ipi(cpumask_t mask)
> > send_IPI_mask(mask, CALL_FUNCTION_VECTOR);
> > }
> >
> > -static void stop_this_cpu(void *dummy)
> > -{
> > - local_irq_disable();
> > - /*
> > - * Remove this CPU:
> > - */
> > - cpu_clear(smp_processor_id(), cpu_online_map);
> > - disable_local_APIC();
> > - if (hlt_works(smp_processor_id()))
> > - for (;;) halt();
> > - for (;;);
> > -}
> > -
> > /*
> > * this function calls the 'stop' function on all other CPUs in the system.
> > */
> > diff --git a/include/asm-x86/system.h b/include/asm-x86/system.h
> > index b20c894..17ac753 100644
> > --- a/include/asm-x86/system.h
> > +++ b/include/asm-x86/system.h
> > @@ -314,6 +314,8 @@ extern void free_init_pages(char *what, unsigned long begin, unsigned long end);
> >
> > void default_idle(void);
> >
> > +void stop_this_cpu(void *dummy);
> > +
> > /*
> > * Force strict CPU ordering.
> > * And yes, this is required on UP too when we're talking
>
--
/***************************************************
*Neil Horman
*Senior Software Engineer
*Red Hat, Inc.
*[email protected]
*gpg keyid: 1024D / 0x92A74FA1
*http://pgp.mit.edu
***************************************************/
* Ivan Vecera <[email protected]> wrote:
> Ingo Molnar wrote:
> > the code in arch/x86/kernel/smp.c::stop_this_cpu() is very similar to
> > this and could be shared. You could move the stop_this_cpu() function to
> > arch/x86/kernel/process.c (out of smp.c), so that it can be used by
> > reboot.c.
> >
> Yes, this make sense. Here is the patch.
looks good. One small detail:
> +#ifdef CONFIG_X86_LOCAL_APIC
> + disable_local_APIC();
> +#endif
could you please avoid this #ifdef by putting an inline void function
for disable_local_APIC() into arch/x86/include/asm/apic.h for the
!CONFIG_X86_LOCAL_APIC case?
Ingo
On Thu 2008-11-06 17:10:01, Ivan Vecera wrote:
> Any comments?
>
> Ivan
>
> ---
> Ivan Vecera wrote:
> > Ingo Molnar wrote:
> >> the code in arch/x86/kernel/smp.c::stop_this_cpu() is very similar to
> >> this and could be shared. You could move the stop_this_cpu() function to
> >> arch/x86/kernel/process.c (out of smp.c), so that it can be used by
> >> reboot.c.
> >>
> > Yes, this make sense. Here is the patch.
> >
> > Ivan
> >
> > ---
> > arch/x86/kernel/process.c | 16 ++++++++++++++++
> > arch/x86/kernel/reboot.c | 5 +++++
> > arch/x86/kernel/smp.c | 13 -------------
> > include/asm-x86/system.h | 2 ++
> > 4 files changed, 23 insertions(+), 13 deletions(-)
> >
> > diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> > index c622772..af6904e 100644
> > --- a/arch/x86/kernel/process.c
> > +++ b/arch/x86/kernel/process.c
> > @@ -8,6 +8,7 @@
> > #include <linux/pm.h>
> > #include <linux/clockchips.h>
> > #include <asm/system.h>
> > +#include <asm/apic.h>
> >
> > unsigned long idle_halt;
> > EXPORT_SYMBOL(idle_halt);
> > @@ -122,6 +123,21 @@ void default_idle(void)
> > EXPORT_SYMBOL(default_idle);
> > #endif
> >
> > +void stop_this_cpu(void *dummy)
> > +{
> > + local_irq_disable();
> > + /*
> > + * Remove this CPU:
> > + */
> > + cpu_clear(smp_processor_id(), cpu_online_map);
> > +#ifdef CONFIG_X86_LOCAL_APIC
> > + disable_local_APIC();
> > +#endif
> > + if (hlt_works(smp_processor_id()))
> > + for (;;) halt();
> > + for (;;);
> > +}
Why the new ifdef? And while we are at it, why is it neccessary to
disable APIC for stopping CPU? (comment in code would be nice)
> > -static void stop_this_cpu(void *dummy)
> > -{
> > - local_irq_disable();
> > - /*
> > - * Remove this CPU:
> > - */
> > - cpu_clear(smp_processor_id(), cpu_online_map);
> > - disable_local_APIC();
> > - if (hlt_works(smp_processor_id()))
> > - for (;;) halt();
> > - for (;;);
> > -}
> > -
> > /*
> > * this function calls the 'stop' function on all other CPUs in the system.
> > */
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Ingo Molnar wrote:
> looks good. One small detail:
>
>> +#ifdef CONFIG_X86_LOCAL_APIC
>> + disable_local_APIC();
>> +#endif
>
> could you please avoid this #ifdef by putting an inline void function
> for disable_local_APIC() into arch/x86/include/asm/apic.h for the
> !CONFIG_X86_LOCAL_APIC case?
>
> Ingo
OK, here is the result.
Ivan
---
arch/x86/include/asm/apic.h | 1 +
arch/x86/include/asm/system.h | 2 ++
arch/x86/kernel/process.c | 14 ++++++++++++++
arch/x86/kernel/reboot.c | 5 +++++
arch/x86/kernel/smp.c | 13 -------------
5 files changed, 22 insertions(+), 13 deletions(-)
diff --git a/arch/x86/include/asm/apic.h b/arch/x86/include/asm/apic.h
index 3b1510b..25caa07 100644
--- a/arch/x86/include/asm/apic.h
+++ b/arch/x86/include/asm/apic.h
@@ -193,6 +193,7 @@ extern u8 setup_APIC_eilvt_ibs(u8 vector, u8 msg_type, u8 mask);
static inline void lapic_shutdown(void) { }
#define local_apic_timer_c2_ok 1
static inline void init_apic_mappings(void) { }
+static inline void disable_local_APIC(void) { }
#endif /* !CONFIG_X86_LOCAL_APIC */
diff --git a/arch/x86/include/asm/system.h b/arch/x86/include/asm/system.h
index 2ed3f0f..07c3e40 100644
--- a/arch/x86/include/asm/system.h
+++ b/arch/x86/include/asm/system.h
@@ -314,6 +314,8 @@ extern void free_init_pages(char *what, unsigned long begin, unsigned long end);
void default_idle(void);
+void stop_this_cpu(void *dummy);
+
/*
* Force strict CPU ordering.
* And yes, this is required on UP too when we're talking
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index c622772..3380458 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -8,6 +8,7 @@
#include <linux/pm.h>
#include <linux/clockchips.h>
#include <asm/system.h>
+#include <asm/apic.h>
unsigned long idle_halt;
EXPORT_SYMBOL(idle_halt);
@@ -122,6 +123,19 @@ void default_idle(void)
EXPORT_SYMBOL(default_idle);
#endif
+void stop_this_cpu(void *dummy)
+{
+ local_irq_disable();
+ /*
+ * Remove this CPU:
+ */
+ cpu_clear(smp_processor_id(), cpu_online_map);
+ disable_local_APIC();
+ if (hlt_works(smp_processor_id()))
+ for (;;) halt();
+ for (;;);
+}
+
static void do_nothing(void *unused)
{
}
diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
index 724adfc..34f8d37 100644
--- a/arch/x86/kernel/reboot.c
+++ b/arch/x86/kernel/reboot.c
@@ -461,6 +461,11 @@ static void native_machine_restart(char *__unused)
static void native_machine_halt(void)
{
+ /* stop other cpus and apics */
+ machine_shutdown();
+
+ /* stop this cpu */
+ stop_this_cpu(NULL);
}
static void native_machine_power_off(void)
diff --git a/arch/x86/kernel/smp.c b/arch/x86/kernel/smp.c
index 18f9b19..3f92b13 100644
--- a/arch/x86/kernel/smp.c
+++ b/arch/x86/kernel/smp.c
@@ -140,19 +140,6 @@ void native_send_call_func_ipi(cpumask_t mask)
send_IPI_mask(mask, CALL_FUNCTION_VECTOR);
}
-static void stop_this_cpu(void *dummy)
-{
- local_irq_disable();
- /*
- * Remove this CPU:
- */
- cpu_clear(smp_processor_id(), cpu_online_map);
- disable_local_APIC();
- if (hlt_works(smp_processor_id()))
- for (;;) halt();
- for (;;);
-}
-
/*
* this function calls the 'stop' function on all other CPUs in the system.
*/
--
1.5.6.3
Pavel Machek wrote:
> On Thu 2008-11-06 17:10:01, Ivan Vecera wrote:
>> Any comments?
>>
>> Ivan
>>
>> ---
>> Ivan Vecera wrote:
>>> Ingo Molnar wrote:
>>>> the code in arch/x86/kernel/smp.c::stop_this_cpu() is very similar to
>>>> this and could be shared. You could move the stop_this_cpu() function to
>>>> arch/x86/kernel/process.c (out of smp.c), so that it can be used by
>>>> reboot.c.
>>>>
>>> Yes, this make sense. Here is the patch.
>>>
>>> Ivan
>>>
>>> ---
>>> arch/x86/kernel/process.c | 16 ++++++++++++++++
>>> arch/x86/kernel/reboot.c | 5 +++++
>>> arch/x86/kernel/smp.c | 13 -------------
>>> include/asm-x86/system.h | 2 ++
>>> 4 files changed, 23 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
>>> index c622772..af6904e 100644
>>> --- a/arch/x86/kernel/process.c
>>> +++ b/arch/x86/kernel/process.c
>>> @@ -8,6 +8,7 @@
>>> #include <linux/pm.h>
>>> #include <linux/clockchips.h>
>>> #include <asm/system.h>
>>> +#include <asm/apic.h>
>>>
>>> unsigned long idle_halt;
>>> EXPORT_SYMBOL(idle_halt);
>>> @@ -122,6 +123,21 @@ void default_idle(void)
>>> EXPORT_SYMBOL(default_idle);
>>> #endif
>>>
>>> +void stop_this_cpu(void *dummy)
>>> +{
>>> + local_irq_disable();
>>> + /*
>>> + * Remove this CPU:
>>> + */
>>> + cpu_clear(smp_processor_id(), cpu_online_map);
>>> +#ifdef CONFIG_X86_LOCAL_APIC
>>> + disable_local_APIC();
>>> +#endif
>>> + if (hlt_works(smp_processor_id()))
>>> + for (;;) halt();
>>> + for (;;);
>>> +}
>
> Why the new ifdef? And while we are at it, why is it neccessary to
> disable APIC for stopping CPU? (comment in code would be nice)
Because stop_this_cpu is shared between native_machine_halt and
native_smp_send_stop. It was only moved from smp.c to process.c.
native_machine_halt doesn't require disable APIC for stopping the
current CPU because at this time is already disabled.
The ifdef is another thing and I sent already corrected patch.
>
>>> -static void stop_this_cpu(void *dummy)
>>> -{
>>> - local_irq_disable();
>>> - /*
>>> - * Remove this CPU:
>>> - */
>>> - cpu_clear(smp_processor_id(), cpu_online_map);
>>> - disable_local_APIC();
>>> - if (hlt_works(smp_processor_id()))
>>> - for (;;) halt();
>>> - for (;;);
>>> -}
>>> -
>>> /*
>>> * this function calls the 'stop' function on all other CPUs in the system.
>>> */
>
* Ivan Vecera <[email protected]> wrote:
> Ingo Molnar wrote:
> > looks good. One small detail:
> >
> >> +#ifdef CONFIG_X86_LOCAL_APIC
> >> + disable_local_APIC();
> >> +#endif
> >
> > could you please avoid this #ifdef by putting an inline void function
> > for disable_local_APIC() into arch/x86/include/asm/apic.h for the
> > !CONFIG_X86_LOCAL_APIC case?
> >
> > Ingo
> OK, here is the result.
>
> Ivan
>
> ---
> arch/x86/include/asm/apic.h | 1 +
> arch/x86/include/asm/system.h | 2 ++
> arch/x86/kernel/process.c | 14 ++++++++++++++
> arch/x86/kernel/reboot.c | 5 +++++
> arch/x86/kernel/smp.c | 13 -------------
> 5 files changed, 22 insertions(+), 13 deletions(-)
applied to tip/x86/reboot, thanks Ivan!
Ingo