Re: [PATCH v3 03/16] xen/arm: make sgi handling generic

[Julien Grall <julien.grall@linaro.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