Re: [PATCH v2 05/15] xen/arm: segregate GIC low level functionality

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