Re: [PATCH 11/12 gnumach] smp: Serialise AP bringup

[Samuel Thibault]

I'd say squash this with patch 9, that'll answer Almudena's fear of
parallel startup :)

Also it'll avoid introducing code in patch 9 that gets fixed here in
patch 11.

Damien Zammit, le mar. 31 janv. 2023 09:39:35 +0000, a ecrit:
> ---
>  i386/i386/mp_desc.c     | 26 +++++++++++---------------
>  i386/i386at/ioapic.c    |  1 -
>  i386/i386at/model_dep.c |  1 -
>  i386/intel/pmap.c       |  1 +
>  kern/startup.c          |  5 ++++-
>  5 files changed, 16 insertions(+), 18 deletions(-)
> 
> diff --git a/i386/i386/mp_desc.c b/i386/i386/mp_desc.c
> index 49ed9f94..97f7b92a 100644
> --- a/i386/i386/mp_desc.c
> +++ b/i386/i386/mp_desc.c
> @@ -204,11 +204,8 @@ interrupt_processor(int cpu)
>  }
> 
>  void
> -cpu_setup()
> +cpu_setup(int cpu)
>  {
> -    unsigned apic_id = (((ApicLocalUnit*)phystokv(lapic_addr))->apic_id.r >> 
> 24) & 0xff;
> -    uint16_t cpu = apic_get_cpu_kernel_id(apic_id);
> -
>      printf("AP=(%u) before\n", cpu);
> 
>      pmap_make_temporary_mapping();
> @@ -262,11 +259,14 @@ cpu_setup()
>  void
>  cpu_ap_main()
>  {
> +    unsigned apic_id = (((ApicLocalUnit*)phystokv(lapic_addr))->apic_id.r >> 
> 24) & 0xff;
> +    int cpu = apic_get_cpu_kernel_id(apic_id);
> +
>      do {
>          asm volatile ("pause" : : : "memory");
> -    } while (!bspdone);
> +    } while (bspdone != cpu);
> 
> -    cpu_setup();
> +    cpu_setup(cpu);
>  }
> 
>  kern_return_t
> @@ -289,9 +289,6 @@ void
>  start_other_cpus(void)
>  {
>       unsigned long flags;
> -
> -     cpu_intr_save(&flags);
> -
>       int ncpus = smp_get_numcpus();
> 
>       //Copy cpu initialization assembly routine
> @@ -302,6 +299,9 @@ start_other_cpus(void)
>       lapic_enable(); /* Enable lapic only once */
>  #endif
>       unsigned cpu;
> +
> +     splhigh();
> +
>       bspdone = 0;
>       for (cpu = 1; cpu < ncpus; cpu++) {
>               machine_slot[cpu].running = FALSE;
> @@ -309,16 +309,12 @@ start_other_cpus(void)
>               //Start cpu
>               printf("Starting AP %d\n", cpu);
>               cpu_start(cpu);
> -     }
> -     printf("BSP: Completed SMP init\n");
> -     bspdone = 1;
> 
> -     for (cpu = 1; cpu < ncpus; cpu++) {
> +             bspdone++;
>               do {
>                       asm volatile ("pause" : : : "memory");
>               } while (machine_slot[cpu].running == FALSE);
>       }
> -
> -        cpu_intr_restore(flags);
> +     printf("BSP: Completed SMP init\n");
>  }
>  #endif       /* NCPUS > 1 */
> diff --git a/i386/i386at/ioapic.c b/i386/i386at/ioapic.c
> index 1dd7af58..f7b0d1d3 100644
> --- a/i386/i386at/ioapic.c
> +++ b/i386/i386at/ioapic.c
> @@ -188,7 +188,6 @@ lapic_enable_timer(void)
> 
>      /* Enable interrupts for the first time */
>      printf("LAPIC timer configured on cpu%d\n", cpu_number());
> -    asm("sti");
>  }
> 
>  void
> diff --git a/i386/i386at/model_dep.c b/i386/i386at/model_dep.c
> index cc18c864..7bf40df2 100644
> --- a/i386/i386at/model_dep.c
> +++ b/i386/i386at/model_dep.c
> @@ -624,7 +624,6 @@ startrtclock(void)
>  {
>  #ifndef APIC
>       clkstart();
> -     asm ("sti");
>       unmask_irq(0);
>  #endif
>  }
> diff --git a/i386/intel/pmap.c b/i386/intel/pmap.c
> index 5cd68878..754334c9 100644
> --- a/i386/intel/pmap.c
> +++ b/i386/intel/pmap.c
> @@ -2920,6 +2920,7 @@ void pmap_update_interrupt(void)
>       int             s;
> 
>       my_cpu = cpu_number();
> +     printf("PMAP(%d)\n", my_cpu);
> 
>       /*
>        *      Exit now if we're idle.  We'll pick up the update request
> diff --git a/kern/startup.c b/kern/startup.c
> index 2eb3a739..42f5ac6c 100644
> --- a/kern/startup.c
> +++ b/kern/startup.c
> @@ -308,8 +308,11 @@ void cpu_launch_first_thread(thread_t th)
> 
>       PMAP_ACTIVATE_USER(vm_map_pmap(th->task->map), th, mycpu);
> 
> +#if defined(APIC)
> +     lapic_enable_timer();
> +#else
>       startrtclock();         /* needs an active thread */
> -
> +#endif
>       load_context(th);
>       /*NOTREACHED*/
>  }
> --
> 2.34.1
> 
> 
> 

-- 
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.