Re: [PATCH 4/4] i386: Refactor int stacks to be per cpu for SMP

[Samuel Thibault]

Damien Zammit, le ven. 11 nov. 2022 23:21:38 +0000, a ecrit:
> diff --git a/i386/i386/cpu_number.h b/i386/i386/cpu_number.h
> index 9aef6370..d56cb602 100644
> --- a/i386/i386/cpu_number.h
> +++ b/i386/i386/cpu_number.h
> @@ -35,14 +35,35 @@
>  /* More-specific code must define cpu_number() and CPU_NUMBER.  */
>  #ifdef __i386__
>  #define      CX(addr, reg)   addr(,reg,4)
> +
> +/* CPU_NUMBER(%ebx) will _not_ work! */
> +#define      CPU_NUMBER(reg) \
> +     pushfl                          ;\
> +     cli                             ;\
> +     pushl   %esi                    ;\
> +     pushl   %edi                    ;\
> +     pushl   %ebx                    ;\
> +     pushl   %eax                    ;\
> +     call    EXT(cpu_number)         ;\
> +     movl    %eax, %ebx              ;\
> +     popl    %eax                    ;\
> +     movl    %ebx, reg               ;\
> +     popl    %ebx                    ;\
> +     popl    %edi                    ;\
> +     popl    %esi                    ;\
> +     popfl

Again, the point of the CPU_NUMBER() macro is to make it efficient and
safe to call in tricky contexts. I'd think we can write an assembly
version of (lapic->apic_id.r >> 24) & 0xff + the loop to find the
apic_id in apic_data.cpu_lapic_list?

>  #endif
>  #ifdef __x86_64__
>  #define      CX(addr, reg)   addr(,reg,8)
> +#warning Missing CPU_NUMBER() for 64 bit
> +#define CPU_NUMBER(reg)

Is it not the same apic code?

> diff --git a/i386/i386/cswitch.S b/i386/i386/cswitch.S
> index 718c8aac..ae941bdd 100644
> --- a/i386/i386/cswitch.S
> +++ b/i386/i386/cswitch.S
> @@ -110,7 +110,7 @@ ENTRY(Thread_continue)
>   */
>  ENTRY(switch_to_shutdown_context)
>       CPU_NUMBER(%edx)
> -     movl    EXT(active_stacks)(,%edx,4),%ecx        /* get old kernel stack 
> */
> +     movl    CX(EXT(active_stacks),%edx),%ecx        /* get old kernel stack 
> */
>       movl    %ebx,KSS_EBX(%ecx)              /* save registers */
>       movl    %ebp,KSS_EBP(%ecx)
>       movl    %edi,KSS_EDI(%ecx)
> @@ -124,8 +124,8 @@ ENTRY(switch_to_shutdown_context)
>       movl    4(%esp),%ebx                    /* get routine to run next */
>       movl    8(%esp),%esi                    /* get its argument */
> 
> -     movl    EXT(interrupt_stack)(,%edx,4),%ecx      /* point to its 
> interrupt stack */
> -     lea     INTSTACK_SIZE(%ecx),%esp        /* switch to it (top) */
> +     movl    CX(EXT(int_stack_base),%edx),%ecx       /* point to its 
> interrupt stack */
> +     lea     -4+INTSTACK_SIZE(%ecx),%esp     /* switch to it (top) */
> 
>       pushl   %eax                            /* push thread */
>       call    EXT(thread_dispatch)            /* reschedule thread */
> diff --git a/i386/i386/locore.S b/i386/i386/locore.S
> index b5122613..fb92b6e7 100644
> --- a/i386/i386/locore.S
> +++ b/i386/i386/locore.S
> @@ -724,19 +728,20 @@ LEXT(return_to_iret)                    /* ( label for 
> kdb_kintr and hardclock) */
>       pop     %fs
>       pop     %es
>       pop     %ds
> -     pop     %edx
> -     pop     %ecx
> -     pop     %eax
> +     popl    %edx
> +     popl    %ecx
> +     popl    %eax

That's just the same, no need to change these.

> diff --git a/i386/i386/mp_desc.c b/i386/i386/mp_desc.c
> index 1e9ea0fc..c6a55d90 100644
> --- a/i386/i386/mp_desc.c
> +++ b/i386/i386/mp_desc.c
> +#if  NCPUS > 1
>  /*
> - * First cpu`s interrupt stack.
> + * Flag to mark SMP init by BSP complete
>   */
> -extern char          _intstack[];    /* bottom */
> -extern char          _eintstack[];   /* top */
> +int bspdone;
> +
> +extern void *apboot, *apbootend;
> +extern volatile ApicLocalUnit* lapic;

Use extern declarations in headers.

>  start_other_cpus(void)
>  {
> -     int cpu;
> -     for (cpu = 0; cpu < NCPUS; cpu++)
> -             if (cpu != cpu_number())
> -                     cpu_start(cpu);
> -}
> +     unsigned long flags;
> +
> +     cpu_intr_save(&flags);
> +
> +     int ncpus = smp_get_numcpus();
> +
> +     //Copy cpu initialization assembly routine
> +     memcpy((void*)phystokv(AP_BOOT_ADDR), (void*) &apboot,
> +            (uint32_t)&apbootend - (uint32_t)&apboot);

Again, do we have something that makes sure that this area is not allocated for
something else?

