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

[Julien Grall <julien.grall@xxxxxxx>]

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