| Message ID | 20171019124847.5978-13-andre.przywara@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, 19 Oct 2017, Andre Przywara wrote: > The functions to actually populate a list register were accessing > the VGIC internal pending_irq struct, although they should be abstracting > from that. > Break the needed information down to remove the reference to pending_irq > from gic-v[23].c. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> Reviewed-by: Stefano Stabellini <sstabellini@kernel.org> > --- > xen/arch/arm/gic-v2.c | 14 +++++++------- > xen/arch/arm/gic-v3.c | 12 ++++++------ > xen/arch/arm/gic-vgic.c | 3 ++- > xen/include/asm-arm/gic.h | 4 ++-- > 4 files changed, 17 insertions(+), 16 deletions(-) > > diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c > index 511c8d7294..e5acff8900 100644 > --- a/xen/arch/arm/gic-v2.c > +++ b/xen/arch/arm/gic-v2.c > @@ -428,8 +428,8 @@ static void gicv2_disable_interface(void) > spin_unlock(&gicv2.lock); > } > > -static void gicv2_update_lr(int lr, const struct pending_irq *p, > - unsigned int state) > +static void gicv2_update_lr(int lr, unsigned int virq, uint8_t priority, > + unsigned int hw_irq, unsigned int state) > { > uint32_t lr_reg; > > @@ -437,12 +437,12 @@ static void gicv2_update_lr(int lr, const struct pending_irq *p, > BUG_ON(lr < 0); > > lr_reg = (((state & GICH_V2_LR_STATE_MASK) << GICH_V2_LR_STATE_SHIFT) | > - ((GIC_PRI_TO_GUEST(p->priority) & GICH_V2_LR_PRIORITY_MASK) > - << GICH_V2_LR_PRIORITY_SHIFT) | > - ((p->irq & GICH_V2_LR_VIRTUAL_MASK) << GICH_V2_LR_VIRTUAL_SHIFT)); > + ((GIC_PRI_TO_GUEST(priority) & GICH_V2_LR_PRIORITY_MASK) > + << GICH_V2_LR_PRIORITY_SHIFT) | > + ((virq & GICH_V2_LR_VIRTUAL_MASK) << GICH_V2_LR_VIRTUAL_SHIFT)); > > - if ( p->desc != NULL ) > - lr_reg |= GICH_V2_LR_HW | ((p->desc->irq & GICH_V2_LR_PHYSICAL_MASK ) > + if ( hw_irq != -1 ) > + lr_reg |= GICH_V2_LR_HW | ((hw_irq & GICH_V2_LR_PHYSICAL_MASK ) > << GICH_V2_LR_PHYSICAL_SHIFT); > > writel_gich(lr_reg, GICH_LR + lr * 4); > diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c > index 74d00e0c54..3dec407a02 100644 > --- a/xen/arch/arm/gic-v3.c > +++ b/xen/arch/arm/gic-v3.c > @@ -944,8 +944,8 @@ static void gicv3_disable_interface(void) > spin_unlock(&gicv3.lock); > } > > -static void gicv3_update_lr(int lr, const struct pending_irq *p, > - unsigned int state) > +static void gicv3_update_lr(int lr, unsigned int virq, uint8_t priority, > + unsigned int hw_irq, unsigned int state) > { > uint64_t val = 0; > > @@ -961,11 +961,11 @@ static void gicv3_update_lr(int lr, const struct pending_irq *p, > if ( current->domain->arch.vgic.version == GIC_V3 ) > val |= GICH_LR_GRP1; > > - val |= ((uint64_t)p->priority & 0xff) << GICH_LR_PRIORITY_SHIFT; > - val |= ((uint64_t)p->irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT; > + val |= (uint64_t)priority << GICH_LR_PRIORITY_SHIFT; > + val |= ((uint64_t)virq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT; > > - if ( p->desc != NULL ) > - val |= GICH_LR_HW | (((uint64_t)p->desc->irq & GICH_LR_PHYSICAL_MASK) > + if ( hw_irq != -1 ) > + val |= GICH_LR_HW | (((uint64_t)hw_irq & GICH_LR_PHYSICAL_MASK) > << GICH_LR_PHYSICAL_SHIFT); > > gicv3_ich_write_lr(lr, val); > diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c > index 7765d83432..e783f3b54b 100644 > --- a/xen/arch/arm/gic-vgic.c > +++ b/xen/arch/arm/gic-vgic.c > @@ -52,7 +52,8 @@ static inline void gic_set_lr(int lr, struct pending_irq *p, > > clear_bit(GIC_IRQ_GUEST_PRISTINE_LPI, &p->status); > > - gic_hw_ops->update_lr(lr, p, state); > + gic_hw_ops->update_lr(lr, p->irq, p->priority, > + p->desc ? p->desc->irq : -1, state); > > set_bit(GIC_IRQ_GUEST_VISIBLE, &p->status); > clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status); > diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h > index fe14094c0f..66f0957fab 100644 > --- a/xen/include/asm-arm/gic.h > +++ b/xen/include/asm-arm/gic.h > @@ -339,8 +339,8 @@ struct gic_hw_operations { > /* Disable CPU physical and virtual interfaces */ > void (*disable_interface)(void); > /* Update LR register with state and priority */ > - void (*update_lr)(int lr, const struct pending_irq *pending_irq, > - unsigned int state); > + void (*update_lr)(int lr, unsigned int virq, uint8_t priority, > + unsigned int hw_irq, unsigned int state); > /* Update HCR status register */ > void (*update_hcr_status)(uint32_t flag, bool set); > /* Clear LR register */ > -- > 2.14.1 >
Hi Andre, On 10/19/2017 01:48 PM, Andre Przywara wrote: > The functions to actually populate a list register were accessing > the VGIC internal pending_irq struct, although they should be abstracting > from that. > Break the needed information down to remove the reference to pending_irq > from gic-v[23].c. > > Signed-off-by: Andre Przywara <andre.przywara@arm.com> > --- > xen/arch/arm/gic-v2.c | 14 +++++++------- > xen/arch/arm/gic-v3.c | 12 ++++++------ > xen/arch/arm/gic-vgic.c | 3 ++- > xen/include/asm-arm/gic.h | 4 ++-- > 4 files changed, 17 insertions(+), 16 deletions(-) > > diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c > index 511c8d7294..e5acff8900 100644 > --- a/xen/arch/arm/gic-v2.c > +++ b/xen/arch/arm/gic-v2.c > @@ -428,8 +428,8 @@ static void gicv2_disable_interface(void) > spin_unlock(&gicv2.lock); > } > > -static void gicv2_update_lr(int lr, const struct pending_irq *p, > - unsigned int state) > +static void gicv2_update_lr(int lr, unsigned int virq, uint8_t priority, > + unsigned int hw_irq, unsigned int state) > { > uint32_t lr_reg; > > @@ -437,12 +437,12 @@ static void gicv2_update_lr(int lr, const struct pending_irq *p, > BUG_ON(lr < 0); > > lr_reg = (((state & GICH_V2_LR_STATE_MASK) << GICH_V2_LR_STATE_SHIFT) | > - ((GIC_PRI_TO_GUEST(p->priority) & GICH_V2_LR_PRIORITY_MASK) > - << GICH_V2_LR_PRIORITY_SHIFT) | > - ((p->irq & GICH_V2_LR_VIRTUAL_MASK) << GICH_V2_LR_VIRTUAL_SHIFT)); > + ((GIC_PRI_TO_GUEST(priority) & GICH_V2_LR_PRIORITY_MASK) > + << GICH_V2_LR_PRIORITY_SHIFT) | > + ((virq & GICH_V2_LR_VIRTUAL_MASK) << GICH_V2_LR_VIRTUAL_SHIFT)); > > - if ( p->desc != NULL ) > - lr_reg |= GICH_V2_LR_HW | ((p->desc->irq & GICH_V2_LR_PHYSICAL_MASK ) > + if ( hw_irq != -1 ) > + lr_reg |= GICH_V2_LR_HW | ((hw_irq & GICH_V2_LR_PHYSICAL_MASK ) > << GICH_V2_LR_PHYSICAL_SHIFT); > > writel_gich(lr_reg, GICH_LR + lr * 4); > diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c > index 74d00e0c54..3dec407a02 100644 > --- a/xen/arch/arm/gic-v3.c > +++ b/xen/arch/arm/gic-v3.c > @@ -944,8 +944,8 @@ static void gicv3_disable_interface(void) > spin_unlock(&gicv3.lock); > } > > -static void gicv3_update_lr(int lr, const struct pending_irq *p, > - unsigned int state) > +static void gicv3_update_lr(int lr, unsigned int virq, uint8_t priority, > + unsigned int hw_irq, unsigned int state) > { > uint64_t val = 0; > > @@ -961,11 +961,11 @@ static void gicv3_update_lr(int lr, const struct pending_irq *p, > if ( current->domain->arch.vgic.version == GIC_V3 ) > val |= GICH_LR_GRP1; > > - val |= ((uint64_t)p->priority & 0xff) << GICH_LR_PRIORITY_SHIFT; > - val |= ((uint64_t)p->irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT; > + val |= (uint64_t)priority << GICH_LR_PRIORITY_SHIFT; > + val |= ((uint64_t)virq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT; > > - if ( p->desc != NULL ) > - val |= GICH_LR_HW | (((uint64_t)p->desc->irq & GICH_LR_PHYSICAL_MASK) > + if ( hw_irq != -1 ) hw_irq is unsigned to technically it should be ~0. Also, I would prefer if you introduce a define making clear where the -1 comes from. Lastly, I guess IRQ ~0 will never exist? Cheers,
diff --git a/xen/arch/arm/gic-v2.c b/xen/arch/arm/gic-v2.c index 511c8d7294..e5acff8900 100644 --- a/xen/arch/arm/gic-v2.c +++ b/xen/arch/arm/gic-v2.c @@ -428,8 +428,8 @@ static void gicv2_disable_interface(void) spin_unlock(&gicv2.lock); } -static void gicv2_update_lr(int lr, const struct pending_irq *p, - unsigned int state) +static void gicv2_update_lr(int lr, unsigned int virq, uint8_t priority, + unsigned int hw_irq, unsigned int state) { uint32_t lr_reg; @@ -437,12 +437,12 @@ static void gicv2_update_lr(int lr, const struct pending_irq *p, BUG_ON(lr < 0); lr_reg = (((state & GICH_V2_LR_STATE_MASK) << GICH_V2_LR_STATE_SHIFT) | - ((GIC_PRI_TO_GUEST(p->priority) & GICH_V2_LR_PRIORITY_MASK) - << GICH_V2_LR_PRIORITY_SHIFT) | - ((p->irq & GICH_V2_LR_VIRTUAL_MASK) << GICH_V2_LR_VIRTUAL_SHIFT)); + ((GIC_PRI_TO_GUEST(priority) & GICH_V2_LR_PRIORITY_MASK) + << GICH_V2_LR_PRIORITY_SHIFT) | + ((virq & GICH_V2_LR_VIRTUAL_MASK) << GICH_V2_LR_VIRTUAL_SHIFT)); - if ( p->desc != NULL ) - lr_reg |= GICH_V2_LR_HW | ((p->desc->irq & GICH_V2_LR_PHYSICAL_MASK ) + if ( hw_irq != -1 ) + lr_reg |= GICH_V2_LR_HW | ((hw_irq & GICH_V2_LR_PHYSICAL_MASK ) << GICH_V2_LR_PHYSICAL_SHIFT); writel_gich(lr_reg, GICH_LR + lr * 4); diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c index 74d00e0c54..3dec407a02 100644 --- a/xen/arch/arm/gic-v3.c +++ b/xen/arch/arm/gic-v3.c @@ -944,8 +944,8 @@ static void gicv3_disable_interface(void) spin_unlock(&gicv3.lock); } -static void gicv3_update_lr(int lr, const struct pending_irq *p, - unsigned int state) +static void gicv3_update_lr(int lr, unsigned int virq, uint8_t priority, + unsigned int hw_irq, unsigned int state) { uint64_t val = 0; @@ -961,11 +961,11 @@ static void gicv3_update_lr(int lr, const struct pending_irq *p, if ( current->domain->arch.vgic.version == GIC_V3 ) val |= GICH_LR_GRP1; - val |= ((uint64_t)p->priority & 0xff) << GICH_LR_PRIORITY_SHIFT; - val |= ((uint64_t)p->irq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT; + val |= (uint64_t)priority << GICH_LR_PRIORITY_SHIFT; + val |= ((uint64_t)virq & GICH_LR_VIRTUAL_MASK) << GICH_LR_VIRTUAL_SHIFT; - if ( p->desc != NULL ) - val |= GICH_LR_HW | (((uint64_t)p->desc->irq & GICH_LR_PHYSICAL_MASK) + if ( hw_irq != -1 ) + val |= GICH_LR_HW | (((uint64_t)hw_irq & GICH_LR_PHYSICAL_MASK) << GICH_LR_PHYSICAL_SHIFT); gicv3_ich_write_lr(lr, val); diff --git a/xen/arch/arm/gic-vgic.c b/xen/arch/arm/gic-vgic.c index 7765d83432..e783f3b54b 100644 --- a/xen/arch/arm/gic-vgic.c +++ b/xen/arch/arm/gic-vgic.c @@ -52,7 +52,8 @@ static inline void gic_set_lr(int lr, struct pending_irq *p, clear_bit(GIC_IRQ_GUEST_PRISTINE_LPI, &p->status); - gic_hw_ops->update_lr(lr, p, state); + gic_hw_ops->update_lr(lr, p->irq, p->priority, + p->desc ? p->desc->irq : -1, state); set_bit(GIC_IRQ_GUEST_VISIBLE, &p->status); clear_bit(GIC_IRQ_GUEST_QUEUED, &p->status); diff --git a/xen/include/asm-arm/gic.h b/xen/include/asm-arm/gic.h index fe14094c0f..66f0957fab 100644 --- a/xen/include/asm-arm/gic.h +++ b/xen/include/asm-arm/gic.h @@ -339,8 +339,8 @@ struct gic_hw_operations { /* Disable CPU physical and virtual interfaces */ void (*disable_interface)(void); /* Update LR register with state and priority */ - void (*update_lr)(int lr, const struct pending_irq *pending_irq, - unsigned int state); + void (*update_lr)(int lr, unsigned int virq, uint8_t priority, + unsigned int hw_irq, unsigned int state); /* Update HCR status register */ void (*update_hcr_status)(uint32_t flag, bool set); /* Clear LR register */
The functions to actually populate a list register were accessing the VGIC internal pending_irq struct, although they should be abstracting from that. Break the needed information down to remove the reference to pending_irq from gic-v[23].c. Signed-off-by: Andre Przywara <andre.przywara@arm.com> --- xen/arch/arm/gic-v2.c | 14 +++++++------- xen/arch/arm/gic-v3.c | 12 ++++++------ xen/arch/arm/gic-vgic.c | 3 ++- xen/include/asm-arm/gic.h | 4 ++-- 4 files changed, 17 insertions(+), 16 deletions(-)