Only include online cpus in cpu_mask_to_apicid_flat
PATCH 1/1 Only include online cpus in cpu_mask_to_apicid_flat
The former fix of __bind_irq_vector() can ensure the destination field in IOAPIC redirection table is valid by limiting the cpu_mask to the online cpus. But there is another issue that some interrupts(timer and serial) would like to be delivered to all online cpus with LPR delivery mode. But at the time of bind_irq_vector() called, there were only one cpu(BSP) was online, so the interrupts would always be delivered to BSP. This method can work but not the best one.
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-
Quoted 1 line
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.
This patch would revert the fix of __bind_irq_vector(), and modify cpu_mask_to_apicid_flat() to contain only online CPUs.
--
regards
Yang, Sheng
xen/arch/x86/genapic/delivery.c +2 -1
@@ -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 ; }/*
xen/arch/x86/irq.c +2 -2
@@ -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;
xen/arch/x86/genapic/delivery.c +2 -1
@@ -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 ; }/*
xen/arch/x86/irq.c +2 -2
@@ -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;
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel
Quoted 12 lines
Quoted 1 line
Quoted 1 line · Sheng
The former fix of
__bind_irq_vector()can ensure the destination field in IOAPIC
redirection table is valid by limiting thecpu_maskto the online cpus. But there
is another issue that some interrupts(timer and serial) would like to be delivered to all online cpus with LPR delivery mode. But at the time ofbind_irq_vector()called, there were only one cpu(BSP) was online, so the interrupts would always be delivered to BSP. This method can work but not the best
one.
Quoted 7 lines
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-Quoted 1 line
cpu_mask, and just make sure the value we wrote to the IOAPIC redirectiontable
is valid. Then modifyingcpu_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.
Quoted 2 lines
This patch would revert the fix of
__bind_irq_vector(), and modifycpu_mask_to_apicid_flat()to contain only online CPUs.
Jan
Quoted 4 lines · Jan Beulich
Quoted 4 lines
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-
Quoted 8 lines
Quoted 3 lines
Quoted 1 line
cpu_mask, and just make sure the value we wrote to the IOAPIC redirectiontable
is valid. Then modifyingcpu_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.
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. 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! 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.
-- Keir
Quoted 10 lines · On Tuesday 31 August 2010 18:46:28 Keir Fraser wrote:
Quoted 9 lines · Jan Beulich
Quoted 9 lines
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-Quoted 2 lines
cpu_mask, and just make sure the value we wrote to the IOAPIC redirection
Quoted 8 lines
Quoted 8 lines
Quoted 3 lines
table
is valid. Then modifyingcpu_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.
Quoted 3 lines
Indeed, also all other assignments to
irq_cfg->cpu_maskinclude 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.
Quoted 5 lines
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.
Quoted 3 lines
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
Quoted 2 lines
-- Keir
xen/arch/x86/genapic/delivery.c +8 -2
@@ -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); }
xen/arch/x86/genapic/x2apic.c +16 -2
@@ -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(
xen/arch/x86/irq.c +2 -2
@@ -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;
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel
Quoted 2 lines
Quoted 1 line
Quoted 1 line · Sheng
Yes, here is the patch with modification of other variants.
If indeed an adjustment like this is needed, then this (and other similar instances)
xen/arch/x86/genapic/delivery.c line 82
Quoted 13 lines
xen/arch/x86/genapic/delivery.c +6
-1@@ -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()).
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.
Jan
Quoted 22 lines · On Wednesday 01 September 2010 17:14:52 Jan Beulich wrote:
Quoted 2 lines
Quoted 1 line
Quoted 1 line · Sheng
Yes, here is the patch with modification of other variants.
If indeed an adjustment like this is needed, then this (and other similar instances)
Quoted 16 lines
(quoted diff) +6
-1@@ -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);
Quoted 5 lines
Quoted 1 line
}
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.
Quoted 5 lines
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
Quoted 2 lines
Jan
xen/arch/x86/io_apic.c +14 -5
@@ -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); +} +
xen/arch/x86/irq.c +2 -2
@@ -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;
xen/arch/x86/msi.c +1 -1
@@ -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 =
xen/include/asm-x86/genapic.h +2 -0
@@ -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 );
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel
Quoted 32 lines · Sheng
Quoted 23 lines · On Wednesday 01 September 2010 17:14:52 Jan Beulich wrote:
Quoted 2 lines
Quoted 1 line
Quoted 1 line · Sheng
Yes, here is the patch with modification of other variants.
If indeed an adjustment like this is needed, then this (and other similar instances)
Quoted 17 lines
(quoted diff) +1
-0@@ -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*/(quoted diff) +5
-1- return cpu_physical_id(first_cpu(cpumask)); + for_each_cpu_mask(cpu, cpumask) { + if (cpu_online(cpu)) + break; + } + return cpu_physical_id(cpu);Quoted 5 lines
Quoted 1 line
}
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.
Quoted 7 lines
Quoted 4 lines
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?
Keir & Jan, how do you think about this patchset?
If you still think we should never allow offline CPUs in the cpu_mask, then at least we need one patch to fix serial port IRQ's cpu_mask which was CPU_MASK_ALL(this fix would also result the serial interrupt delivery to CPU0).
--
regards
Yang, Sheng
xen/arch/x86/smpboot.c +1 -1
@@ -1015,7 +1015,7 @@ irq_vector[irq] =FIRST_HIPRIORITY_VECTOR + seridx +1 ;per_cpu (vector_irq, cpu)[FIRST_HIPRIORITY_VECTOR + seridx +1 ] = irq; irq_cfg[irq].vector =FIRST_HIPRIORITY_VECTOR + seridx +1 ; - irq_cfg[irq].cpu_mask = (cpumask_t )CPU_MASK_ALL ; + irq_cfg[irq].cpu_mask = cpu_online_map; }/* IPI for cleanuping vectors after irq move */
xen/arch/x86/io_apic.c +14 -5
@@ -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); +} +
xen/arch/x86/irq.c +2 -2
@@ -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;
xen/arch/x86/msi.c +1 -1
@@ -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 =
xen/include/asm-x86/genapic.h +2 -0
@@ -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 );
xen/arch/x86/smpboot.c +1 -1
@@ -1015,7 +1015,7 @@ irq_vector[irq] =FIRST_HIPRIORITY_VECTOR + seridx +1 ;per_cpu (vector_irq, cpu)[FIRST_HIPRIORITY_VECTOR + seridx +1 ] = irq; irq_cfg[irq].vector =FIRST_HIPRIORITY_VECTOR + seridx +1 ; - irq_cfg[irq].cpu_mask = (cpumask_t )CPU_MASK_ALL ; + irq_cfg[irq].cpu_mask = cpu_online_map; }/* IPI for cleanuping vectors after irq move */
_______________________________________________
Xen-devel mailing list
[email protected]
http://lists.xensource.com/xen-devel