Re: [PATCH v6 1/5] xen/arm: observe itargets setting in vgic_enable_irqs and vgic_disable_irqs

[Julien Grall <julien.grall@linaro.org>]

Hi Stefano,

On 06/23/2014 05:37 PM, Stefano Stabellini wrote:
> +/* the rank lock is already taken */
> +static struct vcpu *_vgic_get_target_vcpu(struct vcpu *v, unsigned int irq)
> +{
> +    unsigned long target;
> +    struct vcpu *v_target;
> +    struct vgic_irq_rank *rank = vgic_rank_irq(v, irq);
> +    ASSERT(spin_is_locked(&rank->lock));
> +
> +    target = byte_read(rank->itargets[(irq%32)/4], 0, irq % 4);
> +    /* 1-N SPI should be delivered as pending to all the vcpus in the
> +     * mask, but here we just return the first vcpu for simplicity and
> +     * because it would be too slow to do otherwise. */
> +    target = find_first_bit(&target, 8);

I'm tempted to ask you adding an ASSERT here. Just for catching coding
error.

[..]

> @@ -536,8 +575,8 @@ static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
>          vgic_lock_rank(v, rank);
>          tr = rank->ienable;
>          rank->ienable |= *r;
> -        vgic_unlock_rank(v, rank);
>          vgic_enable_irqs(v, (*r) & (~tr), gicd_reg - GICD_ISENABLER);
> +        vgic_unlock_rank(v, rank);
>          return 1;
>  
>      case GICD_ICENABLER ... GICD_ICENABLERN:
> @@ -547,8 +586,8 @@ static int vgic_distr_mmio_write(struct vcpu *v, mmio_info_t *info)
>          vgic_lock_rank(v, rank);
>          tr = rank->ienable;
>          rank->ienable &= ~*r;
> -        vgic_unlock_rank(v, rank);
>          vgic_disable_irqs(v, (*r) & tr, gicd_reg - GICD_ICENABLER);
> +        vgic_unlock_rank(v, rank);
>          return 1;

Sorry for not having catch this earlier. I don't really like the idea to
extend the rank lock to vgic_{enable/disable}_irqs. The 2 functions
can be long to execute as it may touch the GIC distributor.

In another way, the rank lock is only taken in the distributor emulation.

Assuming we consider that distributor access may be slow:

Acked-by: Julien Grall <julien.grall@linaro.org>

Aside that, that made me think about two possible issue (not related to
this patch series) in vgic_inject_irq:
	- We don't take the rank lock to read the priority, even tho it's only
a byte access.
	- If the an interrupt is injected while it's already active, we don't
update the priority of this interrupt.

Regards,

-- 
Julien Grall