Crash in __pirq_guest_eoi()

Crash in __pirq_guest_eoi()

[Jeremy Fitzhardinge]

 I'm seeing a crash in:

static void __pirq_guest_eoi(struct domain *d, int pirq)
{
    struct irq_desc         *desc;
    irq_guest_action_t *action;
    cpumask_t           cpu_eoi_map;
    int                 irq;

    ASSERT(local_irq_is_enabled());
    desc = domain_spin_lock_irq_desc(d, pirq, NULL);
    if ( desc == NULL )
        return;

    action = (irq_guest_action_t *)desc->action;
    irq = desc - irq_desc;

    if ( action->ack_type == ACKTYPE_NONE )
    {

where action is NULL.

I'm playing around with the pvops kernel's handling of pirq interrupts,
so the kernel is probably misbehaving, but it would be nice if Xen
didn't keep crashing on me.

I guess this is the right fix/workaround?

diff -r d37dc6401a1f xen/arch/x86/irq.c
--- a/xen/arch/x86/irq.c	Thu Sep 02 17:44:46 2010 +0100
+++ b/xen/arch/x86/irq.c	Fri Sep 03 11:33:15 2010 -0700
@@ -1028,6 +1028,9 @@
     action = (irq_guest_action_t *)desc->action;
     irq = desc - irq_desc;
 
+    if ( action == NULL )
+	return;
+
     if ( action->ack_type == ACKTYPE_NONE )
     {
         ASSERT(!test_bit(pirq, d->pirq_mask));

    J

Re: Crash in __pirq_guest_eoi()

[Jan Beulich]

 >>> On 03.09.10 at 20:33, Jeremy Fitzhardinge <jeremy@goop.org> wrote:
> I'm seeing a crash in:
> 
> static void __pirq_guest_eoi(struct domain *d, int pirq)
> {
>     struct irq_desc         *desc;
>     irq_guest_action_t *action;
>     cpumask_t           cpu_eoi_map;
>     int                 irq;
> 
>     ASSERT(local_irq_is_enabled());
>     desc = domain_spin_lock_irq_desc(d, pirq, NULL);
>     if ( desc == NULL )
>         return;
> 
>     action = (irq_guest_action_t *)desc->action;
>     irq = desc - irq_desc;
> 
>     if ( action->ack_type == ACKTYPE_NONE )
>     {
> 
> where action is NULL.

Any more precise information on the conditions under which this
happens? Like trying to EOI a bad pirq?

> I'm playing around with the pvops kernel's handling of pirq interrupts,
> so the kernel is probably misbehaving, but it would be nice if Xen
> didn't keep crashing on me.
> 
> I guess this is the right fix/workaround?

No. You can't return without releasing the lock acquired a couple of
lines earlier. And it seems bogus in the first place that you could get
there and find action being NULL, so it'd seem to be a workaround
at best.

Jan

> diff -r d37dc6401a1f xen/arch/x86/irq.c
> --- a/xen/arch/x86/irq.c	Thu Sep 02 17:44:46 2010 +0100
> +++ b/xen/arch/x86/irq.c	Fri Sep 03 11:33:15 2010 -0700
> @@ -1028,6 +1028,9 @@
>      action = (irq_guest_action_t *)desc->action;
>      irq = desc - irq_desc;
>  
> +    if ( action == NULL )
> +	return;
> +
>      if ( action->ack_type == ACKTYPE_NONE )
>      {
>          ASSERT(!test_bit(pirq, d->pirq_mask));
> 
>     J

Re: Crash in __pirq_guest_eoi()

[Jeremy Fitzhardinge]

 On 09/06/2010 10:04 PM, Jan Beulich wrote:
>  >>> On 03.09.10 at 20:33, Jeremy Fitzhardinge <jeremy@goop.org> wrote:
>> I'm seeing a crash in:
>>
>> static void __pirq_guest_eoi(struct domain *d, int pirq)
>> {
>>     struct irq_desc         *desc;
>>     irq_guest_action_t *action;
>>     cpumask_t           cpu_eoi_map;
>>     int                 irq;
>>
>>     ASSERT(local_irq_is_enabled());
>>     desc = domain_spin_lock_irq_desc(d, pirq, NULL);
>>     if ( desc == NULL )
>>         return;
>>
>>     action = (irq_guest_action_t *)desc->action;
>>     irq = desc - irq_desc;
>>
>>     if ( action->ack_type == ACKTYPE_NONE )
>>     {
>>
>> where action is NULL.
> Any more precise information on the conditions under which this
> happens? Like trying to EOI a bad pirq?

I'm not sure.  Part of the problem is that things are not working as I
expect, so I think my model of what's going on is wrong.

>> I'm playing around with the pvops kernel's handling of pirq interrupts,
>> so the kernel is probably misbehaving, but it would be nice if Xen
>> didn't keep crashing on me.
>>
>> I guess this is the right fix/workaround?
> No. You can't return without releasing the lock acquired a couple of
> lines earlier.

Right.  I saw the previous "return NULL" without thinking about the
locking aspects.

>  And it seems bogus in the first place that you could get
> there and find action being NULL, so it'd seem to be a workaround
> at best.

Yep.

    J