From mboxrd@z Thu Jan  1 00:00:00 1970
From: Julien Grall <julien.grall@linaro.org>
Subject: Re: [PATCH v3 03/16] xen/arm: make sgi handling generic
Date: Tue, 15 Apr 2014 18:51:29 +0100
Message-ID: <534D71A1.4050702@linaro.org>
References: <1397560675-29861-1-git-send-email-vijay.kilari@gmail.com>
	<1397560675-29861-4-git-send-email-vijay.kilari@gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <1397560675-29861-4-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
Cc: Ian.Campbell@citrix.com, stefano.stabellini@eu.citrix.com, Prasun.Kapoor@caviumnetworks.com, vijaya.kumar@caviumnetworks.com, xen-devel@lists.xen.org, stefano.stabellini@citrix.com
List-Id: xen-devel@lists.xenproject.org

Hello Vijaya,

Thank you for the patch.

On 04/15/2014 12:17 PM, vijay.kilari@gmail.com wrote:
> From: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> 
> move all the hw specific sgi handling functionality
> to one function and use it.
> 
> Signed-off-by: Vijaya Kumar K <Vijaya.Kumar@caviumnetworks.com>
> ---
>  xen/arch/arm/gic.c        |   36 +++++++++++++++++++++++++++---------
>  xen/include/asm-arm/gic.h |    4 ++++
>  2 files changed, 31 insertions(+), 9 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index 7faa0e9..d5cc931 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -480,21 +480,39 @@ void __init gic_init(void)
>      spin_unlock(&gic.lock);
>  }
>  
> -void send_SGI_mask(const cpumask_t *cpumask, enum gic_sgi sgi)
> +void send_sgi(const cpumask_t *online_mask, enum gic_sgi sgi, uint8_t irqmode)

You would rename the function in send_SGI to stay consistent with the
other functions.

This function has to be static because you are only using it internally.

The first parameter "online_mask" is confusing. I would rename it in
"cpumask".


>  {
>      unsigned int mask = 0;
> +
> +    switch(irqmode)

switch ( ... )

> +    {
> +    case SGI_TARGET_OTHERS:
> +        GICD[GICD_SGIR] = GICD_SGI_TARGET_OTHERS | sgi;
> +        break;
> +    case SGI_TARGET_SELF:
> +        GICD[GICD_SGIR] = GICD_SGI_TARGET_SELF | sgi;
> +        break;
> +    case SGI_TARGET_LIST:
> +        mask = gic_cpu_mask(online_mask);
> +        GICD[GICD_SGIR] = GICD_SGI_TARGET_LIST |
> +                          (mask<<GICD_SGI_TARGET_SHIFT) | sgi;
> +        break;
> +    default:
> +         gdprintk(XENLOG_WARNING, "Wrong sgi irq mode for sgi %x\n", sgi);

You can't use gdprintk here as the function may not be used by guest.

You should replace by dprintk(...). Even better a BUG_ON as this should
be never reached.

[..]

>  void smp_send_state_dump(unsigned int cpu)
> diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> index 5d8f7f1..d03b490 100644
> --- a/xen/include/asm-arm/gic.h
> +++ b/xen/include/asm-arm/gic.h
> @@ -50,6 +50,10 @@
>  #define GICD_SPENDSGIRN (0xF2C/4)
>  #define GICD_ICPIDR2    (0xFE8/4)
>  
> +#define SGI_TARGET_LIST    0
> +#define SGI_TARGET_OTHERS  1
> +#define SGI_TARGET_SELF    2
> +

You don't need to export it. It's used internally...

I would also use an enum as we know it's bound.

Regards,

-- 
Julien Grall