Re: [PATCH] irqchip/gicv3-its: Workaround for GIC-700 erratum 2195890

[Marc Zyngier <maz@kernel.org>]

On Mon, 24 Jun 2024 17:55:41 +0100,
Roman Kagan <rkagan@amazon.de> wrote:
> 
> According to Arm CoreLink GIC-700 erratum 2195890, on GIC revisions
> r0p0, r0p1, r1p0 under certain conditions LPIs may remain in the Pending
> Table until one of a number of external events occurs.

Please add a link to the errata document.

> 
> No LPIs are lost but they may not be delivered in a finite time.
> 
> The workaround is to issue an INV using GICR_INVLPIR to an unused, in
> range LPI ID to retrigger the search.
> 
> Add this workaround to the quirk table.  When the quirk is applicable,
> carve out one LPI ID from the available range and run periodic work to
> do INV to it, in order to prevent GIC from stalling.

The errata document says a lot more:

<quote>
For physical LPIs the workaround is to issue an INV using GICR_INVLPIR
to an unused, in range LPI ID to retrigger the search. This could be
done periodically, for example, in line with a residency change, or as
part of servicing LPIs.  If using LPIs as the event, then the
GICR_INVLPIR write could be issued after servicing every LPI.

However, it only needs to be issued if:

* At least 4 interrupts in the block of 32 are enabled and mapped to
  the current PE or, if easier,

* At least 4 interrupts in the block of 32 are enabled and mapped to
  any PE
</quote>

> 
> TT: https://t.corp.amazon.com/D82032616

Gniii????

> Signed-off-by: Elad Rosner <eladros@amazon.com>
> Signed-off-by: Mohamed Mediouni <mediou@amazon.com>
> Signed-off-by: Roman Kagan <rkagan@amazon.de>

Who is the author?

> ---
>  drivers/irqchip/irq-gic-v3-its.c            | 70 ++++++++++++++++++++-
>  Documentation/arch/arm64/silicon-errata.rst |  2 +
>  arch/arm64/Kconfig                          | 18 ++++++
>  3 files changed, 89 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index 3c755d5dad6e..53cf50dd8e13 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -29,6 +29,7 @@
>  #include <linux/percpu.h>
>  #include <linux/slab.h>
>  #include <linux/syscore_ops.h>
> +#include <linux/workqueue.h>
>  
>  #include <linux/irqchip.h>
>  #include <linux/irqchip/arm-gic-v3.h>
> @@ -49,6 +50,7 @@
>  #define RD_LOCAL_MEMRESERVE_DONE                BIT(2)
>  
>  static u32 lpi_id_bits;
> +static u32 lpi_id_base __initdata = 8192;
>  
>  /*
>   * We allocate memory for PROPBASE to cover 2 ^ lpi_id_bits LPIs to
> @@ -2136,7 +2138,7 @@ static int __init its_lpi_init(u32 id_bits)
>  	 * Initializing the allocator is just the same as freeing the
>  	 * full range of LPIs.
>  	 */
> -	err = free_lpi_range(8192, lpis);
> +	err = free_lpi_range(lpi_id_base, lpis - lpi_id_base + 8192);
>  	pr_debug("ITS: Allocator initialized for %u LPIs\n", lpis);
>  	return err;
>  }
> @@ -4763,6 +4765,61 @@ static bool its_set_non_coherent(void *data)
>  	return true;
>  }
>  
> +#define ITS_QUIRK_GIC700_2195890_PERIOD_MSEC 1000

Use MSEC_PER_SEC.

> +static struct {
> +	u32 lpi;
> +	struct delayed_work work;
> +} its_quirk_gic700_2195890_data __maybe_unused;
> +
> +static void __maybe_unused its_quirk_gic700_2195890_work_handler(struct work_struct *work)
> +{
> +	int cpu;
> +	void __iomem *rdbase;
> +	u64 gicr_invlpir_val;
> +
> +	for_each_online_cpu(cpu) {

The errata document doesn't say that this need to happen for *every*
RD. Can you please clarify this?

> +		rdbase = gic_data_rdist_cpu(cpu)->rd_base;
> +		if (!rdbase) {
> +			continue;
> +		}
> +
> +		/*
> +		 * Prod the respective GIC with an INV for an otherwise unused
> +		 * LPI.  This is only to resume the stalled processing, so
> +		 * there's no need to wait for invalidation to complete.
> +		 */
> +		gicr_invlpir_val =
> +			FIELD_PREP(GICR_INVLPIR_INTID,
> +				   its_quirk_gic700_2195890_data.lpi);

Don't split assignments.

> +		raw_spin_lock(&gic_data_rdist_cpu(cpu)->rd_lock);
> +		gic_write_lpir(gicr_invlpir_val, rdbase + GICR_INVLPIR);
> +		raw_spin_unlock(&gic_data_rdist_cpu(cpu)->rd_lock);

No synchronisation? How is that supposed to work?

Also, if you need to dig into the internals of the driver, extract a
helper from __direct_lpi_inv().


> +	}
> +
> +	schedule_delayed_work(&its_quirk_gic700_2195890_data.work,
> +		msecs_to_jiffies(ITS_QUIRK_GIC700_2195890_PERIOD_MSEC));

It would be pretty easy to detect whether an LPI was ack'ed since the
last pass, and not issue the invalidate.

> +}
> +
> +static bool __maybe_unused its_enable_quirk_gic700_2195890(void *data)
> +{
> +	struct its_node *its = data;
> +
> +	if (its_quirk_gic700_2195890_data.lpi)
> +		return true;
> +
> +	/*
> +	 * Use one LPI INTID from the start of the LPI range for GIC prodding,
> +	 * and make it unavailable for regular LPI use later.
> +	 */
> +	its_quirk_gic700_2195890_data.lpi = lpi_id_base++;
> +
> +	INIT_DELAYED_WORK(&its_quirk_gic700_2195890_data.work,
> +			  its_quirk_gic700_2195890_work_handler);
> +	schedule_delayed_work(&its_quirk_gic700_2195890_data.work, 0);
> +
> +	return true;
> +}

It is a bit odd to hook this on an ITS being probed when the ITS isn't
really involved. Not a big deal, but a bit clumsy.

> +
>  static const struct gic_quirk its_quirks[] = {
>  #ifdef CONFIG_CAVIUM_ERRATUM_22375
>  	{
> @@ -4822,6 +4879,17 @@ static const struct gic_quirk its_quirks[] = {
>  		.property = "dma-noncoherent",
>  		.init   = its_set_non_coherent,
>  	},
> +#ifdef CONFIG_ARM64_ERRATUM_2195890
> +	{
> +		.desc	= "ITS: GIC-700 erratum 2195890",
> +		/*
> +		 * Applies to r0p0, r0p1, r1p0: iidr_var(bits 16..19) == 0 or 1
> +		 */
> +		.iidr	= 0x0400043b,
> +		.mask	= 0xfffeffff,
> +		.init	= its_enable_quirk_gic700_2195890,

This catches r0p0 and r1p0, but not r0p1 (you require that bits 15:12
are 0).

Overall, this requires a bit of rework. Notably, this could be
significantly relaxed to match the requirements of the published
workaround.

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.