| 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
| ||
|
Message-ID: <871qzp7n4r.wl-maz@kernel.org>
Date: Sat, 26 Feb 2022 13:50:28 +0000
From: Marc Zyngier <maz@...nel.org>
To: Linu Cherian <lcherian@...vell.com>
Cc: <tglx@...utronix.de>, <catalin.marinas@....com>, <will@...nel.org>,
<linux-kernel@...r.kernel.org>,
<linux-arm-kernel@...ts.infradead.org>, <linuc.decode@...il.com>
Subject: Re: [PATCH] irqchip/gic-v3: Workaround Marvell erratum 38545 when reading IAR
On Sat, 26 Feb 2022 12:33:32 +0000,
Linu Cherian <lcherian@...vell.com> wrote:
>
> When a IAR register read races with a GIC interrupt RELEASE event,
> GIC-CPU interface could wrongly return a valid INTID to the CPU
> for an interrupt that is already released(non activated) instead of 0x3ff.
>
> As a side effect, an interrupt handler could run twice, once with
> interrupt priority and then with idle priority.
>
> As a workaround, gic_read_iar is updated so that it will return a
> valid interrupt ID only if there is a change in the active priority list
> after the IAR read on all the affected Silicons.
>
> Along with this, Thunderx erratum 23154 is reworked to use GIC IIDR
> based quirk management for the sake of consistency and also
> because there is workaround overlap on some silicon variants.
>
> Signed-off-by: Linu Cherian <lcherian@...vell.com>
> ---
> Documentation/arm64/silicon-errata.rst | 4 +-
> arch/arm64/Kconfig | 10 -----
> arch/arm64/include/asm/arch_gicv3.h | 24 +++++++++--
> arch/arm64/kernel/cpu_errata.c | 8 ----
> arch/arm64/tools/cpucaps | 1 -
> drivers/irqchip/irq-gic-v3.c | 56 +++++++++++++++++++++++++-
> 6 files changed, 77 insertions(+), 26 deletions(-)
>
> diff --git a/Documentation/arm64/silicon-errata.rst b/Documentation/arm64/silicon-errata.rst
> index ea281dd75517..f602faf4bf82 100644
> --- a/Documentation/arm64/silicon-errata.rst
> +++ b/Documentation/arm64/silicon-errata.rst
> @@ -136,10 +136,12 @@ stable kernels.
> +----------------+-----------------+-----------------+-----------------------------+
> | Cavium | ThunderX ITS | #23144 | CAVIUM_ERRATUM_23144 |
> +----------------+-----------------+-----------------+-----------------------------+
> -| Cavium | ThunderX GICv3 | #23154 | CAVIUM_ERRATUM_23154 |
> +| Cavium | ThunderX GICv3 | #23154 | N/A |
> +----------------+-----------------+-----------------+-----------------------------+
> | Cavium | ThunderX GICv3 | #38539 | N/A |
> +----------------+-----------------+-----------------+-----------------------------+
> +| Cavium | ThunderX GICv3 | #38545 | N/A |
Cavium? Or Marvell? I can understand the identity crisis, but you
should pick your poison. It also seem to affect the new TX2 rather
than (or in addition to) the ancient TX.
> ++----------------+-----------------+-----------------+-----------------------------+
> | Cavium | ThunderX Core | #27456 | CAVIUM_ERRATUM_27456 |
> +----------------+-----------------+-----------------+-----------------------------+
> | Cavium | ThunderX Core | #30115 | CAVIUM_ERRATUM_30115 |
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 09b885cc4db5..889cb56bf5ec 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -890,16 +890,6 @@ config CAVIUM_ERRATUM_23144
>
> If unsure, say Y.
>
> -config CAVIUM_ERRATUM_23154
> - bool "Cavium erratum 23154: Access to ICC_IAR1_EL1 is not sync'ed"
> - default y
> - help
> - The gicv3 of ThunderX requires a modified version for
> - reading the IAR status to ensure data synchronization
> - (access to icc_iar1_el1 is not sync'ed before and after).
> -
> - If unsure, say Y.
> -
This is starting really badly. Why make this config option disappear?
This is both useful for documentation (in the absence of a public
errata document from the silicon vendor, I *really* want to know what
I am affected by) and for people who do not want their kernels to be
encumbered by support for broken HW.
I actually want to see a description of the *new* errata in Kconfig,
warts and all.
> config CAVIUM_ERRATUM_27456
> bool "Cavium erratum 27456: Broadcast TLBI instructions may cause icache corruption"
> default y
> diff --git a/arch/arm64/include/asm/arch_gicv3.h b/arch/arm64/include/asm/arch_gicv3.h
> index 4ad22c3135db..bc98a60a4bcb 100644
> --- a/arch/arm64/include/asm/arch_gicv3.h
> +++ b/arch/arm64/include/asm/arch_gicv3.h
> @@ -47,21 +47,37 @@ static inline u64 gic_read_iar_common(void)
> return irqstat;
> }
>
> -/*
> +/* Marvell Erratum 38545
> + *
> + * When a IAR register read races with a GIC interrupt RELEASE event,
> + * GIC-CPU interface could wrongly return a valid INTID to the CPU
> + * for an interrupt that is already released(non activated) instead of 0x3ff.
> + *
> + * To workaround this, return a valid interrupt ID only if there is a change
> + * in the active priority list after the IAR read.
> + *
> * Cavium ThunderX erratum 23154
> *
> * The gicv3 of ThunderX requires a modified version for reading the
> * IAR status to ensure data synchronization (access to icc_iar1_el1
> * is not sync'ed before and after).
> + *
> + * Have merged both the workarounds into a common function since,
> + * 1. On Thunderx 88xx 1.x both erratas are applicable.
This is new. Are you saying that TX is also affected by this new bug?
Experience doesn't seem to support this statement, but I am prepared
to believe that this thing is even more broken than I thought.
Also, what is the story for virtualisation? Does the ICV interface
suffer from the same bug? I can't see why not (23154 definitely
affects virtualisation).
> + * 2. Having extra nops doesn't add any side effects for Silicons where
> + * erratum 23154 is not applicable.
> */
> -static inline u64 gic_read_iar_cavium_thunderx(void)
> +static inline u64 gic_read_iar_marvell_38545_23154(void)
> {
> - u64 irqstat;
> + u64 irqstat, apr;
>
> + apr = read_sysreg_s(SYS_ICC_AP1R0_EL1);
> nops(8);
> irqstat = read_sysreg_s(SYS_ICC_IAR1_EL1);
> nops(4);
> - mb();
> + dsb(sy);
mb() *is* a dsb(sy). Why the change?
> + if (unlikely(apr == read_sysreg_s(SYS_ICC_AP1R0_EL1)))
> + return 0x3ff;
So you are adding extra overhead to all TX platforms that did not
require it. Why is that an acceptable outcome? If all platforms
affected by 23154 are also affected by 38545, why are they treated
independently with separate flags?
>
> return irqstat;
> }
> diff --git a/arch/arm64/kernel/cpu_errata.c b/arch/arm64/kernel/cpu_errata.c
> index b217941713a8..79beb800ee79 100644
> --- a/arch/arm64/kernel/cpu_errata.c
> +++ b/arch/arm64/kernel/cpu_errata.c
> @@ -423,14 +423,6 @@ const struct arm64_cpu_capabilities arm64_errata[] = {
> ERRATA_MIDR_RANGE_LIST(erratum_845719_list),
> },
> #endif
> -#ifdef CONFIG_CAVIUM_ERRATUM_23154
> - {
> - /* Cavium ThunderX, pass 1.x */
> - .desc = "Cavium erratum 23154",
> - .capability = ARM64_WORKAROUND_CAVIUM_23154,
> - ERRATA_MIDR_REV_RANGE(MIDR_THUNDERX, 0, 0, 1),
> - },
> -#endif
NAK. See below.
> #ifdef CONFIG_CAVIUM_ERRATUM_27456
> {
> .desc = "Cavium erratum 27456",
> diff --git a/arch/arm64/tools/cpucaps b/arch/arm64/tools/cpucaps
> index 9c65b1e25a96..3f751fe4fec4 100644
> --- a/arch/arm64/tools/cpucaps
> +++ b/arch/arm64/tools/cpucaps
> @@ -62,7 +62,6 @@ WORKAROUND_2077057
> WORKAROUND_TRBE_OVERWRITE_FILL_MODE
> WORKAROUND_TSB_FLUSH_FAILURE
> WORKAROUND_TRBE_WRITE_OUT_OF_RANGE
> -WORKAROUND_CAVIUM_23154
> WORKAROUND_CAVIUM_27456
> WORKAROUND_CAVIUM_30115
> WORKAROUND_CAVIUM_TX2_219_PRFM
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index 5e935d97207d..a3b58bf4fce4 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -35,6 +35,8 @@
>
> #define FLAGS_WORKAROUND_GICR_WAKER_MSM8996 (1ULL << 0)
> #define FLAGS_WORKAROUND_CAVIUM_ERRATUM_38539 (1ULL << 1)
> +#define FLAGS_WORKAROUND_CAVIUM_ERRATUM_23154 (1ULL << 2)
> +#define FLAGS_WORKAROUND_MARVELL_ERRATUM_38545 (1ULL << 3)
>
> #define GIC_IRQ_TYPE_PARTITION (GIC_IRQ_TYPE_LPI + 1)
>
> @@ -60,6 +62,7 @@ struct gic_chip_data {
>
> static struct gic_chip_data gic_data __read_mostly;
> static DEFINE_STATIC_KEY_TRUE(supports_deactivate_key);
> +static DEFINE_STATIC_KEY_FALSE(gic_iar_quirk);
>
> #define GIC_ID_NR (1U << GICD_TYPER_ID_BITS(gic_data.rdists.gicd_typer))
> #define GIC_LINE_NR min(GICD_TYPER_SPIS(gic_data.rdists.gicd_typer), 1020U)
> @@ -235,10 +238,19 @@ static void gic_redist_wait_for_rwp(void)
>
> #ifdef CONFIG_ARM64
>
> +static u64 __maybe_unused gic_read_iar_fixup(void)
> +{
> + if (gic_data.flags & FLAGS_WORKAROUND_MARVELL_ERRATUM_38545 ||
> + gic_data.flags & FLAGS_WORKAROUND_CAVIUM_ERRATUM_23154)
> + return gic_read_iar_marvell_38545_23154();
> + else /* Not possible */
> + return ICC_IAR1_EL1_SPURIOUS;
Not possible? What does that even mean? And what is the point of
gating things with a static key if you have to check again with an
extra load?
> +}
> +
> static u64 __maybe_unused gic_read_iar(void)
> {
> - if (cpus_have_const_cap(ARM64_WORKAROUND_CAVIUM_23154))
> - return gic_read_iar_cavium_thunderx();
> + if (static_branch_unlikely(&gic_iar_quirk))
> + return gic_read_iar_fixup();
You are trading a static key for another one describing... the same
thing. What is the point?
> else
> return gic_read_iar_common();
> }
> @@ -1614,6 +1626,16 @@ static bool gic_enable_quirk_msm8996(void *data)
> return true;
> }
>
> +static bool gic_enable_quirk_cavium_23154(void *data)
> +{
> + struct gic_chip_data *d = data;
> +
> + d->flags |= FLAGS_WORKAROUND_CAVIUM_ERRATUM_23154;
> + static_branch_enable(&gic_iar_quirk);
> +
> + return true;
> +}
> +
> static bool gic_enable_quirk_cavium_38539(void *data)
> {
> struct gic_chip_data *d = data;
> @@ -1623,6 +1645,16 @@ static bool gic_enable_quirk_cavium_38539(void *data)
> return true;
> }
>
> +static bool gic_enable_quirk_marvell_38545(void *data)
> +{
> + struct gic_chip_data *d = data;
> +
> + d->flags |= FLAGS_WORKAROUND_MARVELL_ERRATUM_38545;
> + static_branch_enable(&gic_iar_quirk);
> +
> + return true;
> +}
> +
> static bool gic_enable_quirk_hip06_07(void *data)
> {
> struct gic_chip_data *d = data;
> @@ -1660,6 +1692,13 @@ static const struct gic_quirk gic_quirks[] = {
> .iidr = 0x00000000,
> .mask = 0xffffffff,
> .init = gic_enable_quirk_hip06_07,
> + },
> + /* ThunderX: CN88xx 1.x */
> + {
> + .desc = "GICv3: Cavium erratum 23154",
> + .iidr = 0xa101034c,
> + .mask = 0xffff0fff,
> + .init = gic_enable_quirk_cavium_23154,
> },
> {
> /*
> @@ -1674,6 +1713,19 @@ static const struct gic_quirk gic_quirks[] = {
> .mask = 0xe8f00fff,
> .init = gic_enable_quirk_cavium_38539,
> },
> + {
> + /*
> + * IAR register reads could be unreliable, under certain
> + * race conditions. This erratum applies to:
> + * - ThunderX: CN88xx
> + * - OCTEON TX: CN83xx, CN81xx
> + * - OCTEON TX2: CN93xx, CN96xx, CN98xx, CNF95xx*
> + */
> + .desc = "GICv3: Marvell erratum 38545",
> + .iidr = 0xa000034c,
> + .mask = 0xe0f00fff,
> + .init = gic_enable_quirk_marvell_38545,
> + },
I love the fact that you are checking for a a *CPU interface* bug
based on the *distributor* IIDR. What could possibly go wrong? Oh,
don't worry, this only breaks... *all virtual machines*.
Puzzled,
M.
--
Without deviation from the norm, progress is not possible.
Powered by blists - more mailing lists