>>> On 23.05.11 at 19:28, Thomas Goetz <tcgoetz@xxxxxxxxx> 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@xxxxxxxxx
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home