Re: [PATCH 3/3] irqchip/gic-v3: Relax polling of GIC{R,D}_CTLR.RWP

[Andre Przywara <andre.przywara@arm.com>]

On Tue, 15 Mar 2022 16:50:34 +0000
Marc Zyngier <maz@kernel.org> wrote:

Hi,

> Recent work on the KVM GIC emulation has revealed that the GICv3
> driver is a bit RWP-happy, as it polls this bit for each and
> every write MMIO access involving a single interrupt.
>
> As it turns out, polling RWP is only required when:
> - Disabling an SGI, PPI or SPI
> - Disabling LPIs at the redistributor level
> - Disabling groups
> - Enabling ARE
> - Dealing with DPG*
>
> Simplify the driver by removing all the other instances of RWP
> polling, and add the one that was missing when enabling the distributor
> (as that's where we set ARE).

Don't we need an explicit call to wait_for_rwp() now for:
gic_irq_set_irqchip_state(IRQCHIP_STATE_MASKED, true) ?

The rest looks fine to me.

Thanks,
Andre

>
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  drivers/irqchip/irq-gic-v3.c | 28 +++++++++++-----------------
>  1 file changed, 11 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index 363bfe172033..05ff7fef64cb 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -352,28 +352,27 @@ static int gic_peek_irq(struct irq_data *d, u32 offset)
>
>  static void gic_poke_irq(struct irq_data *d, u32 offset)
>  {
> -     void (*rwp_wait)(void);
>       void __iomem *base;
>       u32 index, mask;
>
>       offset = convert_offset_index(d, offset, &index);
>       mask = 1 << (index % 32);
>
> -     if (gic_irq_in_rdist(d)) {
> +     if (gic_irq_in_rdist(d))
>               base = gic_data_rdist_sgi_base();
> -             rwp_wait = gic_redist_wait_for_rwp;
> -     } else {
> +     else
>               base = gic_data.dist_base;
> -             rwp_wait = gic_dist_wait_for_rwp;
> -     }
>
>       writel_relaxed(mask, base + offset + (index / 32) * 4);
> -     rwp_wait();
>  }
>
>  static void gic_mask_irq(struct irq_data *d)
>  {
>       gic_poke_irq(d, GICD_ICENABLER);
> +     if (gic_irq_in_rdist(d))
> +             gic_redist_wait_for_rwp();
> +     else
> +             gic_dist_wait_for_rwp();
>  }
>
>  static void gic_eoimode1_mask_irq(struct irq_data *d)
> @@ -574,7 +573,6 @@ static int gic_set_type(struct irq_data *d, unsigned int type)
>  {
>       enum gic_intid_range range;
>       unsigned int irq = gic_irq(d);
> -     void (*rwp_wait)(void);
>       void __iomem *base;
>       u32 offset, index;
>       int ret;
> @@ -590,17 +588,14 @@ static int gic_set_type(struct irq_data *d, unsigned int type)
>           type != IRQ_TYPE_LEVEL_HIGH && type != IRQ_TYPE_EDGE_RISING)
>               return -EINVAL;
>
> -     if (gic_irq_in_rdist(d)) {
> +     if (gic_irq_in_rdist(d))
>               base = gic_data_rdist_sgi_base();
> -             rwp_wait = gic_redist_wait_for_rwp;
> -     } else {
> +     else
>               base = gic_data.dist_base;
> -             rwp_wait = gic_dist_wait_for_rwp;
> -     }
>
>       offset = convert_offset_index(d, GICD_ICFGR, &index);
>
> -     ret = gic_configure_irq(index, type, base + offset, rwp_wait);
> +     ret = gic_configure_irq(index, type, base + offset, NULL);
>       if (ret && (range == PPI_RANGE || range == EPPI_RANGE)) {
>               /* Misconfigured PPIs are usually not fatal */
>               pr_warn("GIC: PPI INTID%d is secure or misconfigured\n", irq);
> @@ -808,7 +803,7 @@ static void __init gic_dist_init(void)
>               writel_relaxed(GICD_INT_DEF_PRI_X4, base + GICD_IPRIORITYRnE + i);
>
>       /* Now do the common stuff, and wait for the distributor to drain */
> -     gic_dist_config(base, GIC_LINE_NR, gic_dist_wait_for_rwp);
> +     gic_dist_config(base, GIC_LINE_NR, NULL);
>
>       val = GICD_CTLR_ARE_NS | GICD_CTLR_ENABLE_G1A | GICD_CTLR_ENABLE_G1;
>       if (gic_data.rdists.gicd_typer2 & GICD_TYPER2_nASSGIcap) {
> @@ -818,6 +813,7 @@ static void __init gic_dist_init(void)
>
>       /* Enable distributor with ARE, Group1 */
>       writel_relaxed(val, base + GICD_CTLR);
> +     gic_dist_wait_for_rwp();
>
>       /*
>        * Set all global interrupts to the boot CPU only. ARE must be
> @@ -1293,8 +1289,6 @@ static int gic_set_affinity(struct irq_data *d, const struct cpumask *mask_val,
>        */
>       if (enabled)
>               gic_unmask_irq(d);
> -     else
> -             gic_dist_wait_for_rwp();
>
>       irq_data_update_effective_affinity(d, cpumask_of(cpu));
>

IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.