From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vijay Kilari Subject: Re: [PATCH v2 05/15] xen/arm: segregate GIC low level functionality Date: Wed, 9 Apr 2014 17:04:52 +0530 Message-ID: References: <1396612593-443-1-git-send-email-vijay.kilari@gmail.com> <1396612593-443-6-git-send-email-vijay.kilari@gmail.com> <534509EF.9010506@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <534509EF.9010506@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 Wed, Apr 9, 2014 at 2:20 PM, Julien Grall wrote: > > Hello Vijaya, > > > On 04/04/14 12:56, vijay.kilari@gmail.com wrote: >> >> +static void gic_send_sgi(const cpumask_t *cpumask, enum gic_sgi sgi) >> { >> unsigned int mask = 0; >> cpumask_t online_mask; >> @@ -498,30 +606,26 @@ void send_SGI_mask(const cpumask_t *cpumask, enum >> gic_sgi sgi) >> | sgi; >> } >> >> -void send_SGI_one(unsigned int cpu, enum gic_sgi sgi) >> +void send_SGI_mask(const cpumask_t *cpumask, enum gic_sgi sgi) >> { >> - ASSERT(cpu < NR_GIC_CPU_IF); /* Targets bitmap only supports 8 CPUs >> */ >> - send_SGI_mask(cpumask_of(cpu), sgi); >> + gic_hw_ops->send_sgi(cpumask, sgi); >> } >> >> -void send_SGI_self(enum gic_sgi sgi) >> +void send_SGI_one(unsigned int cpu, enum gic_sgi sgi) >> { >> - ASSERT(sgi < 16); /* There are only 16 SGIs */ >> - >> - dsb(sy); >> - >> - GICD[GICD_SGIR] = GICD_SGI_TARGET_SELF >> - | sgi; >> + ASSERT(cpu < NR_GIC_CPU_IF); /* Targets bitmap only supports 8 CPUs >> */ >> + send_SGI_mask(cpumask_of(cpu), sgi); >> } >> >> void send_SGI_allbutself(enum gic_sgi sgi) >> { >> - ASSERT(sgi < 16); /* There are only 16 SGIs */ >> + cpumask_t all_others_mask; >> + ASSERT(sgi < 16); /* There are only 16 SGIs */ >> >> - dsb(sy); >> + cpumask_andnot(&all_others_mask, &cpu_possible_map, >> cpumask_of(smp_processor_id())); >> >> - GICD[GICD_SGIR] = GICD_SGI_TARGET_OTHERS >> - | sgi; >> + dsb(sy); >> + send_SGI_mask(&all_others_mask, sgi); >> } > > > As I said in V1, this change breaks SMP boot with GICv2... > > GICD_SGI_TARGERT_OTHERS will send an SGI to every CPUs even if the CPU is > not yet online (i.e. not registered by Xen). It's used during secondary boot > (cpu_up_send_sgi). cpumask_andnot(&all_others_mask, &cpu_possible_map,cpumask_of(smp_processor_id())); In my understanding, with the above statement, I am using cpu_possible_map (all possible cpu's) which should contains all the cpu possible cpu masks. so this is fine. The issue could be in gic_send_sgi() call which is always "and" with cpu_online_map static void gic_send_sgi(const cpumask_t *cpumask, enum gic_sgi sgi) { unsigned int mask = 0; cpumask_t online_mask; ASSERT(sgi < 16); /* There are only 16 SGIs */ cpumask_and(&online_mask, cpumask, &cpu_online_map); mask = gic_cpu_mask(&online_mask); dsb(sy); GICD[GICD_SGIR] = GICD_SGI_TARGET_LIST | (mask< Your solution won't work because send_SGI_mask only deal with online CPU. > > All the changes of send_sgi is more than splitting functions... this should > be moved on another patch and you should justify the modifications. > I will check if I could make this fix in a separate patch. > [..] > > >> int gic_route_irq_to_guest(struct domain *d, const struct dt_irq *irq, >> @@ -921,10 +1046,10 @@ out: >> return retval; >> } >> >> -static void do_sgi(struct cpu_user_regs *regs, int othercpu, enum gic_sgi >> sgi) >> +static void do_sgi(struct cpu_user_regs *regs, enum gic_sgi sgi) > > > Why did you drop the othercpu here? > othercpu is not used at all. and also othercpu is computed with IAR fields #defines which is not required in this generic code. > Again, please justify every change on the prototype of every functions. If > it's not trivial, split in small patches... > > -- > Julien Grall