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

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hello Vijay,

On 22/03/14 08:16, Vijay Kilari wrote:
> On Thu, Mar 20, 2014 at 6:18 PM, Julien Grall <julien.grall@xxxxxxxxxx> wrote:
> > > +/* Set up the per-CPU parts of the GIC for a secondary CPU */
> > > +static int  __cpuinit gic_init_secondary_cpu(struct notifier_block *nfb,
> > > +                                        unsigned long action, void *hcpu)
> > > +{
> > > +    if (action == CPU_STARTING)
> > > +    {
> > > +        spin_lock(&gic.lock);
> > > +        gic_cpu_init();
> > > +        gic_hyp_init();
> > > +        spin_unlock(&gic.lock);
> > > +    }
> > > +    return NOTIFY_DONE;
> > > +}
> > > +
> > This is not the correct way to create a notifier block.
> >
> > You should have a good similar to (see an example in common/timer.c):
> >
> > static cpu_callback(struct notifier_block* nfb,
> >                      unsigned long action, void *hcpu)
> > {
> >       unsigned int cpu = (unsigned long)hcpu;
> >
> >       switch ( action )
> >       case CPU_STARTING:
> >          gic_init_secondary_cpu();
> >          break;
> >       default:
> >          break;
> >       return NOTIFY_DONE;
> > }
> Apart from __cpuinit, I could not see any difference with this implementation.
> am I missing anything specific?

I'd prefer to use the same pattern function for CPU notifier (see 
common/timer.c) rather than creating a new one.
The main difference are:
	- the function name: the callback will be called on different CPU 
state. The current name "gic_init_secondary_cpu" is too specific. We 
might want to extend this callback later.
        - switch case: It's easier to extend with a switch case compare 
to if (foo)
> > > @@ -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?
Regards,

--
Julien Grall

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