Re: [PATCH] Only include online cpus in cpu_mask_to_apicid_flat

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

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

On Tuesday 31 August 2010 18:46:28 Keir Fraser wrote:
> On 31/08/2010 09:55, "Jan Beulich" <JBeulich@novell.com> wrote:
> >> In fact, setup_ioapic_dest() would be called to reprogram the IOAPIC
> >> redirection
> >> table to follow "irq_cfg->cpu_mask", after SMP initialization work was
> >> done. So I think the better choice is to keep the original value in
> >> irq_cfg-
> >> 
> >>> cpu_mask, and just make sure the value we wrote to the IOAPIC
> >>> redirection
> >> 
> >> table
> >> is valid. Then modifying cpu_mask_to_apicid_flat() seems like a better
> >> idea.
> > 
> > Why would you need to modify only this function, but not the other
> > variants? If a CPU in the passed in mask can be offline, then
> > first_cpu() (as used in the other variants) can return an offline CPU,
> > and you don't want to program such into an RTE.

Yes, here is the patch with modification of other variants.
> 
> Indeed, also all other assignments to irq_cfg->cpu_mask include only online
> CPUs, so the current code is only being consistent in that respect. 

After reading the code, I think it may not that consistent. For example, seems 
set_desc_affinity() and __clear_irq_vector()(as well as many other functions) won't 
assume cfg->cpu_mask contained cpus are all online. 

> And in
> the general case (even if not specifically for IRQ0) that is important
> because IDT vectors are not allocated on offline CPUs, and so we could
> otherwise end up with CPUs coming online and finding they are in multiple
> irq_cfg's with the same vector!

I think this still apply, because we still don't allocate vectors for offline CPUs. 
When we allocate vectors, we would check cpu_online_map.

> Also the PIT is usually disabled after boot
> on Xen and so it being restricted to only CPU0 would really not matter. I
> think we should leave the code as is.

But HPET would still being used, which replaced PIT and using IRQ0.

--
regards
Yang, Sheng

> 
>  -- Keir

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

diff --git a/xen/arch/x86/genapic/delivery.c b/xen/arch/x86/genapic/delivery.c
--- a/xen/arch/x86/genapic/delivery.c
+++ b/xen/arch/x86/genapic/delivery.c
@@ -36,9 +36,10 @@
 	return cpu_online_map;
 } 
 
+/* Cover only online cpus */
 unsigned int cpu_mask_to_apicid_flat(cpumask_t cpumask)
 {
-	return cpus_addr(cpumask)[0]&0xFF;
+	return cpus_addr(cpumask)[0] & cpus_addr(cpu_online_map)[0] & 0xFF;
 }
 
 /*
@@ -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);
 }
diff --git a/xen/arch/x86/genapic/x2apic.c b/xen/arch/x86/genapic/x2apic.c
--- a/xen/arch/x86/genapic/x2apic.c
+++ b/xen/arch/x86/genapic/x2apic.c
@@ -101,12 +101,26 @@
 
 unsigned int cpu_mask_to_apicid_x2apic_phys(cpumask_t cpumask)
 {
-    return cpu_physical_id(first_cpu(cpumask));
+    int cpu;
+
+    for_each_cpu_mask( cpu, cpumask ) 
+    {
+        if ( cpu_online(cpu) )
+            break;
+    }
+    return cpu_physical_id(cpu);
 }
 
 unsigned int cpu_mask_to_apicid_x2apic_cluster(cpumask_t cpumask)
 {
-    return cpu_2_logical_apicid[first_cpu(cpumask)];
+    int cpu;
+
+    for_each_cpu_mask( cpu, cpumask ) 
+    {
+        if ( cpu_online(cpu) )
+            break;
+    }
+    return cpu_2_logical_apicid[cpu];
 }
 
 static void __send_IPI_mask_x2apic(
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;

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

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