Re: [Xen-devel] [RFC PATCH v1 01/10] xen/arm: make secondary gic init as notifier call

[Vijay Kilari <vijay.kilari@xxxxxxxxx>]

On Fri, Mar 21, 2014 at 10:45 PM, Ian Campbell <Ian.Campbell@xxxxxxxxxx> wrote:
> On Wed, 2014-03-19 at 19:47 +0530, vijay.kilari@xxxxxxxxx wrote:
>> From: Vijaya Kumar K <Vijaya.Kumar@xxxxxxxxxxxxxxxxxx>
>>
>> make gic init for secondary cpus as notifier call
>> instead calling directly from secondary boot for
>> each cpu.
>
>> This makes secondary gic init generic and runtime.
>
> s/and/at/ perhaps? Otherwise I can't make sense of what you are trying
> to say.
>
 OK, I will rephrase it.
>
>> +static struct notifier_block gic_cpu_nb = {
>> +    .notifier_call = gic_init_secondary_cpu,
>> +    .priority = 100
>>
> In wondering what 100 was, I notice that the other similar uses have a
> "/* Highest priority */" comment, please add one here too.
>
> I notice that the percpu stuff (cpu_percpu_nfb) is also priority == 100,
> which makes me suspect that they will run in arbitrary order, which
> makes me uncomfortable, not least because at least gic_{cpu,hyp}_init
> both touch per-cpu data structures.
>
> Ah, I see the percpu stuff is a CPU_UP_PREPARE notifier, so has already
> happened. Good. It does make me wonder if it is wise to do something as
> critical as interrupt controller setup in a notifier. Do you think this
> is necessary for some reason?
>
   Yes, I kept it at highest priority thinking that it is critical.
    from the comment in include/xen/cpu.h, CPU_STARTING suits because
    gic_{cpu,hyp}_init should run on new cpu.

      /* CPU_STARTING: CPU nearly online. Runs on new CPU, irqs still
disabled. */

    Is there any difference with Xen compared to kernel? that suggest me to use
    CPU_UP_PREPARE?

    If we don't implement as notifier, then it has to be called from
generic gic code
    through call back as we did for other call ,which is not required
as it is mostly one time called

>> +};
>
>> +static void gic_smp_init(void)
>
> Like Julien says there isn't really a need for this, but if it were it
> should be __init I think.
>
OK
>
> Ian.
>

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel