Re: [PATCH 4/7] xen/arm: gic: Use the correct CPU ID

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

On 09/09/2013 02:28 PM, Ian Campbell wrote:
> On Fri, 2013-08-30 at 14:30 +0100, 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>
>> ---
>>  xen/arch/arm/gic.c |   35 ++++++++++++++++++++++++++++-------
>>  1 file changed, 28 insertions(+), 7 deletions(-)
>>
>> diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
>> index cadc258..4f3a8a5 100644
>> --- a/xen/arch/arm/gic.c
>> +++ b/xen/arch/arm/gic.c
>> @@ -57,6 +57,27 @@ 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
>> + */
>> +#define NR_GIC_CPU_IF 8
>> +static u8 gic_cpu_map[NR_GIC_CPU_IF] __read_mostly = {0xff};
> 
> Isn't this mapping from logical cpus ids to gic cpus ids? IOW the size
> of this array is the wrong way around?

This array maps a logical cpu id (the index) to a gic cpus id. The size
if correct, because Xen will allocate logical cpu id from 0 to 7.

> Is what you want here is a per-cpu variable?

Does per-cpu variable allow any cpu to retrieve data of another cpu? I
didn't find a such function.

>> +
>> +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 |= gic_cpu_map[cpu];
> 
> Further to above, can't cpu here be e.g. 0x100 for an a7 on a big.ITTLE
> system?

0x100 is the hardware CPU id. Xen will allocate a logical cpu id from 0.

>> +    }
>> +
>> +    return mask;
>> +}
>> +
>>  unsigned int gic_number_lines(void)
>>  {
>>      return gic.lines;
>> @@ -206,9 +227,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];
>> @@ -317,6 +336,8 @@ static void __cpuinit gic_cpu_init(void)
>>  {
>>      int i;
>>  
>> +    gic_cpu_map[smp_processor_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. */
>> @@ -443,13 +464,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