Re: [Xen-devel] [PATCH v2 20/45] ARM: new VGIC: Add GICv2 world switch backend

[Julien Grall <julien.grall@xxxxxxx>]

Hi Andre,

On 03/15/2018 08:30 PM, Andre Przywara wrote:
> +
> +       /*
> +        * 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.
> +        */
Again, the indentation looks wrong.

> +        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);
> +        }
> +
> +        spin_unlock_irqrestore(&irq->irq_lock, flags);
> +        vgic_put_irq(vcpu->domain, irq);
> +    }
> +
> +    gic_hw_ops->update_hcr_status(GICH_HCR_EN, 0);
Please use false instead 0.

[...]

> +void vgic_v2_populate_lr(struct vcpu *vcpu, struct vgic_irq *irq, int lr)
> +{
> +    struct gic_lr lr_val = {0};
> +
> +    lr_val.virq = irq->intid;
> +
> +    if ( irq_is_pending(irq) )
> +    {
> +        lr_val.pending = true;
> +
> +        if ( irq->config == VGIC_CONFIG_EDGE )
> +            irq->pending_latch = false;
> +
> +        if ( vgic_irq_is_sgi(irq->intid) )
> +        {
> +            u32 src = ffs(irq->source);
> +
> +            BUG_ON(!src);
> +            lr_val.virt.source = (src - 1);
> +            irq->source &= ~(1 << (src - 1));
> +            if ( irq->source )
> +                irq->pending_latch = true;
> +        }
> +    }
> +
> +    lr_val.active = irq->active;
> +
> +    if ( irq->hw )
> +    {
> +        lr_val.hw_status = 1;
Please use true.

> +        lr_val.hw.pirq = irq->hwintid;
> +        /*
> +         * Never set pending+active on a HW interrupt, as the
> +         * pending state is kept at the physical distributor
> +         * level.
> +         */
> +        if ( irq->active && irq_is_pending(irq) )
> +            lr_val.pending = false;
> +    }
> +    else
> +    {
> +        if ( irq->config == VGIC_CONFIG_LEVEL )
> +            lr_val.virt.eoi = 1;
Please use true.

Cheers,

--
Julien Grall

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