Re: [Xen-devel] [PATCH] xen: do not unmask disabled IRQ on eoi.

[Jeremy Fitzhardinge <jeremy@xxxxxxxx>]

 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:
>>>          [<c1377034>] ? printk+0x18/0x1c
>>>          [<c106f5c7>] __report_bad_irq+0x27/0x90
>>>          [<c106f78e>] note_interrupt+0x15e/0x1a0
>>>          [<c1169669>] ? _raw_spin_unlock+0x89/0xa0
>>>          [<c106febc>] handle_fasteoi_irq+0xac/0xd0
>>>          [<c11a6a5f>] __xen_evtchn_do_upcall+0x1cf/0x1f0
>>>          [<c11a6ab8>] xen_evtchn_do_upcall+0x28/0x40
>>>          [<c100b767>] xen_do_upcall+0x7/0xc
>>>          [<c10013a7>] ? hypercall_page+0x3a7/0x1010
>>>          [<c1007472>] ? xen_safe_halt+0x12/0x30
>>>          [<c100397f>] xen_idle+0x5f/0x80
>>>          [<c1009c49>] cpu_idle+0x49/0xa0
>>>          [<c1007c40>] ? xen_save_fl_direct+0x0/0xd
>>>          [<c135d913>] rest_init+0x53/0x60
>>>          [<c1543975>] start_kernel+0x382/0x39f
>>>          [<c15433be>] ? unknown_bootoption+0x0/0x1e4
>>>          [<c154308f>] i386_start_kernel+0x7e/0xa8
>>>          [<c1546471>] xen_start_kernel+0x561/0x5d5
>>>         handlers:
>>>         [<c1223500>] (ata_sff_interrupt+0x0/0xd0)
>>>         [<c128b2b0>] (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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel