Re: [PATCH] fix broken x86 SMP boot sequence

[James Bottomley]

On Wed, 2006-02-22 at 10:08 -0800, Zachary Amsden wrote:
> I'm not adverse to making the gdt descriptors part of the per-cpu data.  

If you can think of another way to fix the bug, I'm open to it.

> But I think your patch is missing some necessary changes to 
> include/asm-i386/desc.h - what do you plan to do with the following 
> definition there?
> 
> extern struct Xgt_desc_struct idt_descr, cpu_gdt_descr[NR_CPUS];

Nothing ... it predates your patch ... it's not much use, but just in
case someone wants to get at the boot cpu gdt descriptors.

> Also, it seems likely you may have broken APM and/or PnP BIOS.

I don't think so, how? ... APM alters the GDT for the APM bios long
after cpu_init() is called, as does drivers/pnp/pnpbios.

> Can't you get rid of this ugly hack _and_ optimize the code at the same 
> time?  I don't understand the details of voyager bringup, but it seems 
> clear that the BSP or node 0 is special in both sub-architectures.  If 
> it is special anyway, the conditional logic can be merged, and better 
> yet, the allocation for the first caller of cpu_init() can skip the 
> allocation entirely - it can simply use the boot GDT, which is already 
> page-aligned and ready to go.  This gets rid of the 
> alloc_bootmem_pages() piece of the hack above.

Not without repeating your bug.  In the original code the gdt runs in
the boot area until cpu_init() where it switches to the original per_cpu
for the descriptor.  Your patch moved the boot CPU to using the boot
area after cpu_init(), which means subsequent boot cpu gdt changes alter
that area before the secondaries have come up (also using it).

If we have to move to individual pages for descriptors, then every CPU
needs one.

> Also, it seems you left the allocation of the GDT in do_boot_cpu().  How 
> do you plan to make sure that GDT allocation is compatible with hotplug 
> CPU startup?  I was worried about that, which is why I moved the GDT 
> allocation for secondary processors (originally in 
> arch/i386/kernel/cpu/common.c) to smpboot.c.

No, this piece of the patch:

diff --git a/arch/i386/kernel/smpboot.c b/arch/i386/kernel/smpboot.c
index b3c2e2c..9ed449a 100644
--- a/arch/i386/kernel/smpboot.c
+++ b/arch/i386/kernel/smpboot.c
@@ -903,12 +903,6 @@ static int __devinit do_boot_cpu(int api
        unsigned long start_eip;
        unsigned short nmi_high = 0, nmi_low = 0;
 
-       if (!cpu_gdt_descr[cpu].address &&
-           !(cpu_gdt_descr[cpu].address = get_zeroed_page(GFP_KERNEL)))
{
-               printk("Failed to allocate GDT for CPU %d\n", cpu);
-               return 1;
-       }
-
        ++cpucount;
 
        /*

removes it.

Since the allocation is in cpu_init(), it will be called on hotplug as
well.

James


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/