From mboxrd@z Thu Jan  1 00:00:00 1970
From: Julien Grall <julien.grall@linaro.org>
Subject: Re: [PATCH v2 05/15] xen/arm: segregate GIC low level
	functionality
Date: Wed, 09 Apr 2014 09:50:55 +0100
Message-ID: <534509EF.9010506@linaro.org>
References: <1396612593-443-1-git-send-email-vijay.kilari@gmail.com>
	<1396612593-443-6-git-send-email-vijay.kilari@gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"; Format="flowed"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <1396612593-443-6-git-send-email-vijay.kilari@gmail.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: vijay.kilari@gmail.com, Ian.Campbell@citrix.com, stefano.stabellini@eu.citrix.com, stefano.stabellini@citrix.com, xen-devel@lists.xen.org
Cc: Prasun.Kapoor@caviumnetworks.com, vijaya.kumar@caviumnetworks.com
List-Id: xen-devel@lists.xenproject.org


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

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.

[..]

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

Again, please justify every change on the prototype of every functions. 
If it's not trivial, split in small patches...

-- 
Julien Grall