Re: [PATCH] Only include online cpus in cpu_mask_to_apicid_flat

["Yang, Sheng" <sheng.yang@intel.com>]

[-- Attachment #1: Type: Text/Plain, Size: 1218 bytes --]

On Wednesday 01 September 2010 17:14:52 Jan Beulich wrote:
> >>> On 01.09.10 at 05:39, "Yang, Sheng" <sheng.yang@intel.com> wrote:
> > Yes, here is the patch with modification of other variants.
> 
> If indeed an adjustment like this is needed, then this (and other similar
> instances)
> 
> >@@ -71,6 +72,11 @@
> >
> > unsigned int cpu_mask_to_apicid_phys(cpumask_t cpumask)
> > {
> >
> >+	int cpu;
> >
> > 	/* As we are using single CPU as destination, pick only one CPU here */
> >
> >-	return cpu_physical_id(first_cpu(cpumask));
> >+	for_each_cpu_mask(cpu, cpumask) {
> >+		if (cpu_online(cpu))
> >+			break;
> >+	}
> >+	return cpu_physical_id(cpu);
> >
> > }
> 
> is both insufficient: You need to handle the case where you don't
> find any online CPU in the mask (at least by adding a respective
> BUG_ON()).

Yes, BUG_ON() is needed.
> 
> But I tend to agree with Keir that this shouldn't be done here -
> these functions are simple accessors, which shouldn't enforce
> any policy. Higher level code, if it doesn't already, should be
> adjusted to never allow offline CPUs to slip through.

Well, I think it's acceptable to add a wrap function for it. So how about this 
one?

--
regards
Yang, Sheng

> 
> Jan

[-- Attachment #2: dest_fix.patch --]
[-- Type: text/x-patch, Size: 3703 bytes --]

diff --git a/xen/arch/x86/io_apic.c b/xen/arch/x86/io_apic.c
--- a/xen/arch/x86/io_apic.c
+++ b/xen/arch/x86/io_apic.c
@@ -490,7 +490,7 @@
     cpus_copy(desc->affinity, mask);
     cpus_and(dest_mask, desc->affinity, cfg->cpu_mask);
 
-    return cpu_mask_to_apicid(dest_mask);
+    return cpu_mask_to_apicid_online(dest_mask);
 }
 
 static void
@@ -1003,7 +1003,7 @@
             }
             cfg = irq_cfg(irq);
             SET_DEST(entry.dest.dest32, entry.dest.logical.logical_dest,
-                cpu_mask_to_apicid(cfg->cpu_mask));
+                cpu_mask_to_apicid_online(cfg->cpu_mask));
             spin_lock_irqsave(&ioapic_lock, flags);
             io_apic_write(apic, 0x11+2*pin, *(((int *)&entry)+1));
             io_apic_write(apic, 0x10+2*pin, *(((int *)&entry)+0));
@@ -1038,7 +1038,7 @@
     entry.dest_mode = INT_DEST_MODE;
     entry.mask = 0;					/* unmask IRQ now */
     SET_DEST(entry.dest.dest32, entry.dest.logical.logical_dest,
-        cpu_mask_to_apicid(TARGET_CPUS));
+        cpu_mask_to_apicid_online(TARGET_CPUS));
     entry.delivery_mode = INT_DELIVERY_MODE;
     entry.polarity = 0;
     entry.trigger = 0;
@@ -2253,7 +2253,7 @@
     entry.delivery_mode = INT_DELIVERY_MODE;
     entry.dest_mode = INT_DEST_MODE;
     SET_DEST(entry.dest.dest32, entry.dest.logical.logical_dest,
-        cpu_mask_to_apicid(TARGET_CPUS));
+        cpu_mask_to_apicid_online(TARGET_CPUS));
     entry.trigger = edge_level;
     entry.polarity = active_high_low;
     entry.mask  = 1;
@@ -2446,7 +2446,7 @@
     rte.vector = cfg->vector;
 
     SET_DEST(rte.dest.dest32, rte.dest.logical.logical_dest,
-        cpu_mask_to_apicid(cfg->cpu_mask));
+        cpu_mask_to_apicid_online(cfg->cpu_mask));
 
     io_apic_write(apic, 0x10 + 2 * pin, *(((int *)&rte) + 0));
     io_apic_write(apic, 0x11 + 2 * pin, *(((int *)&rte) + 1));
@@ -2584,3 +2584,12 @@
     printk(XENLOG_INFO "IRQ limits: %u GSI, %u MSI/MSI-X\n",
            nr_irqs_gsi, nr_irqs - nr_irqs_gsi);
 }
+
+unsigned int cpu_mask_to_apicid_online(cpumask_t cpu_mask)
+{
+    cpumask_t dest;
+    cpus_and(dest, cpu_mask, cpu_online_map);
+    BUG_ON(cpus_empty(dest));
+    return cpu_mask_to_apicid(dest);
+}
+
diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -86,14 +86,14 @@
     cpus_and(online_mask, cpu_mask, cpu_online_map);
     if (cpus_empty(online_mask))
         return -EINVAL;
-    if ((cfg->vector == vector) && cpus_equal(cfg->cpu_mask, online_mask))
+    if ((cfg->vector == vector) && cpus_equal(cfg->cpu_mask, cpu_mask))
         return 0;
     if (cfg->vector != IRQ_VECTOR_UNASSIGNED) 
         return -EBUSY;
     for_each_cpu_mask(cpu, online_mask)
         per_cpu(vector_irq, cpu)[vector] = irq;
     cfg->vector = vector;
-    cfg->cpu_mask = online_mask;
+    cfg->cpu_mask = cpu_mask;
     irq_status[irq] = IRQ_USED;
     if (IO_APIC_IRQ(irq))
         irq_vector[irq] = vector;
diff --git a/xen/arch/x86/msi.c b/xen/arch/x86/msi.c
--- a/xen/arch/x86/msi.c
+++ b/xen/arch/x86/msi.c
@@ -133,7 +133,7 @@
 
     if ( vector ) {
 
-        dest = cpu_mask_to_apicid(domain);
+        dest = cpu_mask_to_apicid_online(domain);
 
         msg->address_hi = MSI_ADDR_BASE_HI;
         msg->address_lo =
diff --git a/xen/include/asm-x86/genapic.h b/xen/include/asm-x86/genapic.h
--- a/xen/include/asm-x86/genapic.h
+++ b/xen/include/asm-x86/genapic.h
@@ -52,6 +52,8 @@
 extern const struct genapic apic_x2apic_phys;
 extern const struct genapic apic_x2apic_cluster;
 
+unsigned int cpu_mask_to_apicid_online(cpumask_t cpu_mask);
+
 void init_apic_ldr_flat(void);
 void clustered_apic_check_flat(void);
 cpumask_t target_cpus_flat(void);

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel