| Message ID | 20180305160415.16760-20-andre.przywara@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | New VGIC(-v2) implementation | expand |
Hi Andre, On 05/03/18 16:03, Andre Przywara wrote: > A GICv3 hardware implementation can be implemented in several parts that > communicate with each other (think multi-socket systems). > To make sure that critical settings have arrived at all endpoints, some > bits are tracked using the RWP bit in the GICD_CTLR register, which > signals whether a register write is still in progress. > However this only applies to *some* registers, namely the bits in the > GICD_ICENABLER (disabling interrupts) and some bits in the GICD_CTLR > register (cf. Arm IHI 0069D, 8.9.4: RWP, bit[31]). > But our gicv3_poke_irq() was always polling this bit before returning, > resulting in pointless MMIO reads for many registers. > Add an option to gicv3_poke_irq() to state whether we want to wait for > this bit and use it accordingly to match the spec. > > Signed-off-by: Andre Przywara <andre.przywara@linaro.org> > --- > Changelog RFC ... v1: > - new patch > > xen/arch/arm/gic-v3.c | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c > index 3e381d031b..44dfba2267 100644 > --- a/xen/arch/arm/gic-v3.c > +++ b/xen/arch/arm/gic-v3.c > @@ -428,9 +428,9 @@ static void gicv3_dump_state(const struct vcpu *v) > } > } > > -static void gicv3_poke_irq(struct irq_desc *irqd, u32 offset) > +static void gicv3_poke_irq(struct irq_desc *irqd, u32 offset, bool wait_for_rwp) > { > - u32 mask = 1 << (irqd->irq % 32); > + u32 mask = 1U << (irqd->irq % 32); Do you mind adding a word about 1U in the commit message? With that: Reviewed-by: Julien Grall <julien.grall@arm.com> Cheers,
diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c index 3e381d031b..44dfba2267 100644 --- a/xen/arch/arm/gic-v3.c +++ b/xen/arch/arm/gic-v3.c @@ -428,9 +428,9 @@ static void gicv3_dump_state(const struct vcpu *v) } } -static void gicv3_poke_irq(struct irq_desc *irqd, u32 offset) +static void gicv3_poke_irq(struct irq_desc *irqd, u32 offset, bool wait_for_rwp) { - u32 mask = 1 << (irqd->irq % 32); + u32 mask = 1U << (irqd->irq % 32); void __iomem *base; if ( irqd->irq < NR_GIC_LOCAL_IRQS ) @@ -439,17 +439,19 @@ static void gicv3_poke_irq(struct irq_desc *irqd, u32 offset) base = GICD; writel_relaxed(mask, base + offset + (irqd->irq / 32) * 4); - gicv3_wait_for_rwp(irqd->irq); + + if ( wait_for_rwp ) + gicv3_wait_for_rwp(irqd->irq); } static void gicv3_unmask_irq(struct irq_desc *irqd) { - gicv3_poke_irq(irqd, GICD_ISENABLER); + gicv3_poke_irq(irqd, GICD_ISENABLER, false); } static void gicv3_mask_irq(struct irq_desc *irqd) { - gicv3_poke_irq(irqd, GICD_ICENABLER); + gicv3_poke_irq(irqd, GICD_ICENABLER, true); } static void gicv3_eoi_irq(struct irq_desc *irqd)
A GICv3 hardware implementation can be implemented in several parts that communicate with each other (think multi-socket systems). To make sure that critical settings have arrived at all endpoints, some bits are tracked using the RWP bit in the GICD_CTLR register, which signals whether a register write is still in progress. However this only applies to *some* registers, namely the bits in the GICD_ICENABLER (disabling interrupts) and some bits in the GICD_CTLR register (cf. Arm IHI 0069D, 8.9.4: RWP, bit[31]). But our gicv3_poke_irq() was always polling this bit before returning, resulting in pointless MMIO reads for many registers. Add an option to gicv3_poke_irq() to state whether we want to wait for this bit and use it accordingly to match the spec. Signed-off-by: Andre Przywara <andre.przywara@linaro.org> --- Changelog RFC ... v1: - new patch xen/arch/arm/gic-v3.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)