From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jeremy Fitzhardinge Subject: Re: [PATCH] xen: do not unmask disabled IRQ on eoi. Date: Fri, 15 Oct 2010 10:33:17 -0700 Message-ID: <4CB8905D.5020407@goop.org> References: <1287139966-19391-2-git-send-email-ian.campbell@citrix.com> <1287160335.2003.7973.camel@zakaz.uk.xensource.com> <1287160787.2003.7979.camel@zakaz.uk.xensource.com> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit 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: Stefano Stabellini Cc: Ian Campbell , "xen-devel@lists.xensource.com" List-Id: xen-devel@lists.xenproject.org On 10/15/2010 10:03 AM, Stefano Stabellini wrote: > On Fri, 15 Oct 2010, Ian Campbell wrote: >> On Fri, 2010-10-15 at 17:32 +0100, Ian Campbell wrote: >>> On Fri, 2010-10-15 at 17:12 +0100, Stefano Stabellini wrote: >>>> Reading a little bit more about the fasteoi handler, it seems to me that >>>> a better solution would be not to mask_evtchn and clear_evtchn in >>>> __xen_evtchn_do_upcall, but just clear_evtchn in the eoi handler. >>>> This would also be much more similar to the way the fasteoi handler is >>>> used by the ioapic chip. >>>> The appended patch has been only smoked tested. >>> Fails to boot dom0 for me. >>> >>> irq 20: nobody cared (try booting with the "irqpoll" option) >>> Pid: 0, comm: swapper Not tainted 2.6.32.24-x86_32p-xen0-01025-g5238c00-dirty #205 >>> Call Trace: >>> [] ? printk+0x18/0x1c >>> [] __report_bad_irq+0x27/0x90 >>> [] note_interrupt+0x15e/0x1a0 >>> [] ? _raw_spin_unlock+0x89/0xa0 >>> [] handle_fasteoi_irq+0xac/0xd0 >>> [] __xen_evtchn_do_upcall+0x1cf/0x1f0 >>> [] xen_evtchn_do_upcall+0x28/0x40 >>> [] xen_do_upcall+0x7/0xc >>> [] ? hypercall_page+0x3a7/0x1010 >>> [] ? xen_safe_halt+0x12/0x30 >>> [] xen_idle+0x5f/0x80 >>> [] cpu_idle+0x49/0xa0 >>> [] ? xen_save_fl_direct+0x0/0xd >>> [] rest_init+0x53/0x60 >>> [] start_kernel+0x382/0x39f >>> [] ? unknown_bootoption+0x0/0x1e4 >>> [] i386_start_kernel+0x7e/0xa8 >>> [] xen_start_kernel+0x561/0x5d5 >>> handlers: >>> [] (ata_sff_interrupt+0x0/0xd0) >>> [] (usb_hcd_irq+0x0/0xe0) >>> Disabling IRQ #20 >>> >>> I think because you missed the pirq case. >> Mixed the patch up with some other patch. Lets try that again... > Yes I did. I think this version is better because in case the pirq is > already masked in pirq_eoi it makes sure that it stays that way, thus > preventing cases like the one you described before from happening. This looks plausible in general, but... > diff --git a/drivers/xen/events.c b/drivers/xen/events.c > index 175e931..9fb6e0f 100644 > --- a/drivers/xen/events.c > +++ b/drivers/xen/events.c > @@ -441,16 +441,23 @@ static void pirq_eoi(unsigned int irq) > struct irq_info *info = info_for_irq(irq); > struct physdev_eoi eoi = { .irq = info->u.pirq.gsi }; > bool need_eoi; > + struct shared_info *s = HYPERVISOR_shared_info; > + int need_mask = 0; > > need_eoi = pirq_needs_eoi(irq); > > - if (!need_eoi || !pirq_eoi_does_unmask) > - unmask_evtchn(info->evtchn); > + if (need_eoi && pirq_eoi_does_unmask) > + need_mask = sync_test_and_set_bit(info->evtchn, s->evtchn_mask); > > if (need_eoi) { > int rc = HYPERVISOR_physdev_op(PHYSDEVOP_eoi, &eoi); > WARN_ON(rc); > + > + if (need_mask) > + mask_evtchn(info->evtchn); > } > + > + clear_evtchn(info->evtchn); This is all a mess. This "auto unmask pirq eoi with shared memory" thing seems pretty pointless if we need to go to all this effort to remask again, and I didn't see any other point to it aside from that. So if this works, we may as well revert "xen/events: use PHYSDEVOP_pirq_eoi_gmfn to get pirq need-EOI info". > } > > static void pirq_query_unmask(int irq) > @@ -1074,9 +1081,6 @@ static void __xen_evtchn_do_upcall(struct pt_regs *regs) > int irq = evtchn_to_irq[port]; > struct irq_desc *desc; > > - mask_evtchn(port); > - clear_evtchn(port); > - > if (irq != -1) { > desc = irq_to_desc(irq); > if (desc) > @@ -1202,7 +1206,7 @@ static void ack_dynirq(unsigned int irq) > move_masked_irq(irq); > > if (VALID_EVTCHN(evtchn)) > - unmask_evtchn(evtchn); > + clear_evtchn(evtchn); > } > > static int retrigger_irq(unsigned int irq) > @@ -1384,7 +1388,6 @@ void xen_irq_resume(void) > static struct irq_chip xen_dynamic_chip __read_mostly = { > .name = "xen-dyn", > > - .disable = mask_irq, > .mask = mask_irq, > .unmask = unmask_irq, > > @@ -1412,7 +1415,6 @@ static struct irq_chip xen_pirq_chip __read_mostly = { > .enable = pirq_eoi, > .unmask = unmask_irq, > > - .disable = mask_irq, > .mask = mask_irq, > > .eoi = ack_pirq, > J