From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Yang, Sheng" Subject: Re: [PATCH] Only include online cpus in cpu_mask_to_apicid_flat Date: Wed, 1 Sep 2010 11:39:46 +0800 Message-ID: <201009011139.47130.sheng.yang@intel.com> References: Mime-Version: 1.0 Content-Type: Multipart/Mixed; boundary="Boundary-00=_DscfMN38ix/9Ll+" Return-path: In-Reply-To: List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Keir Fraser Cc: "xen-devel@lists.xensource.com" , Jan Beulich List-Id: xen-devel@lists.xenproject.org --Boundary-00=_DscfMN38ix/9Ll+ Content-Type: Text/Plain; charset="us-ascii" Content-Transfer-Encoding: 7bit On Tuesday 31 August 2010 18:46:28 Keir Fraser wrote: > On 31/08/2010 09:55, "Jan Beulich" 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 --Boundary-00=_DscfMN38ix/9Ll+ Content-Type: text/x-patch; charset="UTF-8"; name="dest_fix.patch" Content-Transfer-Encoding: 7bit Content-Disposition: attachment; filename="dest_fix.patch" 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; --Boundary-00=_DscfMN38ix/9Ll+ Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xensource.com http://lists.xensource.com/xen-devel --Boundary-00=_DscfMN38ix/9Ll+--