Re: [PATCH v3 3/6] xen/arm: gic: Use the correct CPU ID

[Julien Grall <julien.grall@linaro.org>]

On 09/18/2013 02:15 PM, Julien Grall wrote:
> The GIC mapping of CPU interfaces does not necessarily match the logical
> CPU numbering.
> 
> When Xen wants to send an SGI to specific CPU, it needs to use the GIC CPU ID.
> It can be retrieved from ITARGETSR0, in fact when this field is read, the GIC
> will return a value that corresponds only to the processor reading the register.
> So Xen can use the PPI 0 to initialize the mapping.
> 
> Signed-off-by: Julien Grall <julien.grall@linaro.org>
> 
> ---
>     Changes in v3:
>         - Correctly create the mask in gic_cpu_mask
> 
>     Changes in v2:
>         - Use per-cpu variable instead of an array
>         - Add comment for NR_GIC_CPU_IF
> ---
>  xen/arch/arm/gic.c |   37 ++++++++++++++++++++++++++++++-------
>  1 file changed, 30 insertions(+), 7 deletions(-)
> 
> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> index b969d23..4061691 100644
> --- a/xen/arch/arm/gic.c
> +++ b/xen/arch/arm/gic.c
> @@ -57,6 +57,29 @@ static DEFINE_PER_CPU(uint64_t, lr_mask);
>  
>  static unsigned nr_lrs;
>  
> +/* The GIC mapping of CPU interfaces does not necessarily match the
> + * logical CPU numbering. Let's use mapping as returned by the GIC
> + * itself
> + */
> +static DEFINE_PER_CPU(u8, gic_cpu_id);
> +

Following the discussion on another thread
(http://lists.xen.org/archives/html/xen-devel/2013-09/msg02072.html), we
need to initialize each CPU ID to 0xff by default, to be able to bring
up secondary CPUs with an SGI (send_SGI_mask).
But as I understand, per_cpu area for a give area is initialized in
cpu_up and there is no way to give a default value. So user could call
send_SGI_mask to early function and use an invalid CPU area.

IMHO, this solution is too fragile and I would like to use the same
solution as my first version (ie with an array of 8 cells).

Ian, is ok for you?

> +/* Maximum cpu interface per GIC */
> +#define NR_GIC_CPU_IF 8
> +
> +static unsigned int gic_cpu_mask(const cpumask_t *cpumask)
> +{
> +    unsigned int cpu;
> +    unsigned int mask = 0;
> +
> +    for_each_cpu(cpu, cpumask)
> +    {
> +        ASSERT(cpu < NR_GIC_CPU_IF);
> +        mask |= per_cpu(gic_cpu_id, cpu);
> +    }
> +
> +    return mask;
> +}
> +
>  unsigned int gic_number_lines(void)
>  {
>      return gic.lines;
> @@ -189,9 +212,7 @@ static void gic_set_irq_properties(unsigned int irq, bool_t level,
>  {
>      volatile unsigned char *bytereg;
>      uint32_t cfg, edgebit;
> -    unsigned int mask = cpumask_bits(cpu_mask)[0];
> -
> -    ASSERT(!(mask & ~0xff)); /* Target bitmap only support 8 CPUS */
> +    unsigned int mask = gic_cpu_mask(cpu_mask);
>  
>      /* Set edge / level */
>      cfg = GICD[GICD_ICFGR + irq / 16];
> @@ -300,6 +321,8 @@ static void __cpuinit gic_cpu_init(void)
>  {
>      int i;
>  
> +    this_cpu(gic_cpu_id) = GICD[GICD_ITARGETSR] & 0xff;
> +
>      /* The first 32 interrupts (PPI and SGI) are banked per-cpu, so
>       * even though they are controlled with GICD registers, they must
>       * be set up here with the other per-cpu state. */
> @@ -431,13 +454,13 @@ void __init gic_init(void)
>  
>  void send_SGI_mask(const cpumask_t *cpumask, enum gic_sgi sgi)
>  {
> -    unsigned long mask = cpumask_bits(cpumask)[0];
> +    cpumask_t online_mask;
> +    unsigned int mask = 0;
>  
>      ASSERT(sgi < 16); /* There are only 16 SGIs */
>  
> -    mask &= cpumask_bits(&cpu_online_map)[0];
> -
> -    ASSERT(mask < 0x100); /* The target bitmap only supports 8 CPUs */
> +    cpumask_and(&online_mask, cpumask, &cpu_online_map);
> +    mask = gic_cpu_mask(&online_mask);
>  
>      dsb();
>  
> 


-- 
Julien Grall