Re: [Xen-devel] Re: Losing PS/2 Interrupts

[Thomas Goetz <tcgoetz@xxxxxxxxx>]

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);
        }


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@xxxxxxxxx





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