From: Heiko Carstens <[email protected]>
Just copy the first 512 read-only bytes of the current cpu lowcore if
a new cpu gets onlined. The rest is zeroed out and must be explicitly
initialized. Current code just copies the entire lowcore and
initializes the needed fields.
This should reveal bugs in future enhancements quite early.
Also when the lowcore of the first cpu is replaced this is now done
atomically (no interrupts, no machine checks).
Signed-off-by: Heiko Carstens <[email protected]>
Signed-off-by: Martin Schwidefsky <[email protected]>
---
arch/s390/kernel/smp.c | 53 +++++++++++++++++++++++++++++++++++--------------
1 file changed, 38 insertions(+), 15 deletions(-)
Index: quilt-2.6/arch/s390/kernel/smp.c
===================================================================
--- quilt-2.6.orig/arch/s390/kernel/smp.c
+++ quilt-2.6/arch/s390/kernel/smp.c
@@ -626,13 +626,17 @@ static int __cpuinit smp_alloc_lowcore(i
if (!lowcore)
return -ENOMEM;
async_stack = __get_free_pages(GFP_KERNEL, ASYNC_ORDER);
- if (!async_stack)
- goto out_async_stack;
panic_stack = __get_free_page(GFP_KERNEL);
- if (!panic_stack)
- goto out_panic_stack;
-
- *lowcore = S390_lowcore;
+ if (!panic_stack || !async_stack)
+ goto out;
+ /*
+ * Only need to copy the first 512 bytes from address 0. But since
+ * the compiler emits a warning if src == NULL for memcpy use copy_page
+ * instead. Copies more than needed but this code is not performance
+ * critical.
+ */
+ copy_page(lowcore, &S390_lowcore);
+ memset((void *)lowcore + 512, 0, sizeof(*lowcore) - 512);
lowcore->async_stack = async_stack + ASYNC_SIZE;
lowcore->panic_stack = panic_stack + PAGE_SIZE;
@@ -653,9 +657,8 @@ static int __cpuinit smp_alloc_lowcore(i
out_save_area:
free_page(panic_stack);
#endif
-out_panic_stack:
+out:
free_pages(async_stack, ASYNC_ORDER);
-out_async_stack:
free_pages((unsigned long) lowcore, lc_order);
return -ENOMEM;
}
@@ -719,8 +722,8 @@ int __cpuinit __cpu_up(unsigned int cpu)
cpu_lowcore->percpu_offset = __per_cpu_offset[cpu];
cpu_lowcore->current_task = (unsigned long) idle;
cpu_lowcore->cpu_data.cpu_nr = cpu;
- cpu_lowcore->softirq_pending = 0;
- cpu_lowcore->ext_call_fast = 0;
+ cpu_lowcore->kernel_asce = S390_lowcore.kernel_asce;
+ cpu_lowcore->ipl_device = S390_lowcore.ipl_device;
eieio();
while (signal_processor(cpu, sigp_restart) == sigp_busy)
@@ -797,23 +800,43 @@ void cpu_die(void)
void __init smp_prepare_cpus(unsigned int max_cpus)
{
+#ifndef CONFIG_64BIT
+ unsigned long save_area = 0;
+#endif
+ unsigned long async_stack, panic_stack;
+ struct _lowcore *lowcore;
unsigned int cpu;
+ int lc_order;
smp_detect_cpus();
/* request the 0x1201 emergency signal external interrupt */
if (register_external_interrupt(0x1201, do_ext_call_interrupt) != 0)
panic("Couldn't request external interrupt 0x1201");
- memset(lowcore_ptr, 0, sizeof(lowcore_ptr));
print_cpu_info(&S390_lowcore.cpu_data);
- smp_alloc_lowcore(smp_processor_id());
+ /* Reallocate current lowcore, but keep its contents. */
+ lc_order = sizeof(long) == 8 ? 1 : 0;
+ lowcore = (void *) __get_free_pages(GFP_KERNEL | GFP_DMA, lc_order);
+ panic_stack = __get_free_page(GFP_KERNEL);
+ async_stack = __get_free_pages(GFP_KERNEL, ASYNC_ORDER);
#ifndef CONFIG_64BIT
if (MACHINE_HAS_IEEE)
- ctl_set_bit(14, 29); /* enable extended save area */
+ save_area = get_zeroed_page(GFP_KERNEL);
#endif
- set_prefix((u32)(unsigned long) lowcore_ptr[smp_processor_id()]);
-
+ local_irq_disable();
+ local_mcck_disable();
+ lowcore_ptr[smp_processor_id()] = lowcore;
+ *lowcore = S390_lowcore;
+ lowcore->panic_stack = panic_stack + PAGE_SIZE;
+ lowcore->async_stack = async_stack + ASYNC_SIZE;
+#ifndef CONFIG_64BIT
+ if (MACHINE_HAS_IEEE)
+ lowcore->extended_save_area_addr = (u32) save_area;
+#endif
+ set_prefix((u32)(unsigned long) lowcore);
+ local_mcck_enable();
+ local_irq_enable();
for_each_possible_cpu(cpu)
if (cpu != smp_processor_id())
smp_create_idle(cpu);
--
blue skies,
Martin.
"Reality continues to ruin my life." - Calvin.
On Tue, Feb 19, 2008 at 03:40:54PM +0100, Martin Schwidefsky wrote:
> + /*
> + * Only need to copy the first 512 bytes from address 0. But since
> + * the compiler emits a warning if src == NULL for memcpy use copy_page
> + * instead. Copies more than needed but this code is not performance
> + * critical.
> + */
> + copy_page(lowcore, &S390_lowcore);
Boah, workaround alert. Why do you not fix the compiler?
Bastian
--
We fight only when there is no other choice. We prefer the ways of
peaceful contact.
-- Kirk, "Spectre of the Gun", stardate 4385.3
On Tue, Feb 19, 2008 at 04:13:55PM +0100, Bastian Blank wrote:
> On Tue, Feb 19, 2008 at 03:40:54PM +0100, Martin Schwidefsky wrote:
> > + /*
> > + * Only need to copy the first 512 bytes from address 0. But since
> > + * the compiler emits a warning if src == NULL for memcpy use copy_page
> > + * instead. Copies more than needed but this code is not performance
> > + * critical.
> > + */
> > + copy_page(lowcore, &S390_lowcore);
>
> Boah, workaround alert. Why do you not fix the compiler?
We need to copy from address 0 (that's where the lowcore resides). But gcc
insists to complain if memcpy is used with src == NULL.. Now what?
On Tue, Feb 19, 2008 at 04:38:56PM +0100, Heiko Carstens wrote:
> On Tue, Feb 19, 2008 at 04:13:55PM +0100, Bastian Blank wrote:
> > On Tue, Feb 19, 2008 at 03:40:54PM +0100, Martin Schwidefsky wrote:
> > > + /*
> > > + * Only need to copy the first 512 bytes from address 0. But since
> > > + * the compiler emits a warning if src == NULL for memcpy use copy_page
> > > + * instead. Copies more than needed but this code is not performance
> > > + * critical.
> > > + */
> > > + copy_page(lowcore, &S390_lowcore);
> >
> > Boah, workaround alert. Why do you not fix the compiler?
>
> We need to copy from address 0 (that's where the lowcore resides). But gcc
> insists to complain if memcpy is used with src == NULL.. Now what?
Erm sorry, misread your question. Usually it's a bug to use memcpy with
src == NULL. In this case it's ok. So I think it's perfectly ok if gcc
emits a warning.
If you know of a better way to get around this please let me know.
>>>> + /*
>>>> + * Only need to copy the first 512 bytes from address 0. But since
>>>> + * the compiler emits a warning if src == NULL for memcpy use
>>>> copy_page
>>>> + * instead. Copies more than needed but this code is not
>>>> performance
>>>> + * critical.
>>>> + */
>>>> + copy_page(lowcore, &S390_lowcore);
>>>
>>> Boah, workaround alert. Why do you not fix the compiler?
>>
>> We need to copy from address 0 (that's where the lowcore resides).
>> But gcc
>> insists to complain if memcpy is used with src == NULL.. Now what?
>
> Erm sorry, misread your question. Usually it's a bug to use memcpy with
> src == NULL. In this case it's ok. So I think it's perfectly ok if gcc
> emits a warning.
> If you know of a better way to get around this please let me know.
-ffreestanding or -Wno-nonnull?
Segher
>>>> Boah, workaround alert. Why do you not fix the compiler?
>>>
>>> We need to copy from address 0 (that's where the lowcore resides). But
>>> gcc
>>> insists to complain if memcpy is used with src == NULL.. Now what?
>>
>> Erm sorry, misread your question. Usually it's a bug to use memcpy with
>> src == NULL. In this case it's ok. So I think it's perfectly ok if gcc
>> emits a warning.
>> If you know of a better way to get around this please let me know.
>
> -ffreestanding or -Wno-nonnull?
Ok, how about the patch below? Everybody happy with it?
---
arch/s390/kernel/Makefile | 5 +++++
arch/s390/kernel/smp.c | 8 +-------
2 files changed, 6 insertions(+), 7 deletions(-)
Index: linux-2.5/arch/s390/kernel/Makefile
===================================================================
--- linux-2.5.orig/arch/s390/kernel/Makefile
+++ linux-2.5/arch/s390/kernel/Makefile
@@ -4,6 +4,11 @@
EXTRA_AFLAGS := -traditional
+#
+# Passing null pointers is ok for smp code, since we access the lowcore here.
+#
+CFLAGS_smp.o := -Wno-nonnull
+
obj-y := bitmap.o traps.o time.o process.o base.o early.o \
setup.o sys_s390.o ptrace.o signal.o cpcmd.o ebcdic.o \
semaphore.o s390_ext.o debug.o irq.o ipl.o dis.o diag.o
Index: linux-2.5/arch/s390/kernel/smp.c
===================================================================
--- linux-2.5.orig/arch/s390/kernel/smp.c
+++ linux-2.5/arch/s390/kernel/smp.c
@@ -631,13 +631,7 @@ static int __cpuinit smp_alloc_lowcore(i
panic_stack = __get_free_page(GFP_KERNEL);
if (!panic_stack || !async_stack)
goto out;
- /*
- * Only need to copy the first 512 bytes from address 0. But since
- * the compiler emits a warning if src == NULL for memcpy use copy_page
- * instead. Copies more than needed but this code is not performance
- * critical.
- */
- copy_page(lowcore, &S390_lowcore);
+ memcpy(lowcore, &S390_lowcore, 512);
memset((void *)lowcore + 512, 0, sizeof(*lowcore) - 512);
lowcore->async_stack = async_stack + ASYNC_SIZE;
lowcore->panic_stack = panic_stack + PAGE_SIZE;
On Wed, Feb 20, 2008 at 10:45:52AM +0100, Heiko Carstens wrote:
> - copy_page(lowcore, &S390_lowcore);
> + memcpy(lowcore, &S390_lowcore, 512);
Okay
> memset((void *)lowcore + 512, 0, sizeof(*lowcore) - 512);
Not completely okay. void pointer are not allowed in arithmetic. gcc
handles void * as char * in this case, but I think it should usualy be
avoided.
Bastian
--
Peace was the way.
-- Kirk, "The City on the Edge of Forever", stardate unknown
On Wed, Feb 20, 2008 at 11:09:33AM +0100, Bastian Blank wrote:
> On Wed, Feb 20, 2008 at 10:45:52AM +0100, Heiko Carstens wrote:
> > - copy_page(lowcore, &S390_lowcore);
> > + memcpy(lowcore, &S390_lowcore, 512);
>
> Okay
>
> > memset((void *)lowcore + 512, 0, sizeof(*lowcore) - 512);
>
> Not completely okay. void pointer are not allowed in arithmetic. gcc
> handles void * as char * in this case, but I think it should usualy be
> avoided.
There are many places all over the kernel that assume sizeof(void) == 1.
That's yet another gcc extension we use... but I'm going to change that
to a char * cast anyway.
Thanks for commenting!