From mboxrd@z Thu Jan 1 00:00:00 1970 From: Keir Fraser Subject: Re: Ping: [PATCH] x86/IO-APIC: fix guest RTE write corner cases Date: Tue, 14 May 2013 16:17:37 +0100 Message-ID: References: <51926BBC02000078000D6148@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <51926BBC02000078000D6148@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich , Andrew Cooper , xiantao.zhang@intel.com Cc: xen-devel List-Id: xen-devel@lists.xenproject.org On 14/05/2013 15:52, "Jan Beulich" wrote: >>>> On 08.05.13 at 14:51, "Jan Beulich" wrote: >> This fixes two regressions from c/s 20143:a7de5bd776ca ("x86: Make the >> hypercall PHYSDEVOP_alloc_irq_vector hypercall dummy"): >> >> For one, IRQs that had their vector set up by Xen internally without a >> handler ever having got set (e.g. via "com=..." without a matching >> consumer option like "console=com") would wrongly call >> add_pin_to_irq() here, triggering the BUG_ON() in that function. >> >> Second, when assign_irq_vector() fails this addition to irq_2_pin[] >> needs to be undone. >> >> In the context of this I'm also surprised that the irq_2_pin[] >> manipulations here occur without any lock, i.e. rely on Dom0 to do >> some sort of serialization. >> >> Signed-off-by: Jan Beulich > > No-one having any opinion? I'm hesitant to commit changes like > this without anyone having said any word... Yuk, well, the motivation is good and the code looks sane, so... Acked-by: Keir Fraser > Jan > >> --- a/xen/arch/x86/io_apic.c >> +++ b/xen/arch/x86/io_apic.c >> @@ -133,6 +133,37 @@ static void add_pin_to_irq(unsigned int >> share_vector_maps(irq_2_pin[irq].apic, apic); >> } >> >> +static void remove_pin_from_irq(unsigned int irq, int apic, int pin) >> +{ >> + struct irq_pin_list *entry, *prev; >> + >> + for (entry = &irq_2_pin[irq]; ; entry = &irq_2_pin[entry->next]) { >> + if ((entry->apic == apic) && (entry->pin == pin)) >> + break; >> + BUG_ON(!entry->next); >> + } >> + >> + entry->pin = entry->apic = -1; >> + >> + if (entry != &irq_2_pin[irq]) { >> + /* Removed entry is not at head of list. */ >> + prev = &irq_2_pin[irq]; >> + while (&irq_2_pin[prev->next] != entry) >> + prev = &irq_2_pin[prev->next]; >> + prev->next = entry->next; >> + } else if (entry->next) { >> + /* Removed entry is at head of multi-item list. */ >> + prev = entry; >> + entry = &irq_2_pin[entry->next]; >> + *prev = *entry; >> + entry->pin = entry->apic = -1; >> + } else >> + return; >> + >> + entry->next = irq_2_pin_free_entry; >> + irq_2_pin_free_entry = entry - irq_2_pin; >> +} >> + >> /* >> * Reroute an IRQ to a different pin. >> */ >> @@ -2280,7 +2311,7 @@ int ioapic_guest_read(unsigned long phys >> >> int ioapic_guest_write(unsigned long physbase, unsigned int reg, u32 val) >> { >> - int apic, pin, irq, ret, vector, pirq; >> + int apic, pin, irq, ret, pirq; >> struct IO_APIC_route_entry rte = { 0 }; >> unsigned long flags; >> struct irq_desc *desc; >> @@ -2348,13 +2379,25 @@ int ioapic_guest_write(unsigned long phy >> return 0; >> } >> >> - if ( desc->arch.vector <= 0 || desc->arch.vector > LAST_DYNAMIC_VECTOR ) >> { >> - add_pin_to_irq(irq, apic, pin); >> - vector = assign_irq_vector(irq, NULL); >> - if ( vector < 0 ) >> - return vector; >> + if ( desc->arch.vector <= 0 || desc->arch.vector > LAST_DYNAMIC_VECTOR ) >> + { >> + int vector = desc->arch.vector; >> + >> + if ( vector < FIRST_HIPRIORITY_VECTOR ) >> + add_pin_to_irq(irq, apic, pin); >> + else >> + desc->arch.vector = IRQ_VECTOR_UNASSIGNED; >> + ret = assign_irq_vector(irq, NULL); >> + if ( ret < 0 ) >> + { >> + if ( vector < FIRST_HIPRIORITY_VECTOR ) >> + remove_pin_from_irq(irq, apic, pin); >> + else >> + desc->arch.vector = vector; >> + return ret; >> + } >> >> - printk(XENLOG_INFO "allocated vector %02x for irq %d\n", vector, >> irq); >> + printk(XENLOG_INFO "allocated vector %02x for irq %d\n", ret, irq); >> } >> spin_lock(&dom0->event_lock); >> ret = map_domain_pirq(dom0, pirq, irq, > > >