> diff --git a/i386/i386at/boothdr.S b/i386/i386at/boothdr.S
> index a4830326..79d186eb 100644
> --- a/i386/i386at/boothdr.S
> +++ b/i386/i386at/boothdr.S
> @@ -1,6 +1,6 @@
> 
>  #include <mach/machine/asm.h>
> -
> +#include <i386/apic.h>
>  #include <i386/i386asm.h>
> 
>       /*
> @@ -54,7 +54,18 @@ boot_entry:
>       movw    %ax,%ss
> 
>       /* Switch to our own interrupt stack.  */
> -     movl    $_intstack+INTSTACK_SIZE,%esp
> +     movl    $solid_intstack+INTSTACK_SIZE-4, %esp
> +     andl    $0xfffffff0,%esp
> +
> +     /* Enable local apic */

This probably needs an ifdef APIC?

> +     xorl    %eax, %eax
> +     xorl    %edx, %edx
> +     movl    $APIC_MSR, %ecx
> +     rdmsr
> +     orl     $APIC_MSR_ENABLE, %eax
> +     orl     $APIC_MSR_BSP, %eax
> +     movl    $APIC_MSR, %ecx
> +     wrmsr
> 
>       /* Reset EFLAGS to a known state.  */
>       pushl   $0
> @@ -91,9 +102,6 @@ iplt_done:
>       /* Jump into C code.  */
>       call    EXT(c_boot_entry)
> 
> -     .comm   _intstack,INTSTACK_SIZE
> -     .comm   _eintstack,0
> -
>  .align 16
>       .word 0
>  boot_gdt_descr:

> diff --git a/i386/i386at/model_dep.c b/i386/i386at/model_dep.c
> index 1819526b..ad1128ca 100644
> --- a/i386/i386at/model_dep.c
> +++ b/i386/i386at/model_dep.c
> @@ -650,7 +584,6 @@ void c_boot_entry(vm_offset_t bi)
>  #endif       /* MACH_KDB */
> 
>       machine_slot[0].is_cpu = TRUE;
> -     machine_slot[0].running = TRUE;

Why?

>       machine_slot[0].cpu_subtype = CPU_SUBTYPE_AT386;
> 
>       switch (cpu_type)

> diff --git a/i386/i386at/model_dep.h b/i386/i386at/model_dep.h
> index a972695f..f72ddc3b 100644
> --- a/i386/i386at/model_dep.h
> +++ b/i386/i386at/model_dep.h
> @@ -28,10 +28,9 @@
>  extern vm_offset_t int_stack_top[NCPUS], int_stack_base[NCPUS];
> 
>  /* Check whether P points to the interrupt stack.  */
> -#define ON_INT_STACK(P)      (((P) & ~(KERNEL_STACK_SIZE-1)) == 
> int_stack_base[0])
> +#define ON_INT_STACK(P)      (((P) & ~(NCPUS*INTSTACK_SIZE-1)) == 
> int_stack_base[0])

This needs to use the current CPU stack base.

> diff --git a/i386/intel/pmap.c b/i386/intel/pmap.c
> index 0f2ad641..490f8459 100644
> --- a/i386/intel/pmap.c
> +++ b/i386/intel/pmap.c
> @@ -1261,7 +1262,7 @@ pmap_t pmap_create(vm_size_t size)
>                       return PMAP_NULL;
>               }
>               memcpy(page_dir[i],
> -                    (void *) kernel_page_dir + i * INTEL_PGBYTES,
> +                    (void *) ap_page_dir[0] + i * INTEL_PGBYTES,

I'd say make kernel_page_dir a macro for ap_page_dir[0], so it'll be
simpler to read/maintain (and avoid changing it in all the places that
don't care about APs)

> diff --git a/linux/dev/arch/i386/kernel/irq.c 
> b/linux/dev/arch/i386/kernel/irq.c
> index 67feea84..6f99003e 100644
> --- a/linux/dev/arch/i386/kernel/irq.c
> +++ b/linux/dev/arch/i386/kernel/irq.c
> @@ -31,6 +31,7 @@
>  #include <i386/spl.h>
>  #include <i386/irq.h>
>  #include <i386/pit.h>
> +#include <i386/model_dep.h>
> 
>  #define MACH_INCLUDE
>  #include <linux/mm.h>
> @@ -421,7 +422,7 @@ reserve_mach_irqs (void)
>  {
>    unsigned int i;
> 
> -  for (i = 0; i < NINTR; i++)
> +  for (i = 1; i < NINTR; i++)
>      {
>        if (ivect[i] != intnull)
>       /* This dummy action does not specify SA_SHIRQ, so
> @@ -707,7 +708,6 @@ void
>  init_IRQ (void)
>  {
>    char *p;
> -  int latch = (CLKNUM + hz / 2) / hz;
> 
>    /*
>     * Ensure interrupts are disabled.
> @@ -715,19 +715,12 @@ init_IRQ (void)
>    (void) splhigh ();
> 
>  #ifndef APIC
> -  /*
> -   * Program counter 0 of 8253 to interrupt hz times per second.
> -   */
> -  outb_p (PIT_C0 | PIT_SQUAREMODE | PIT_READMODE, PITCTL_PORT);
> -  outb_p (latch & 0xff, PITCTR0_PORT);
> -  outb (latch >> 8, PITCTR0_PORT);
> -#endif
> -
>    /*
>     * Install our clock interrupt handler.
>     */
>    old_clock_handler = ivect[0];
>    ivect[0] = linux_timer_intr;
> +#endif
> 
>    reserve_mach_irqs ();
> 
> diff --git a/linux/dev/init/main.c b/linux/dev/init/main.c
> index 6d853957..207724f3 100644
> --- a/linux/dev/init/main.c
> +++ b/linux/dev/init/main.c
> @@ -160,7 +160,9 @@ linux_init (void)
>    pcmcia_init ();
>  #endif
> 
> +#ifndef APIC
>    restore_IRQ ();
> +#endif
> 
>    linux_auto_config = 0;
>  }

That part seems unrelated?

Samuel