From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vijay Kilari Subject: Re: [RFC PATCH v1 01/10] xen/arm: make secondary gic init as notifier call Date: Sat, 22 Mar 2014 13:46:52 +0530 Message-ID: References: <1395238631-2024-1-git-send-email-vijay.kilari@gmail.com> <1395238631-2024-2-git-send-email-vijay.kilari@gmail.com> <532AE392.1080107@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <532AE392.1080107@linaro.org> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Julien Grall Cc: Ian Campbell , Stefano Stabellini , Prasun Kapoor , Vijaya Kumar K , xen-devel@lists.xen.org, Stefano Stabellini List-Id: xen-devel@lists.xenproject.org On Thu, Mar 20, 2014 at 6:18 PM, Julien Grall wrote: > Hello Vijay, > > Thanks for your patch. > > On 03/19/2014 02:17 PM, vijay.kilari@gmail.com wrote: >> From: Vijaya Kumar K >> >> 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 >> --- >> 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 >> #include >> #include >> +#include >> #include >> #include >> #include >> @@ -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