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 03:21 PM, Ian Campbell wrote:
> On Fri, 2013-09-27 at 15:10 +0100, Ian Campbell wrote:
>> On Fri, 2013-09-27 at 14:30 +0100, Julien Grall wrote:
>>
>>>> @@ -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).
>>
>> Yes, I thought I wanted to pass the hwid here, but it looks like I've
>> got my wires crossed.
>>
>>> So you should used j here.
>>
>> You mean cpuidx I think, after having moved the call after the cpuidx++
>>
>> I wanted to handle the case where the function failed by having
>> possible_map not contain failed cpus. I think I'll handle this by making
>> tmp_map[i] == INVALID_MIDR in that case and checking that in the loop
>> which sets bits in cpu_possible_map.
> 
> Incrementally this looks like:

It looks good to me.

> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
> index 5923cfc..b836be4 100644
> --- a/xen/arch/arm/smpboot.c
> +++ b/xen/arch/arm/smpboot.c
> @@ -165,12 +165,6 @@ void __init smp_init_cpus(void)
>              }
>          }
>  
> -        if ( (rc = arch_cpu_init(hwid, cpu)) < 0 )
> -        {
> -            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
> @@ -196,7 +190,13 @@ void __init smp_init_cpus(void)
>              break;
>          }
>  
> -        tmp_map[i] = hwid;
> +        if ( (rc = arch_cpu_init(i, cpu)) < 0 )
> +        {
> +            printk("cpu%d init failed (hwid %x): %d\n", i, hwid, rc);
> +            tmp_map[i] = MPIDR_INVALID;
> +        }
> +        else
> +            tmp_map[i] = hwid;
>      }
>  
>      if ( !bootcpu_valid )
> @@ -208,6 +208,8 @@ void __init smp_init_cpus(void)
>  
>      for ( i = 0; i < cpuidx; i++ )
>      {
> +        if ( tmp_map[i] == MPIDR_INVALID )
> +            continue;
>          cpumask_set_cpu(i, &cpu_possible_map);
>          cpu_logical_map(i) = tmp_map[i];
>      }
> 
> 


-- 
Julien Grall