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

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Mon, 23 Jun 2014, Julien Grall wrote:
> 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.
> 

OK


> 
> > @@ -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:

We could end up enabling or disabling the wrong set of interrupts
without this change. I think it is necessary.


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

Thanks


> 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.

That's a good point


>       - If the an interrupt is injected while it's already active, we don't
> update the priority of this interrupt.

Yeah, I noticed this before. I'll write a comment to make sure we keep
in mind that we have this limitation.


> Regards,
> 
> -- 
> Julien Grall
> 

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel