[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH v3 14/39] ARM: new VGIC: Add GICv2 world switch backend



Hi Andre,

On 03/21/2018 04:32 PM, Andre Przywara wrote:
+        /*
+         * If a hardware mapped IRQ has been handled for good, we need to
+         * clear the _IRQ_INPROGRESS bit to allow handling of new IRQs.
+         */
+        if ( irq->hw && !lr_val.active && !lr_val.pending )
+        {
+            struct irq_desc *irqd = irq_to_desc(irq->hwintid);
+
+            clear_bit(_IRQ_INPROGRESS, &irqd->status);

I realize the current vGIC is doing exactly the same thing. But this is racy.

Imagine the interrupt is firing on another pCPU (I wasn't able to rule out this even when the interrupt is following the vCPU), that pCPU may set _IRQ_INPROGRESS before this is cleared here.

So you would end up clearing _IRQ_INPROGRESS here, making the desc state inconsistent and would affect the removal of the IRQ from a guest (see gic_remove_irq_from_guest).

I am not entirely sure how to fix this because taking the desc->lock would not be enough. Indeed you may end up to clear right after the flag was set in do_IRQ.

Because the race is already there in the current vGIC and only affecting release IRQ, I guess I would be ok to keep like that for now. Can you add a TODO?

+        }
+
+        /* Always preserve the active bit */
+        irq->active = lr_val.active;
+
+        /* Edge is the only case where we preserve the pending bit */
+        if ( irq->config == VGIC_CONFIG_EDGE && lr_val.pending )
+        {
+            irq->pending_latch = true;
+
+            if ( vgic_irq_is_sgi(intid) )
+                irq->source |= (1U << lr_val.virt.source);
+        }
+
+        /* Clear soft pending state when level irqs have been acked. */
+        if ( irq->config == VGIC_CONFIG_LEVEL && !lr_val.pending )
+            irq->pending_latch = false;
+
+        /*
+         * Level-triggered mapped IRQs are special because we only
+         * observe rising edges as input to the VGIC.
+         *
+         * If the guest never acked the interrupt we have to sample
+         * the physical line and set the line level, because the
+         * device state could have changed or we simply need to
+         * process the still pending interrupt later.
+         *
+         * If this causes us to lower the level, we have to also clear
+         * the physical active state, since we will otherwise never be
+         * told when the interrupt becomes asserted again.
+         */
+        if ( vgic_irq_is_mapped_level(irq) && lr_val.pending )
+        {
+            struct irq_desc *irqd;
+
+            ASSERT(irq->hwintid >= VGIC_NR_PRIVATE_IRQS);
+
+            irqd = irq_to_desc(irq->hwintid);
+            irq->line_level = gic_read_pending_state(irqd);
+
+            if ( !irq->line_level )
+                gic_set_active_state(irqd, false);

Sorry, I didn't notice it before. gic_set_active_state expect the desc->lock to be taken. But I can't see the code here to do that.

Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.