diff mbox

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

Message ID 523C63C9.1040708@linaro.org
State Superseded, archived
Headers show

Commit Message

Julien Grall Sept. 20, 2013, 3:03 p.m. UTC 
I only resend this patch.

Comments

Ian Campbell Sept. 20, 2013, 3:44 p.m. UTC | #1 
On Fri, 2013-09-20 at 16:03 +0100, Julien Grall wrote:

> +static unsigned int gic_cpu_mask(const cpumask_t *cpumask)
> +{
> +    unsigned int cpu;
> +    unsigned int mask = 0;
> +    cpumask_t possible_mask;
> +
> +    cpumask_and(&possible_mask, cpumask, &cpu_possible_map);

Based on the other subconversation doesn't this need to be online_mask?
Or is the cpu area setup for cpus which are possible but not online?

Should the check (whichever it is) be an assertion?

> +    for_each_cpu(cpu, &possible_mask)
> +    {
> +        ASSERT(cpu < NR_GIC_CPU_IF);
> +        ASSERT(__per_cpu_offset[cpu] != -(long)__per_cpu_start);
> +        mask |= per_cpu(gic_cpu_id, cpu);
> +    }
> +
> +    return mask;
> +}
Julien Grall Sept. 20, 2013, 3:58 p.m. UTC | #2 
On 09/20/2013 04:44 PM, Ian Campbell wrote:
> On Fri, 2013-09-20 at 16:03 +0100, Julien Grall wrote:
> 
>> +static unsigned int gic_cpu_mask(const cpumask_t *cpumask)
>> +{
>> +    unsigned int cpu;
>> +    unsigned int mask = 0;
>> +    cpumask_t possible_mask;
>> +
>> +    cpumask_and(&possible_mask, cpumask, &cpu_possible_map);
> 
> Based on the other subconversation doesn't this need to be online_mask?
> Or is the cpu area setup for cpus which are possible but not online?

cpu area is initialized a little bit before the cpu is bring up:
  * initialize per cpu data (via the notifier_call_chain CPU_UP_PREPARE)
  * signal the cpu to boot (__cpu_up)
  ...
  * call start_secondary
      * initialize the gic cpu interface (gic_cpu_init)
      * route ppis (which used gic_cpu_mask)
      * set the cpu online

If we use the cpu online mask, Xen won't be able to route the different
ppis to the processor.

> Should the check (whichever it is) be an assertion?

If we stay with the cpu_possible_map, I think we are fine without an
assertion.

>> +    for_each_cpu(cpu, &possible_mask)
>> +    {
>> +        ASSERT(cpu < NR_GIC_CPU_IF);
>> +        ASSERT(__per_cpu_offset[cpu] != -(long)__per_cpu_start);
>> +        mask |= per_cpu(gic_cpu_id, cpu);
>> +    }
>> +
>> +    return mask;
>> +}
> 
>
Ian Campbell Sept. 20, 2013, 4:06 p.m. UTC | #3 
On Fri, 2013-09-20 at 16:58 +0100, Julien Grall wrote:
> On 09/20/2013 04:44 PM, Ian Campbell wrote:
> > On Fri, 2013-09-20 at 16:03 +0100, Julien Grall wrote:
> > 
> >> +static unsigned int gic_cpu_mask(const cpumask_t *cpumask)
> >> +{
> >> +    unsigned int cpu;
> >> +    unsigned int mask = 0;
> >> +    cpumask_t possible_mask;
> >> +
> >> +    cpumask_and(&possible_mask, cpumask, &cpu_possible_map);
> > 
> > Based on the other subconversation doesn't this need to be online_mask?
> > Or is the cpu area setup for cpus which are possible but not online?
> 
> cpu area is initialized a little bit before the cpu is bring up:
>   * initialize per cpu data (via the notifier_call_chain CPU_UP_PREPARE)
>   * signal the cpu to boot (__cpu_up)
>   ...
>   * call start_secondary
>       * initialize the gic cpu interface (gic_cpu_init)
>       * route ppis (which used gic_cpu_mask)
>       * set the cpu online
> 
> If we use the cpu online mask, Xen won't be able to route the different
> ppis to the processor.
> 
> > Should the check (whichever it is) be an assertion?
> 
> If we stay with the cpu_possible_map, I think we are fine without an
> assertion.

OK.

OOI where in the above is the cpu_possible_map setup? Quite a bit before
the CPU_UP_PREPARE hook I think? It shouldn't be a problem, just
interested...

Ian
Julien Grall Sept. 20, 2013, 6:48 p.m. UTC | #4 
On 09/20/2013 05:06 PM, Ian Campbell wrote:
> On Fri, 2013-09-20 at 16:58 +0100, Julien Grall wrote:
>> On 09/20/2013 04:44 PM, Ian Campbell wrote:
>>> On Fri, 2013-09-20 at 16:03 +0100, Julien Grall wrote:
>>>
>>>> +static unsigned int gic_cpu_mask(const cpumask_t *cpumask)
>>>> +{
>>>> +    unsigned int cpu;
>>>> +    unsigned int mask = 0;
>>>> +    cpumask_t possible_mask;
>>>> +
>>>> +    cpumask_and(&possible_mask, cpumask, &cpu_possible_map);
>>>
>>> Based on the other subconversation doesn't this need to be online_mask?
>>> Or is the cpu area setup for cpus which are possible but not online?
>>
>> cpu area is initialized a little bit before the cpu is bring up:
>>    * initialize per cpu data (via the notifier_call_chain CPU_UP_PREPARE)
>>    * signal the cpu to boot (__cpu_up)
>>    ...
>>    * call start_secondary
>>        * initialize the gic cpu interface (gic_cpu_init)
>>        * route ppis (which used gic_cpu_mask)
>>        * set the cpu online
>>
>> If we use the cpu online mask, Xen won't be able to route the different
>> ppis to the processor.
>>
>>> Should the check (whichever it is) be an assertion?
>>
>> If we stay with the cpu_possible_map, I think we are fine without an
>> assertion.
>
> OK.
>
> OOI where in the above is the cpu_possible_map setup? Quite a bit before
> the CPU_UP_PREPARE hook I think? It shouldn't be a problem, just
> interested...

With this patch series, it's initialized by init_cpus_maps.
Ian Campbell Sept. 25, 2013, 3:35 p.m. UTC | #5 
On Fri, 2013-09-20 at 16:03 +0100, Julien Grall wrote:
> +static unsigned int gic_cpu_mask(const cpumask_t *cpumask)
> +{
> +    unsigned int cpu;
> +    unsigned int mask = 0;
> +    cpumask_t possible_mask;
> +
> +    cpumask_and(&possible_mask, cpumask, &cpu_possible_map);
> +    for_each_cpu(cpu, &possible_mask)
> +    {
> +        ASSERT(cpu < NR_GIC_CPU_IF);
> +        ASSERT(__per_cpu_offset[cpu] != -(long)__per_cpu_start);

This should be INVALID_PERCPU_AREA, but that is private to percpu.c. I
think we can live without this check. After all the CPU is in possible
map.
Julien Grall Sept. 25, 2013, 3:42 p.m. UTC | #6 
On 09/25/2013 04:35 PM, Ian Campbell wrote:
> On Fri, 2013-09-20 at 16:03 +0100, Julien Grall wrote:
>> +static unsigned int gic_cpu_mask(const cpumask_t *cpumask)
>> +{
>> +    unsigned int cpu;
>> +    unsigned int mask = 0;
>> +    cpumask_t possible_mask;
>> +
>> +    cpumask_and(&possible_mask, cpumask, &cpu_possible_map);
>> +    for_each_cpu(cpu, &possible_mask)
>> +    {
>> +        ASSERT(cpu < NR_GIC_CPU_IF);
>> +        ASSERT(__per_cpu_offset[cpu] != -(long)__per_cpu_start);
> 
> This should be INVALID_PERCPU_AREA, but that is private to percpu.c. I
> think we can live without this check. After all the CPU is in possible
> map.

Being in cpu possible map doesn't mean that the per cpu region is
initialized for the given cpu.
I have noticed the INVALID_PERCPU_AREA is the same both Intel and ARM
platform. Can we move this define in percpu.h?
Ian Campbell Sept. 25, 2013, 3:48 p.m. UTC | #7 
On Wed, 2013-09-25 at 16:42 +0100, Julien Grall wrote:
> On 09/25/2013 04:35 PM, Ian Campbell wrote:
> > On Fri, 2013-09-20 at 16:03 +0100, Julien Grall wrote:
> >> +static unsigned int gic_cpu_mask(const cpumask_t *cpumask)
> >> +{
> >> +    unsigned int cpu;
> >> +    unsigned int mask = 0;
> >> +    cpumask_t possible_mask;
> >> +
> >> +    cpumask_and(&possible_mask, cpumask, &cpu_possible_map);
> >> +    for_each_cpu(cpu, &possible_mask)
> >> +    {
> >> +        ASSERT(cpu < NR_GIC_CPU_IF);
> >> +        ASSERT(__per_cpu_offset[cpu] != -(long)__per_cpu_start);
> > 
> > This should be INVALID_PERCPU_AREA, but that is private to percpu.c. I
> > think we can live without this check. After all the CPU is in possible
> > map.
> 
> Being in cpu possible map doesn't mean that the per cpu region is
> initialized for the given cpu.

So you expect this ASSERT to trigger in practice? That's not good...

> I have noticed the INVALID_PERCPU_AREA is the same both Intel and ARM
> platform. Can we move this define in percpu.h?

I'm wondering if it should be the same, I think it was chosen on x86 to
result in a non-canonical address (i.e. a guaranteed fault) and ARM
copied it.

__per_cpu_start on ARM is in the first 2MB so -__per_cpu_start is 2MB
from the top of the 64 bit address space, which is also invalid. I
think, Tim, did you consider this or just copy the x86 value?

Anyway, my point is that they are the same only through coincidence ;-)

Ian.
Ian Campbell Sept. 25, 2013, 3:53 p.m. UTC | #8 
On Wed, 2013-09-25 at 16:48 +0100, Ian Campbell wrote:
> On Wed, 2013-09-25 at 16:42 +0100, Julien Grall wrote:
> > On 09/25/2013 04:35 PM, Ian Campbell wrote:
> > > On Fri, 2013-09-20 at 16:03 +0100, Julien Grall wrote:
> > >> +static unsigned int gic_cpu_mask(const cpumask_t *cpumask)
> > >> +{
> > >> +    unsigned int cpu;
> > >> +    unsigned int mask = 0;
> > >> +    cpumask_t possible_mask;
> > >> +
> > >> +    cpumask_and(&possible_mask, cpumask, &cpu_possible_map);
> > >> +    for_each_cpu(cpu, &possible_mask)
> > >> +    {
> > >> +        ASSERT(cpu < NR_GIC_CPU_IF);
> > >> +        ASSERT(__per_cpu_offset[cpu] != -(long)__per_cpu_start);
> > > 
> > > This should be INVALID_PERCPU_AREA, but that is private to percpu.c. I
> > > think we can live without this check. After all the CPU is in possible
> > > map.
> > 
> > Being in cpu possible map doesn't mean that the per cpu region is
> > initialized for the given cpu.
> 
> So you expect this ASSERT to trigger in practice? That's not good...
> 
> > I have noticed the INVALID_PERCPU_AREA is the same both Intel and ARM
> > platform. Can we move this define in percpu.h?
> 
> I'm wondering if it should be the same, I think it was chosen on x86 to
> result in a non-canonical address (i.e. a guaranteed fault) and ARM
> copied it.
> 
> __per_cpu_start on ARM is in the first 2MB so -__per_cpu_start is 2MB
> from the top of the 64 bit address space, which is also invalid. I
> think, Tim, did you consider this or just copy the x86 value?

Actually, since the offset of the per_cpu var gets added the result is
not invalid at all, it'll be the offset within the percpu of the area
(i.e. a small number). Luckily we deliberately don't map anything at
0..2MB and Xen is smaller than 2MB and per_cpu stuff fits inside Xen. So
it works out OK (where OK means deliberately traps). Phew.
Tim Deegan Sept. 25, 2013, 4:34 p.m. UTC | #9 
At 16:53 +0100 on 25 Sep (1380128037), Ian Campbell wrote:
> On Wed, 2013-09-25 at 16:48 +0100, Ian Campbell wrote:
> > On Wed, 2013-09-25 at 16:42 +0100, Julien Grall wrote:
> > > On 09/25/2013 04:35 PM, Ian Campbell wrote:
> > > > On Fri, 2013-09-20 at 16:03 +0100, Julien Grall wrote:
> > > >> +static unsigned int gic_cpu_mask(const cpumask_t *cpumask)
> > > >> +{
> > > >> +    unsigned int cpu;
> > > >> +    unsigned int mask = 0;
> > > >> +    cpumask_t possible_mask;
> > > >> +
> > > >> +    cpumask_and(&possible_mask, cpumask, &cpu_possible_map);
> > > >> +    for_each_cpu(cpu, &possible_mask)
> > > >> +    {
> > > >> +        ASSERT(cpu < NR_GIC_CPU_IF);
> > > >> +        ASSERT(__per_cpu_offset[cpu] != -(long)__per_cpu_start);
> > > > 
> > > > This should be INVALID_PERCPU_AREA, but that is private to percpu.c. I
> > > > think we can live without this check. After all the CPU is in possible
> > > > map.
> > > 
> > > Being in cpu possible map doesn't mean that the per cpu region is
> > > initialized for the given cpu.
> > 
> > So you expect this ASSERT to trigger in practice? That's not good...
> > 
> > > I have noticed the INVALID_PERCPU_AREA is the same both Intel and ARM
> > > platform. Can we move this define in percpu.h?
> > 
> > I'm wondering if it should be the same, I think it was chosen on x86 to
> > result in a non-canonical address (i.e. a guaranteed fault) and ARM
> > copied it.
> > 
> > __per_cpu_start on ARM is in the first 2MB so -__per_cpu_start is 2MB
> > from the top of the 64 bit address space, which is also invalid. I
> > think, Tim, did you consider this or just copy the x86 value?
> 
> Actually, since the offset of the per_cpu var gets added the result is
> not invalid at all, it'll be the offset within the percpu of the area
> (i.e. a small number). Luckily we deliberately don't map anything at
> 0..2MB and Xen is smaller than 2MB and per_cpu stuff fits inside Xen. So
> it works out OK (where OK means deliberately traps). Phew.

Yes, it maps to NULL+offset, where offset is comfortably < 1 page in the
current build; AFAICS this was deliberate on x86 and certainly
deliberate when I copied it.  The stronger property that offset must
always < 2MB didn't occur to me, but is reassuring. :)

Tim.
diff mbox

Patch

====================================================================================

commit 89aafa2d3d8a4f9ebce700673f89c8b4822a02e8
Author: Julien Grall <julien.grall@linaro.org>
Date:   Thu Aug 29 18:38:06 2013 +0100

    xen/arm: gic: Use the correct CPU ID
    
    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 v4:
            - Make logical and between the cpumask given in arguments and
            cpu_possible_map.
            - Make sure the per_cpu is initialized
            - Add comment restriction for gic_set_irq_properties
    
        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

diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
index b969d23..9f703a0 100644
--- a/xen/arch/arm/gic.c
+++ b/xen/arch/arm/gic.c
@@ -57,6 +57,32 @@  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);
+
+/* 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;
+    cpumask_t possible_mask;
+
+    cpumask_and(&possible_mask, cpumask, &cpu_possible_map);
+    for_each_cpu(cpu, &possible_mask)
+    {
+        ASSERT(cpu < NR_GIC_CPU_IF);
+        ASSERT(__per_cpu_offset[cpu] != -(long)__per_cpu_start);
+        mask |= per_cpu(gic_cpu_id, cpu);
+    }
+
+    return mask;
+}
+
 unsigned int gic_number_lines(void)
 {
     return gic.lines;
@@ -182,16 +208,18 @@  static hw_irq_controller gic_guest_irq_type = {
     .set_affinity = gic_irq_set_affinity,
 };
 
-/* needs to be called with gic.lock held */
+/*
+ * - needs to be called with gic.lock held
+ * - needs to be called with a valid cpu_mask, ie each cpu in the mask has
+ * already called gic_cpu_init
+ */
 static void gic_set_irq_properties(unsigned int irq, bool_t level,
                                    const cpumask_t *cpu_mask,
                                    unsigned int priority)
 {
     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 +328,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 +461,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];
+    unsigned int mask = 0;
+    cpumask_t online_mask;
 
     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();