Re: [Xen-devel] [PATCH-4.5 3/4] xen/arm: do not request maintenance_interrupts

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Fri, 7 Feb 2014, Julien Grall wrote:
> Hi Stefano,
> 
> On 07/02/14 18:56, Stefano Stabellini wrote:
> > Do not set GICH_LR_MAINTENANCE_IRQ for every interrupt with set in the
> > GICH_LR registers.
> > 
> > Introduce a new function, gic_clear_lrs, that goes over the GICH_LR
> > registers, clear the invalid ones and free the corresponding interrupts
> > from the inflight queue if appropriate. Add the interrupt to lr_pending
> > if the GIC_IRQ_GUEST_PENDING is still set.
> > 
> > Call gic_clear_lrs from gic_restore_state and on return to guest
> > (gic_inject).
> > 
> > Remove the now unused code in maintenance_interrupts and gic_irq_eoi.
> > 
> > In vgic_vcpu_inject_irq, if the target is a vcpu running on another cpu,
> > send and SGI to it to interrupt it and force it to clear the old LRs.
> > 
> > Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
> > ---
> >   xen/arch/arm/gic.c  |  126
> > ++++++++++++++++++++++-----------------------------
> >   xen/arch/arm/vgic.c |    3 +-
> >   2 files changed, 56 insertions(+), 73 deletions(-)
> > 
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index 215b679..87bd5d3 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > +static void gic_clear_lrs(struct vcpu *v)
> > +{
> > +    struct pending_irq *p;
> > +    int i = 0, irq;
> > +    uint32_t lr;
> > +    bool_t inflight;
> > +
> > +    ASSERT(!local_irq_is_enabled());
> > +
> > +    while ((i = find_next_bit((const long unsigned int *)
> > &this_cpu(lr_mask),
> > +                              nr_lrs, i)) < nr_lrs) {
> > +        lr = GICH[GICH_LR + i];
> > +        if ( !(lr & (GICH_LR_PENDING|GICH_LR_ACTIVE)) )
> > +        {
> > +            if ( lr & GICH_LR_HW )
> > +                irq = (lr >> GICH_LR_PHYSICAL_SHIFT) &
> > GICH_LR_PHYSICAL_MASK;
> > +            else
> > +                irq = (lr >> GICH_LR_VIRTUAL_SHIFT) & GICH_LR_VIRTUAL_MASK;
> > +
> 
> The if sentence can be simply by:
> 
> irq = (lr >> GICH_LR_VIRTUAL_SHIFT) & GICH_LR_VIRTUAL_MASK;

right

 
> > +            inflight = 0;
> > +            GICH[GICH_LR + i] = 0;
> > +            clear_bit(i, &this_cpu(lr_mask));
> > +
> > +            spin_lock(&gic.lock);
> > +            p = irq_to_pending(v, irq);
> > +            if ( p->desc != NULL )
> > +                p->desc->status &= ~IRQ_INPROGRESS;
> > +            clear_bit(GIC_IRQ_GUEST_VISIBLE, &p->status);
> > +            if ( test_bit(GIC_IRQ_GUEST_PENDING, &p->status) &&
> > +                    test_bit(GIC_IRQ_GUEST_ENABLED, &p->status))
> > +            {
> 
> I would add a WARN_ON(p->desc != NULL) here. AFAIK, this code path shouldn't
> be used for physical IRQ.

That's not true: an edge physical irq can come through while another one
of the same type is being handled. In fact pending and active bits exist
even on the physical GIC interface.

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