Re: [Xen-devel] [PATCH 04/12] ARM: VGIC: move gic_remove_irq_from_queues()

[Julien Grall <julien.grall@xxxxxxxxxx>]

Hi,

On 10/26/2017 01:19 AM, Stefano Stabellini wrote:
> On Thu, 19 Oct 2017, Andre Przywara wrote:
> > gic_remove_irq_from_queues() was not only misnamed, it also has the wrong
> > abstraction, as it should not live in gic.c.
> > Move it into vgic.c and vgic.h, where it belongs to, and rename it on
> > the way.
> >
> > Signed-off-by: Andre Przywara <andre.przywara@xxxxxxx>
> Yes, gic_remove_irq_from_queues could be in the vgic.
>
> Reviewed-by: Stefano Stabellini <sstabellini@xxxxxxxxxx>
>
> One comment about cosmetics below.
>
>
> > ---
> >   xen/arch/arm/gic.c         |  9 ---------
> >   xen/arch/arm/vgic-v3-its.c |  4 ++--
> >   xen/arch/arm/vgic.c        | 11 ++++++++++-
> >   xen/include/asm-arm/gic.h  |  1 -
> >   xen/include/asm-arm/vgic.h |  1 +
> >   5 files changed, 13 insertions(+), 13 deletions(-)
> >
> > diff --git a/xen/arch/arm/gic.c b/xen/arch/arm/gic.c
> > index 75b2e0e0ca..ef041354ea 100644
> > --- a/xen/arch/arm/gic.c
> > +++ b/xen/arch/arm/gic.c
> > @@ -411,15 +411,6 @@ void gic_remove_from_lr_pending(struct vcpu *v, struct 
> > pending_irq *p)
> >       list_del_init(&p->lr_queue);
> >   }
> >   
> > -void gic_remove_irq_from_queues(struct vcpu *v, struct pending_irq *p)
> > -{
> > -    ASSERT(spin_is_locked(&v->arch.vgic.lock));
> > -
> > -    clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status);
> > -    list_del_init(&p->inflight);
> > -    gic_remove_from_lr_pending(v, p);
> > -}
> > -
> >   void gic_raise_inflight_irq(struct vcpu *v, unsigned int virtual_irq)
> >   {
> >       struct pending_irq *n = irq_to_pending(v, virtual_irq);
> > diff --git a/xen/arch/arm/vgic-v3-its.c b/xen/arch/arm/vgic-v3-its.c
> > index 72a5c70656..d8fa44258d 100644
> > --- a/xen/arch/arm/vgic-v3-its.c
> > +++ b/xen/arch/arm/vgic-v3-its.c
> > @@ -381,7 +381,7 @@ static int its_handle_clear(struct virt_its *its, uint64_t 
> > *cmdptr)
> >        * have no active state, we don't need to care about this here.
> >        */
> >       if ( !test_bit(GIC_IRQ_GUEST_VISIBLE, &p->status) )
> > -        gic_remove_irq_from_queues(vcpu, p);
> > +        vgic_remove_irq_from_queues(vcpu, p);
> >   
> >       spin_unlock_irqrestore(&vcpu->arch.vgic.lock, flags);
> >       ret = 0;
> > @@ -619,7 +619,7 @@ static int its_discard_event(struct virt_its *its,
> >       }
> >   
> >       /* Cleanup the pending_irq and disconnect it from the LPI. */
> > -    gic_remove_irq_from_queues(vcpu, p);
> > +    vgic_remove_irq_from_queues(vcpu, p);
> >       vgic_init_pending_irq(p, INVALID_LPI);
> >   
> >       spin_unlock_irqrestore(&vcpu->arch.vgic.lock, flags);
> > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c
> > index 451a306a98..cd50b90d67 100644
> > --- a/xen/arch/arm/vgic.c
> > +++ b/xen/arch/arm/vgic.c
> > @@ -281,7 +281,7 @@ bool vgic_migrate_irq(struct vcpu *old, struct vcpu *new, 
> > unsigned int irq)
> >       /* If the IRQ is still lr_pending, re-inject it to the new vcpu */
> >       if ( !list_empty(&p->lr_queue) )
> >       {
> > -        gic_remove_irq_from_queues(old, p);
> > +        vgic_remove_irq_from_queues(old, p);
> >           irq_set_affinity(p->desc, cpumask_of(new->processor));
> >           spin_unlock_irqrestore(&old->arch.vgic.lock, flags);
> >           vgic_vcpu_inject_irq(new, irq);
> > @@ -510,6 +510,15 @@ void vgic_clear_pending_irqs(struct vcpu *v)
> >       spin_unlock_irqrestore(&v->arch.vgic.lock, flags);
> >   }
> >   
> > +void vgic_remove_irq_from_queues(struct vcpu *v, struct pending_irq *p)
> > +{
> > +    ASSERT(spin_is_locked(&v->arch.vgic.lock));
> > +
> > +    clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status);
> > +    list_del_init(&p->inflight);
> > +    gic_remove_from_lr_pending(v, p);
> > +}
> > +
> >   void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq)
> >   {
> >       uint8_t priority;
> > diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h
> > index 2f248301ce..030c1d86a7 100644
> > --- a/xen/include/asm-arm/gic.h
> > +++ b/xen/include/asm-arm/gic.h
> > @@ -243,7 +243,6 @@ extern void gic_raise_guest_irq(struct vcpu *v, unsigned 
> > int irq,
> >           unsigned int priority);
> >   extern void gic_raise_inflight_irq(struct vcpu *v, unsigned int virtual_irq);
> >   extern void gic_remove_from_lr_pending(struct vcpu *v, struct pending_irq *p);
> > -extern void gic_remove_irq_from_queues(struct vcpu *v, struct pending_irq *p);
> >   
> >   /* Accept an interrupt from the GIC and dispatch its handler */
> >   extern void gic_interrupt(struct cpu_user_regs *regs, int is_fiq);
> > diff --git a/xen/include/asm-arm/vgic.h b/xen/include/asm-arm/vgic.h
> > index e489d0bf21..8d0ff65708 100644
> > --- a/xen/include/asm-arm/vgic.h
> > +++ b/xen/include/asm-arm/vgic.h
> > @@ -204,6 +204,7 @@ extern int vcpu_vgic_init(struct vcpu *v);
> >   extern struct vcpu *vgic_get_target_vcpu(struct vcpu *v, unsigned int virq);
> >   extern void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int virq);
> >   extern void vgic_vcpu_inject_spi(struct domain *d, unsigned int virq);
> > +void vgic_remove_irq_from_queues(struct vcpu *v, struct pending_irq *p);
> cosmetic: you might as well add an extern
Or just dropped extern from the others. The keyword is pointless.

> >   extern void vgic_clear_pending_irqs(struct vcpu *v);
> >   extern void vgic_init_pending_irq(struct pending_irq *p, unsigned int virq);
> >   extern struct pending_irq *irq_to_pending(struct vcpu *v, unsigned int irq);
--
Julien Grall

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