[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

On Wed, Mar 26, 2014 at 2:41 PM, Julien Grall <julien.grall@xxxxxxxxxx> 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.


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.