Re: [Xen-devel] [PATCH 16/38] arm: Add simple cpu_{sibling, core}_mask

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

(Keir, this slightly touches common code...)

On Thu, 2012-06-07 at 10:08 +0100, Tim Deegan wrote:
> At 15:39 +0000 on 01 Jun (1338565185), Ian Campbell wrote:
> > @@ -230,6 +230,13 @@ void __init start_xen(unsigned long boot_phys_offset,
> >          }
> >      }
> >  
> > +    if ( !zalloc_cpumask_var(&per_cpu(cpu_sibling_mask, 0)) ||
> > +         !zalloc_cpumask_var(&per_cpu(cpu_core_mask, 0)) )
> > +        BUG();
> > +
> > +    cpumask_clear(per_cpu(cpu_sibling_mask, 0));
> > +    cpumask_clear(per_cpu(cpu_core_mask, 0));
> 
> Aren't these clear()s noops?  

Yes, they were also incorrect since a CPU is it's own sibling and shares
a core with itself. Otherwise all manner of weirdness ensues (see commit
message). These also need to be setup on all CPUs.

I replaced this patch with the following.

Ian.

8<------------------------------------------------------

>From e980ca1ec9bf92b2f1255ac5222b1da1292f9f72 Mon Sep 17 00:00:00 2001
From: Ian Campbell <ian.campbell@xxxxxxxxxx>
Date: Mon, 14 May 2012 12:25:31 +0100
Subject: [PATCH] arm: Add simple cpu_{sibling,core}_mask

This needs to be done for all cpus. The allocations require smp_prepare_cpus
to be called a bit later on.

In a previous version of this patch these maps were being zeroed (instead of
setting the CPU itself in them). This in turn causes cpumask_first to return
NR_CPUS, which in turn was causing default_vcpu0_location to misbehave and
read off the end of its cnt array. Add a couple of asserts to catch this in
the future.

Signed-off-by: Ian Campbell <ian.campbell@xxxxxxxxxx>
---
 xen/arch/arm/dummy.S   |    2 --
 xen/arch/arm/setup.c   |    4 ++--
 xen/arch/arm/smpboot.c |   21 +++++++++++++++++++++
 xen/common/domctl.c    |    2 ++
 4 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/dummy.S b/xen/arch/arm/dummy.S
index c001e8d..03f7489 100644
--- a/xen/arch/arm/dummy.S
+++ b/xen/arch/arm/dummy.S
@@ -7,8 +7,6 @@ x:      .word 0xe7f000f0 /* Undefined instruction */
 x:     mov pc, lr
        
 /* SMP support */
-DUMMY(per_cpu__cpu_core_mask);
-DUMMY(per_cpu__cpu_sibling_mask);
 DUMMY(node_online_map);
 DUMMY(smp_send_state_dump);
 
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c
index 81ababb..d6c0178 100644
--- a/xen/arch/arm/setup.c
+++ b/xen/arch/arm/setup.c
@@ -173,8 +173,6 @@ void __init start_xen(unsigned long boot_phys_offset,
     set_current((struct vcpu *)0xfffff000); /* debug sanity */
     idle_vcpu[0] = current;
 
-    smp_prepare_cpus(cpus);
-
     init_xen_time();
 
     setup_mm(atag_paddr, fdt_size);
@@ -214,6 +212,8 @@ void __init start_xen(unsigned long boot_phys_offset,
 
     local_irq_enable();
 
+    smp_prepare_cpus(cpus);
+
     initialize_keytable();
 
     console_init_postirq();
diff --git a/xen/arch/arm/smpboot.c b/xen/arch/arm/smpboot.c
index ea05afc..6463a8d 100644
--- a/xen/arch/arm/smpboot.c
+++ b/xen/arch/arm/smpboot.c
@@ -52,6 +52,23 @@ unsigned long __initdata ready_cpus = 0;
 
 /* ID of the PCPU we're running on */
 DEFINE_PER_CPU(unsigned int, cpu_id);
+/* XXX these seem awfully x86ish... */
+/* representing HT siblings of each logical CPU */
+DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, cpu_sibling_mask);
+/* representing HT and core siblings of each logical CPU */
+DEFINE_PER_CPU_READ_MOSTLY(cpumask_var_t, cpu_core_mask);
+
+static void setup_cpu_sibling_map(int cpu)
+{
+    if ( !zalloc_cpumask_var(&per_cpu(cpu_sibling_mask, cpu)) ||
+         !zalloc_cpumask_var(&per_cpu(cpu_core_mask, cpu)) )
+        panic("No memory for CPU sibling/core maps\n");
+
+    /* A CPU is a sibling with itself and is always on its own core. */
+    cpumask_set_cpu(cpu, per_cpu(cpu_sibling_mask, cpu));
+    cpumask_set_cpu(cpu, per_cpu(cpu_core_mask, cpu));
+}
+
 
 void __init
 smp_prepare_cpus (unsigned int max_cpus)
@@ -65,6 +82,8 @@ smp_prepare_cpus (unsigned int max_cpus)
     for ( i = 0; i < max_cpus; i++ )
         cpumask_set_cpu(i, &cpu_possible_map);
     cpumask_copy(&cpu_present_map, &cpu_possible_map);
+
+    setup_cpu_sibling_map(0);
 }
 
 void __init
@@ -115,6 +134,8 @@ void __cpuinit start_secondary(unsigned long 
boot_phys_offset,
 
     set_current(idle_vcpu[cpuid]);
 
+    setup_cpu_sibling_map(cpuid);
+
     /* Run local notifiers */
     notify_cpu_starting(cpuid);
     wmb();
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 9f1a9ad..c1acd1d 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -190,10 +190,12 @@ static unsigned int default_vcpu0_location(cpumask_t 
*online)
      */
     cpumask_copy(&cpu_exclude_map, per_cpu(cpu_sibling_mask, 0));
     cpu = cpumask_first(&cpu_exclude_map);
+    ASSERT(cpu < nr_cpus);
     if ( cpumask_weight(&cpu_exclude_map) > 1 )
         cpu = cpumask_next(cpu, &cpu_exclude_map);
     for_each_cpu(i, online)
     {
+        ASSERT(i < nr_cpus);
         if ( cpumask_test_cpu(i, &cpu_exclude_map) )
             continue;
         if ( (i == cpumask_first(per_cpu(cpu_sibling_mask, i))) &&
-- 
1.7.9.1




_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel