| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 25 Jun 2024 09:45:22 +0100
From: Marc Zyngier <maz@...nel.org>
To: Roman Kagan <rkagan@...zon.de>
Cc: <linux-arm-kernel@...ts.infradead.org>,
Catalin Marinas <catalin.marinas@....com>,
Will Deacon <will@...nel.org>,
<nh-open-source@...zon.com>,
<linux-doc@...r.kernel.org>,
<linux-kernel@...r.kernel.org>,
Thomas Gleixner <tglx@...utronix.de>,
Jonathan Corbet <corbet@....net>
Subject: Re: [PATCH] irqchip/gicv3-its: Workaround for GIC-700 erratum 2195890
On Mon, 24 Jun 2024 17:55:41 +0100,
Roman Kagan <rkagan@...zon.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@...zon.com>
> Signed-off-by: Mohamed Mediouni <mediou@...zon.com>
> Signed-off-by: Roman Kagan <rkagan@...zon.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.
Powered by blists - more mailing lists