| Message ID | 20171019124847.5978-5-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@arm.com> Yes, gic_remove_irq_from_queues could be in the vgic. Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> 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 > 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); > -- > 2.14.1 >
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@arm.com> > > Yes, gic_remove_irq_from_queues could be in the vgic. > > Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > > 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);
Hi, ... >> 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 I was wondering about that. I think extern in front of a prototype in a header file is a bit pointless. Linux mostly doesn't use it (apart from fs/ and some parts of security/). Though I can of course easily add it. Cheers, Andre. > > >> 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); >> -- >> 2.14.1 >>
On Fri, 10 Nov 2017, Andre Przywara wrote: > Hi, > > ... > > >> 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 > > I was wondering about that. I think extern in front of a prototype in a > header file is a bit pointless. Linux mostly doesn't use it (apart from > fs/ and some parts of security/). > Though I can of course easily add it. It was just to stay consistent. It's fine also to remove all the useless extern keywords from this header.
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); 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);
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@arm.com> --- 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(-)