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

[George Dunlap <George.Dunlap@eu.citrix.com>]

On Wed, Mar 26, 2014 at 2:41 PM, Julien Grall <julien.grall@linaro.org> wrote:
> Hello Vijay,
>
> (Adding George for the scheduler part)
>
> On 03/26/2014 11:27 AM, Vijay Kilari wrote:
>> Hi Julien,
>>
>>>>>> @@ -283,7 +283,7 @@ void __cpuinit start_secondary(unsigned long
>>>>>> boot_phys_offset,
>>>>>>
>>>>>>       mmu_init_secondary_cpu();
>>>>>>
>>>>>> -    gic_init_secondary_cpu();
>>>>>> +    notify_cpu_starting(cpuid);
>>>>>
>>>>>
>>>>> Can you explain why it's safe to move notify_cpu_starting earlier?
>>>>>
>>>>     When gic registers a notifier with action as CPU_STARTING, I am
>>>> getting a panic
>>>>     from gic driver because notify_cpu_starting() is called after most of
>>>> the GIC
>>>>     initialization calls as below from start_secondary() are called.
>>>> Especially the issue is coming
>>>>     with init_maintenanc_interrupt(). So I have moved this notifier
>>>> before other GIC initialization
>>>>     calls and since I move notifier only before GIC initialization
>>>> calls it not be a problem.
>>>
>>>
>>> It doesn't explain why it's safe... CPU_STARTING is also used in some place
>>> to initialize internal data structure. Are you sure that theses callbacks
>>> can be called earlier?
>>>
>>
>> The below callback is the only callback that is using CPU_STARTING,
>> IMO it is only initializing pcpu data.
>>
>> static int cpu_credit2_callback(
>>     struct notifier_block *nfb, unsigned long action, void *hcpu)
>> {
>>     unsigned int cpu = (unsigned long)hcpu;
>>     int rc = 0;
>>
>>     switch ( action )
>>     {
>>     case CPU_STARTING:
>>         csched_cpu_starting(cpu);
>>         break;
>>     default:
>>         break;
>>     }
>>
>>     return !rc ? NOTIFY_DONE : notifier_from_errno(rc);
>> }
>>
>> With this patch, notifier is only called just before GIC initialization.
>> So should not be a problem. Let me know your opinion? I am
>> not very familiar with this piece of code.
>
> I think, the sched credit2 initialization code is relying on the sibling
> map (see cpu_to_socket), which is basically a no-op for now on ARM.
>
> You may have to also move setup_cpu_sibling_map(cpuid) earlier.
>
> I don't know enough the scheduler to say it's safe to move earlier.
> George any opinion?

Well it definitely breaks the interface of what CPU_STARTING means
defined in cpu.h:

/* CPU_STARTING: CPU nearly online. Runs on new CPU, irqs still disabled. */
#define CPU_STARTING     (0x0003 | NOTIFY_FORWARD)

I'm not sure exactly what you're trying to accomplish by running this
as a notifier instead of just calling it directly, but I think using
CPU_STARTING it not what you want.  In any case, you'd need to change
the definition in generic code, and in the x86 code if that's what you
wanted to do.

 -George