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

[Julien Grall <julien.grall@linaro.org>]

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;
}

> +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.

>  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?

>  
>      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.

> -    notify_cpu_starting(cpuid);
>      wmb();

Regards,

-- 
Julien Grall