Re: Re: Losing PS/2 Interrupts

["Jan Beulich" <JBeulich@novell.com>]

>>> On 23.05.11 at 19:28, Thomas Goetz <tcgoetz@gmail.com> wrote:

> On May 23, 2011, at 1:16 PM, Thomas Goetz wrote:
> 
>> 
>> On May 23, 2011, at 9:45 AM, Stefano Stabellini wrote:
>> 
>>> On Mon, 23 May 2011, Thomas Goetz wrote:
>>>> My assumption is that at the point that the i8042 driver reads the data 
> register a new interrupt happens. There is gap in
>>>> time between when the data register is read and when the event channel 
> pending state is cleared. Since the hypervisor
>>>> ACKed the previous real interrupt before delivering it to the guest, there 
> is nothing to stop the i8042 device from
>>>> interrupting immediately after the data register is read. If it interrupt 
> before the event channel pending state is
>>>> cleared, then it will not be delivered to the guest and the EOI mechanism 
> will be set up, but I haven't found anything in
>>>> that that will set up a delayed delivery of the second interrupt.
>>>> 
>>>> In this situation the i8042 device has every reason to believe the second 
> interrupt will be delivered. The previous
>>>> interrupt was received and handled. Nothing is masked.
>>>> 
>>>> Am I missing something?
>>> 
>>> 
>>> I am assuming you have the latest version of my fixes to
>>> drivers/xen/events.c
>> 
>> I'll have a version ported from your 2.6.39 tree to my 2.6.38 tree. I'll 
> update my copy of your tree and make sure it's up to date.
>> 
>>> 
>>> The problem you are describing shouldn't happen because the interrupt
>>> handler returned by request_irq to i8042 is handle_edge_irq that calls
>>> chip->irq_ack() before handle_irq_event().
>> 
>> I checked on which method it is using and it's using handle_fasteoi_irq. In 
> fatc all of the IRQs under 16 are despite most being edge. Log snippet below. 
> I'm looking into why pirq_needs_eoi is returning the wrong answer now.
> 
> 
> pirq_needs_eoi checks info->u.pirq.flags & PIRQ_NEEDS_EOI. PIRQ_NEEDS_EOI is 
> only set by pirq_query_unmask which sets it based on the hypercall 
> PHYSDEVOP_irq_status_query which in Xen 4.0.1 and Xen unstable always returns 
> an EOI is needed. Stefano, I don't see any changes in your 2.6.39 tree that 
> would effect this.
> 
> Relevant code snippets included below:
> 
>         if (pirq_needs_eoi(irq)) {
>                 printk(KERN_ERR "%s: irq %d handle_fasteoi_irq\n", 
> __FUNCTION__, irq);
>                 set_irq_chip_and_handler_name(irq, &xen_pirq_chip,
>                                 handle_fasteoi_irq, name);
>         } else {
>                 printk(KERN_ERR "%s: irq %d handle_edge_irq\n", 
> __FUNCTION__, irq);
>                 set_irq_chip_and_handler_name(irq, &xen_pirq_chip,
>                                 handle_edge_irq, name);
>         }

Now this, imo, is a very good reason to not use handle_edge_irq()
at all, and instead use the prior control flow (masking and clearing
the event channel up front in do_upcall()) with only fasteoi (leaving
aside per-CPU ones).

Jan

> static bool pirq_needs_eoi(unsigned irq)
> {
>         struct irq_info *info = info_for_irq(irq);
> 
>         BUG_ON(info->type != IRQT_PIRQ);
> 
>         return info->u.pirq.flags & PIRQ_NEEDS_EOI;
> }
> 
> static void pirq_query_unmask(int irq)
> {
>         struct physdev_irq_status_query irq_status;
>         struct irq_info *info = info_for_irq(irq);
> 
>         BUG_ON(info->type != IRQT_PIRQ);
> 
>         irq_status.irq = pirq_from_irq(irq);
>         if (HYPERVISOR_physdev_op(PHYSDEVOP_irq_status_query, &irq_status))
>                 irq_status.flags = 0;
> 
>         printk(KERN_ERR "%s: irq %d needs eoi %d\n", __FUNCTION__, irq, 
> (irq_status.flags & XENIRQSTAT_needs_eoi) == XENIRQSTAT_needs_eoi);
> 
>         info->u.pirq.flags &= ~PIRQ_NEEDS_EOI;
>         if (irq_status.flags & XENIRQSTAT_needs_eoi)
>                 info->u.pirq.flags |= PIRQ_NEEDS_EOI;
> }
> 
>     case PHYSDEVOP_irq_status_query: {
>         struct physdev_irq_status_query irq_status_query;
>         ret = -EFAULT;
>         if ( copy_from_guest(&irq_status_query, arg, 1) != 0 )
>             break;
>         irq = irq_status_query.irq;
>         ret = -EINVAL;
>         if ( (irq < 0) || (irq >= v->domain->nr_pirqs) )
>             break;
>         irq_status_query.flags = 0;
>         /*
>          * Even edge-triggered or message-based IRQs can need masking from
>          * time to time. If teh guest is not dynamically checking for this
>          * via the new pirq_eoi_map mechanism, it must conservatively always
>          * execute the EOI hypercall. In practice, this only really makes a
>          * difference for maskable MSI sources, and if those are supported
>          * then dom0 is probably modern anyway.
>          */
>         irq_status_query.flags |= XENIRQSTAT_needs_eoi;
>         if ( pirq_shared(v->domain, irq) )
>             irq_status_query.flags |= XENIRQSTAT_shared;
>         ret = copy_to_guest(arg, &irq_status_query, 1) ? -EFAULT : 0;
>         break;
>     }
> 
> 
> ---
> Tom Goetz
> tcgoetz@gmail.com