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

[Vijay Kilari <vijay.kilari@gmail.com>]

On Thu, Mar 20, 2014 at 6:18 PM, Julien Grall <julien.grall@linaro.org> wrote:
> Hello Vijay,
>
> Thanks for your patch.
>
> On 03/19/2014 02:17 PM, vijay.kilari@gmail.com wrote:
>> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>>
>> 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.
>>
>> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
>> ---
>>  xen/arch/arm/gic.c        |   35 ++++++++++++++++++++++++++---------
>>  xen/arch/arm/smpboot.c    |    3 +--
>>  xen/include/asm-arm/gic.h |    2 --
>>  3 files changed, 27 insertions(+), 13 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index 91a2982..4be0897 100644
>> --- a/xen/arch/arm/gic.c
>> +++ b/xen/arch/arm/gic.c
>> @@ -20,6 +20,7 @@
>>  #include <xen/config.h>
>>  #include <xen/lib.h>
>>  #include <xen/init.h>
>> +#include <xen/cpu.h>
>>  #include <xen/mm.h>
>>  #include <xen/irq.h>
>>  #include <xen/sched.h>
>> @@ -380,6 +381,30 @@ static void __cpuinit gic_hyp_disable(void)
>>      GICH[GICH_HCR] = 0;
>>  }
>>
>> +/* 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?

>
>> +static struct notifier_block gic_cpu_nb = {
>> +    .notifier_call = gic_init_secondary_cpu,
>> +    .priority = 100
>> +};
>
>> +static void gic_smp_init(void)
>> +{
>> +   register_cpu_notifier(&gic_cpu_nb);
>> +}
>> +
>
> You don't need to create a separate function to register the notifier.
> You can directly call it in gic_init.
>
OK
>>  int gic_irq_xlate(const u32 *intspec, unsigned int intsize,
>>                    unsigned int *out_hwirq,
>>                    unsigned int *out_type)
>> @@ -469,6 +494,7 @@ void __init gic_init(void)
>>      spin_lock_init(&gic.lock);
>>      spin_lock(&gic.lock);
>>
>> +    gic_smp_init();
>>      gic_dist_init();
>>      gic_cpu_init();
>>      gic_hyp_init();
>> @@ -524,15 +550,6 @@ void smp_send_state_dump(unsigned int cpu)
>>      send_SGI_one(cpu, GIC_SGI_DUMP_STATE);
>>  }
>>
>> -/* Set up the per-CPU parts of the GIC for a secondary CPU */
>> -void __cpuinit gic_init_secondary_cpu(void)
>> -{
>> -    spin_lock(&gic.lock);
>> -    gic_cpu_init();
>> -    gic_hyp_init();
>> -    spin_unlock(&gic.lock);
>> -}
>> -
>>  /* Shut down the per-CPU GIC interface */
>>  void gic_disable_cpu(void)
>>  {
>> diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
>> index a829957..765efcf 100644
>> --- a/xen/arch/arm/smpboot.c
>> +++ b/xen/arch/arm/smpboot.c
>> @@ -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.

    init_secondary_IRQ();

    gic_route_ppis();

    init_maintenance_interrupt();
    init_timer_interrupt();

    In linux also similar problem (not same) with GICv3 is observered
    http://marc.info/?l=git-commits-head&m=138418939813393&w=2

>>
>>      init_secondary_IRQ();
>>
>> @@ -297,7 +297,6 @@ void __cpuinit start_secondary(unsigned long boot_phys_offset,
>>      setup_cpu_sibling_map(cpuid);
>>
>>      /* Run local notifiers */
>
> Please move also the comment, it's part of notify_cpu_starting. It
> doesn't make any sense alone here.
>
OK
>> -    notify_cpu_starting(cpuid);
>>      wmb();
>
> Regards,
>
> --
> Julien Grall