Re: [PATCH v3 08/11] xen: arm: rewrite start of day page table and cpu bring up

[Julien Grall <julien.grall@linaro.org>]

On 09/27/2013 11:20 AM, Ian Campbell wrote:

[..]

> -static void __init init_cpus_maps(void)
> +static void __init smp_init_cpus(void)
>  {
>      register_t mpidr;
>      struct dt_device_node *cpus = dt_find_node_by_path("/cpus");
> @@ -530,6 +530,14 @@ static void __init init_cpus_maps(void)
>          [0 ... NR_CPUS - 1] = MPIDR_INVALID
>      };
>      bool_t bootcpu_valid = 0;
> +    int rc;
> +
> +    if ( (rc = arch_smp_init()) < 0 )
> +    {
> +        printk(XENLOG_WARNING "SMP init failed (%d)\n"
> +               "Using only 1 CPU\n", rc);
> +        return;
> +    }
>  
>      mpidr = boot_cpu_data.mpidr.bits & MPIDR_HWID_MASK;
>  
> @@ -581,6 +589,12 @@ static void __init init_cpus_maps(void)
>              }
>          }
>  
> +        if ( (rc = arch_cpu_init(hwid, cpu)) < 0 )

As I understand your patch #6, arch_cpu_init take a logical cpu id (on
ARM64 it's used as an index in an array).
So you should used j here.

Also, do we really need to call arch_cpu_init on the boot CPU?

> +        {
> +            printk("cpu init failed (hwid %x): %d\n", hwid, rc);
> +            continue;
> +        }
> +
>          /*
>           * Build a stashed array of MPIDR values. Numbering scheme requires
>           * that if detected the boot CPU must be assigned logical id 0. Other
> @@ -599,7 +613,8 @@ static void __init init_cpus_maps(void)
>  
>          if ( cpuidx > NR_CPUS )
>          {
> -            printk(XENLOG_WARNING "DT /cpu %u node greater than max cores %u, capping them\n",
> +            printk(XENLOG_WARNING
> +                   "DT /cpu %u node greater than max cores %u, capping them\n",
>                     cpuidx, NR_CPUS);
>              cpuidx = NR_CPUS;
>              break;
> @@ -657,15 +672,14 @@ void __init start_xen(unsigned long boot_phys_offset,
>  
>      processor_id();
>  
> -    init_cpus_maps();
> -    cpus = smp_get_max_cpus();
> -
>      platform_init();
>  
> +    smp_init_cpus();
> +    cpus = smp_get_max_cpus();
> +
>      init_xen_time();
>  
>      gic_init();
> -    make_cpus_ready(cpus, boot_phys_offset);
>  
>      set_current((struct vcpu *)0xfffff000); /* debug sanity */
>      idle_vcpu[0] = current;
> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> index 234748e..7b4ad8a 100644
> --- a/xen/arch/arm/smpboot.c
> +++ b/xen/arch/arm/smpboot.c
> @@ -56,12 +56,10 @@ struct init_info __initdata init_data =
>  };
>  
>  /* Shared state for coordinating CPU bringup */
> -unsigned long smp_up_cpu = 0;
> +unsigned long smp_up_cpu = ~0UL;

MPIDR_INVALID?

[..]

>  /* Boot the current CPU */
>  void __cpuinit start_secondary(unsigned long boot_phys_offset,
>                                 unsigned long fdt_paddr,
> @@ -176,6 +147,7 @@ void __cpuinit start_secondary(unsigned long boot_phys_offset,
>      wmb();
>  
>      /* Now report this CPU is up */
> +    smp_up_cpu = ~0UL;

smp_up_cpu = MPIDR_INVALID?

Also, perhaps a dsb is needed here to ensure to update smp_up_cpu before
cpumask_set_cpu is updated.

>      cpumask_set_cpu(cpuid, &cpu_online_map);
>      wmb();

-- 
Julien Grall