From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH] x86/IO-APIC: don't create pIRQ mapping from masked RTE Date: Fri, 21 Aug 2015 15:58:08 +0100 Message-ID: <55D73C80.8050908@citrix.com> References: <55D7005C020000780009C57C@prv-mh.provo.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1ZSnll-0001VE-0Q for xen-devel@lists.xenproject.org; Fri, 21 Aug 2015 14:58:13 +0000 In-Reply-To: <55D7005C020000780009C57C@prv-mh.provo.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 , xen-devel Cc: Keir Fraser , Wei Liu List-Id: xen-devel@lists.xenproject.org On 21/08/15 09:41, Jan Beulich wrote: > While moving our XenoLinux patches to 4.2-rc I noticed bogus "already > mapped" messages resulting from Linux (legitimately) writing RTEs with > only the mask bit set. Clearly we shouldn't even attempt to create a > pIRQ <-> IRQ mapping from such RTEs. Oops. Agreed. > > In the course of this I also found that the respective message isn't > really useful without also printing the pre-existing mapping. And I > noticed that map_domain_pirq() allowed IRQ0 to get through, despite us > never allowing and domain to control that interrupt. s/and/a/ ? (I can't quite parse the original statement) Also, doesn't irq_access_permitted() catch the irq0 case? > > Signed-off-by: Jan Beulich Reviewed-by: Andrew Cooper with one suggestion > > --- a/xen/arch/x86/io_apic.c > +++ b/xen/arch/x86/io_apic.c > @@ -2371,9 +2371,14 @@ int ioapic_guest_write(unsigned long phy > * pirq and irq mapping. Where the GSI is greater than 256, we assume > * that dom0 pirq == irq. > */ > - pirq = (irq >= 256) ? irq : rte.vector; > - if ( (pirq < 0) || (pirq >= hardware_domain->nr_pirqs) ) > - return -EINVAL; > + if ( !rte.mask ) > + { > + pirq = (irq >= 256) ? irq : rte.vector; > + if ( pirq >= hardware_domain->nr_pirqs ) > + return -EINVAL; > + } > + else > + pirq = -1; > > if ( desc->action ) > { > @@ -2408,12 +2413,15 @@ int ioapic_guest_write(unsigned long phy > > printk(XENLOG_INFO "allocated vector %02x for irq %d\n", ret, irq); > } > - spin_lock(&hardware_domain->event_lock); > - ret = map_domain_pirq(hardware_domain, pirq, irq, > - MAP_PIRQ_TYPE_GSI, NULL); > - spin_unlock(&hardware_domain->event_lock); > - if ( ret < 0 ) > - return ret; > + if ( pirq >= 0 ) > + { > + spin_lock(&hardware_domain->event_lock); > + ret = map_domain_pirq(hardware_domain, pirq, irq, > + MAP_PIRQ_TYPE_GSI, NULL); > + spin_unlock(&hardware_domain->event_lock); > + if ( ret < 0 ) > + return ret; > + } > > spin_lock_irqsave(&ioapic_lock, flags); > /* Set the correct irq-handling type. */ > --- a/xen/arch/x86/irq.c > +++ b/xen/arch/x86/irq.c > @@ -1906,7 +1906,7 @@ int map_domain_pirq( > if ( !irq_access_permitted(current->domain, irq)) > return -EPERM; > I would be tempted to put a comment here stating that irq0 is definitely a xen-reserved irq. Otherwise, it is odd to have the mismatch between pirq and irq. ~Andrew > - if ( pirq < 0 || pirq >= d->nr_pirqs || irq < 0 || irq >= nr_irqs ) > + if ( pirq < 0 || pirq >= d->nr_pirqs || irq <= 0 || irq >= nr_irqs ) > { > dprintk(XENLOG_G_ERR, "dom%d: invalid pirq %d or irq %d\n", > d->domain_id, pirq, irq); > @@ -1919,8 +1919,9 @@ int map_domain_pirq( > if ( (old_irq > 0 && (old_irq != irq) ) || > (old_pirq && (old_pirq != pirq)) ) > { > - dprintk(XENLOG_G_WARNING, "dom%d: pirq %d or irq %d already mapped\n", > - d->domain_id, pirq, irq); > + dprintk(XENLOG_G_WARNING, > + "dom%d: pirq %d or irq %d already mapped (%d,%d)\n", > + d->domain_id, pirq, irq, old_pirq, old_irq); > return 0; > } > > > >