| Message ID | 1406818852-31856-5-git-send-email-julien.grall@linaro.org |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Thu, 31 Jul 2014, Julien Grall wrote: > The structure pending_irq is initialized on the same way in 2 differents > place. Introduce vgic_init_pending_irq to avoid code duplication. > > Also move the setting of the irq field in this function as we need to > initialize it once rather than every time an IRQ is injected to the guest. > > Signed-off-by: Julien Grall <julien.grall@linaro.org> > Changes in v2: > - Patch added > --- > xen/arch/arm/vgic.c | 19 ++++++++++--------- > 1 file changed, 10 insertions(+), 9 deletions(-) > > diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c > index ac34437..17cde7a 100644 > --- a/xen/arch/arm/vgic.c > +++ b/xen/arch/arm/vgic.c > @@ -53,6 +53,13 @@ struct vgic_irq_rank *vgic_rank_irq(struct vcpu *v, unsigned int irq) > return vgic_rank_offset(v, 8, irq, DABT_WORD); > } > > +static void vgic_init_pending_irq(struct pending_irq *p, unsigned virq) > +{ > + INIT_LIST_HEAD(&p->inflight); > + INIT_LIST_HEAD(&p->lr_queue); > + p->irq = virq; > +} Reading this code made me realize that the irq field in pending_irq is a signed integer, while virq is unsigned. We should be consistent. In any case this patch is still an improvement over what we have now: Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > int domain_vgic_init(struct domain *d) > { > int i; > @@ -88,10 +95,8 @@ int domain_vgic_init(struct domain *d) > } > > for (i=0; i<d->arch.vgic.nr_spis; i++) > - { > - INIT_LIST_HEAD(&d->arch.vgic.pending_irqs[i].inflight); > - INIT_LIST_HEAD(&d->arch.vgic.pending_irqs[i].lr_queue); > - } > + vgic_init_pending_irq(&d->arch.vgic.pending_irqs[i], i + 32); > + > for (i=0; i<DOMAIN_NR_RANKS(d); i++) > { > spin_lock_init(&d->arch.vgic.shared_irqs[i].lock); > @@ -131,10 +136,7 @@ int vcpu_vgic_init(struct vcpu *v) > > memset(&v->arch.vgic.pending_irqs, 0, sizeof(v->arch.vgic.pending_irqs)); > for (i = 0; i < 32; i++) > - { > - INIT_LIST_HEAD(&v->arch.vgic.pending_irqs[i].inflight); > - INIT_LIST_HEAD(&v->arch.vgic.pending_irqs[i].lr_queue); > - } > + vgic_init_pending_irq(&v->arch.vgic.pending_irqs[i], i); > > INIT_LIST_HEAD(&v->arch.vgic.inflight_irqs); > INIT_LIST_HEAD(&v->arch.vgic.lr_pending); > @@ -386,7 +388,6 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq) > goto out; > } > > - n->irq = irq; > n->priority = priority; > > /* the irq is enabled */ > -- > 1.7.10.4 >
Hi Stefano, On 08/06/2014 03:06 PM, Stefano Stabellini wrote: > On Thu, 31 Jul 2014, Julien Grall wrote: >> The structure pending_irq is initialized on the same way in 2 differents >> place. Introduce vgic_init_pending_irq to avoid code duplication. >> >> Also move the setting of the irq field in this function as we need to >> initialize it once rather than every time an IRQ is injected to the guest. >> >> Signed-off-by: Julien Grall <julien.grall@linaro.org> > > > >> Changes in v2: >> - Patch added >> --- >> xen/arch/arm/vgic.c | 19 ++++++++++--------- >> 1 file changed, 10 insertions(+), 9 deletions(-) >> >> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c >> index ac34437..17cde7a 100644 >> --- a/xen/arch/arm/vgic.c >> +++ b/xen/arch/arm/vgic.c >> @@ -53,6 +53,13 @@ struct vgic_irq_rank *vgic_rank_irq(struct vcpu *v, unsigned int irq) >> return vgic_rank_offset(v, 8, irq, DABT_WORD); >> } >> >> +static void vgic_init_pending_irq(struct pending_irq *p, unsigned virq) >> +{ >> + INIT_LIST_HEAD(&p->inflight); >> + INIT_LIST_HEAD(&p->lr_queue); >> + p->irq = virq; >> +} > > Reading this code made me realize that the irq field in pending_irq is a > signed integer, while virq is unsigned. We should be consistent. I think we should use unsigned int everywhere. As p->irq is only used in gic_dump_info, I will made the p->irq change type in this patch. > In any case this patch is still an improvement over what we have now: > > Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> Can I keep your keep with the p->irq change type? Regards,
On Wed, 6 Aug 2014, Julien Grall wrote: > Hi Stefano, > > On 08/06/2014 03:06 PM, Stefano Stabellini wrote: > > On Thu, 31 Jul 2014, Julien Grall wrote: > >> The structure pending_irq is initialized on the same way in 2 differents > >> place. Introduce vgic_init_pending_irq to avoid code duplication. > >> > >> Also move the setting of the irq field in this function as we need to > >> initialize it once rather than every time an IRQ is injected to the guest. > >> > >> Signed-off-by: Julien Grall <julien.grall@linaro.org> > > > > > > > >> Changes in v2: > >> - Patch added > >> --- > >> xen/arch/arm/vgic.c | 19 ++++++++++--------- > >> 1 file changed, 10 insertions(+), 9 deletions(-) > >> > >> diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c > >> index ac34437..17cde7a 100644 > >> --- a/xen/arch/arm/vgic.c > >> +++ b/xen/arch/arm/vgic.c > >> @@ -53,6 +53,13 @@ struct vgic_irq_rank *vgic_rank_irq(struct vcpu *v, unsigned int irq) > >> return vgic_rank_offset(v, 8, irq, DABT_WORD); > >> } > >> > >> +static void vgic_init_pending_irq(struct pending_irq *p, unsigned virq) > >> +{ > >> + INIT_LIST_HEAD(&p->inflight); > >> + INIT_LIST_HEAD(&p->lr_queue); > >> + p->irq = virq; > >> +} > > > > Reading this code made me realize that the irq field in pending_irq is a > > signed integer, while virq is unsigned. We should be consistent. > > I think we should use unsigned int everywhere. As p->irq is only used in > gic_dump_info, I will made the p->irq change type in this patch. > > > In any case this patch is still an improvement over what we have now: > > > > Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com> > > Can I keep your keep with the p->irq change type? Yes
diff --git a/xen/arch/arm/vgic.c b/xen/arch/arm/vgic.c index ac34437..17cde7a 100644 --- a/xen/arch/arm/vgic.c +++ b/xen/arch/arm/vgic.c @@ -53,6 +53,13 @@ struct vgic_irq_rank *vgic_rank_irq(struct vcpu *v, unsigned int irq) return vgic_rank_offset(v, 8, irq, DABT_WORD); } +static void vgic_init_pending_irq(struct pending_irq *p, unsigned virq) +{ + INIT_LIST_HEAD(&p->inflight); + INIT_LIST_HEAD(&p->lr_queue); + p->irq = virq; +} + int domain_vgic_init(struct domain *d) { int i; @@ -88,10 +95,8 @@ int domain_vgic_init(struct domain *d) } for (i=0; i<d->arch.vgic.nr_spis; i++) - { - INIT_LIST_HEAD(&d->arch.vgic.pending_irqs[i].inflight); - INIT_LIST_HEAD(&d->arch.vgic.pending_irqs[i].lr_queue); - } + vgic_init_pending_irq(&d->arch.vgic.pending_irqs[i], i + 32); + for (i=0; i<DOMAIN_NR_RANKS(d); i++) { spin_lock_init(&d->arch.vgic.shared_irqs[i].lock); @@ -131,10 +136,7 @@ int vcpu_vgic_init(struct vcpu *v) memset(&v->arch.vgic.pending_irqs, 0, sizeof(v->arch.vgic.pending_irqs)); for (i = 0; i < 32; i++) - { - INIT_LIST_HEAD(&v->arch.vgic.pending_irqs[i].inflight); - INIT_LIST_HEAD(&v->arch.vgic.pending_irqs[i].lr_queue); - } + vgic_init_pending_irq(&v->arch.vgic.pending_irqs[i], i); INIT_LIST_HEAD(&v->arch.vgic.inflight_irqs); INIT_LIST_HEAD(&v->arch.vgic.lr_pending); @@ -386,7 +388,6 @@ void vgic_vcpu_inject_irq(struct vcpu *v, unsigned int irq) goto out; } - n->irq = irq; n->priority = priority; /* the irq is enabled */
The structure pending_irq is initialized on the same way in 2 differents place. Introduce vgic_init_pending_irq to avoid code duplication. Also move the setting of the irq field in this function as we need to initialize it once rather than every time an IRQ is injected to the guest. Signed-off-by: Julien Grall <julien.grall@linaro.org> --- Changes in v2: - Patch added --- xen/arch/arm/vgic.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-)