[v4] irqchip/gic-v3-its: Flush ITS tables before writing GITS_BASER<n> registers in non-coherent GIC designs.

[PATCH v4] irqchip/gic-v3-its: Flush ITS tables before writing GITS_BASER<n> registers in non-coherent GIC designs.

Posted by Fang Xiang 2 years, 1 month ago

In non-coherent GIC design, ITS tables should be clean and flushed
to the PoV of the ITS before writing GITS_BASER<n> registers, otherwise
the ITS would read dirty tables and lead to UNPREDICTABLE behaviors.

The ITS always got clean tables in initialization with this fix, by
observing the signals from GIC.

Furthermore, hoist the quirked non-shareable attributes earlier to
save effort in tables setup.

Suggested-by: Marc Zyngier <maz@kernel.org>
Signed-off-by: Fang Xiang <fangxiang3@xiaomi.com>
Tested-by: Fang Xiang <fangxiang3@xiaomi.com>
---
Link to v3:
https://lore.kernel.org/all/20231030070020.3689-1-fangxiang3@xiaomi.com/

Link to v2:
https://lore.kernel.org/all/20231027031007.2088-1-fangxiang3@xiaomi.com/

Link to v1:
https://lore.kernel.org/all/20231026020116.4238-1-fangxiang3@xiaomi.com/

v3-> v4:
    - Change 'Signed-off-by' trailer to 'Suggested-by' for original
      author Marc
    - Fix the format of commit message

v2 -> v3:
    - Add 'Signed-off-by' trailer for original author Marc
    - Expand commit message with testing result

v1 -> v2:
    - Flush ITS tables before writing GITS_BASER<n> registers
    - Hoist the quirked non-shareable attributes earlier
---
 drivers/irqchip/irq-gic-v3-its.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 75a2dd550625..d76d44ea2de1 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -2379,12 +2379,12 @@ static int its_setup_baser(struct its_node *its, struct its_baser *baser,
 		break;
 	}
 
+	if (!shr)
+		gic_flush_dcache_to_poc(base, PAGE_ORDER_TO_SIZE(order));
+
 	its_write_baser(its, baser, val);
 	tmp = baser->val;
 
-	if (its->flags & ITS_FLAGS_FORCE_NON_SHAREABLE)
-		tmp &= ~GITS_BASER_SHAREABILITY_MASK;
-
 	if ((val ^ tmp) & GITS_BASER_SHAREABILITY_MASK) {
 		/*
 		 * Shareability didn't stick. Just use
@@ -2394,10 +2394,9 @@ static int its_setup_baser(struct its_node *its, struct its_baser *baser,
 		 * non-cacheable as well.
 		 */
 		shr = tmp & GITS_BASER_SHAREABILITY_MASK;
-		if (!shr) {
+		if (!shr)
 			cache = GITS_BASER_nC;
-			gic_flush_dcache_to_poc(base, PAGE_ORDER_TO_SIZE(order));
-		}
+
 		goto retry_baser;
 	}
 
@@ -2609,6 +2608,11 @@ static int its_alloc_tables(struct its_node *its)
 		/* erratum 24313: ignore memory access type */
 		cache = GITS_BASER_nCnB;
 
+	if (its->flags & ITS_FLAGS_FORCE_NON_SHAREABLE) {
+		cache = GITS_BASER_nC;
+		shr = 0;
+	}
+
 	for (i = 0; i < GITS_BASER_NR_REGS; i++) {
 		struct its_baser *baser = its->tables + i;
 		u64 val = its_read_baser(its, baser);
-- 
2.34.1

Re: [PATCH v4] irqchip/gic-v3-its: Flush ITS tables before writing GITS_BASER<n> registers in non-coherent GIC designs.

Posted by Marc Zyngier 2 years, 1 month ago

On Mon, 30 Oct 2023 08:32:56 +0000,
Fang Xiang <fangxiang3@xiaomi.com> wrote:
> 
> In non-coherent GIC design, ITS tables should be clean and flushed
> to the PoV of the ITS before writing GITS_BASER<n> registers, otherwise
> the ITS would read dirty tables and lead to UNPREDICTABLE behaviors.
> 
> The ITS always got clean tables in initialization with this fix, by
> observing the signals from GIC.
> 
> Furthermore, hoist the quirked non-shareable attributes earlier to
> save effort in tables setup.
> 
> Suggested-by: Marc Zyngier <maz@kernel.org>
> Signed-off-by: Fang Xiang <fangxiang3@xiaomi.com>
> Tested-by: Fang Xiang <fangxiang3@xiaomi.com>

Reviewed-by: Marc Zyngier <maz@kernel.org>

	M.

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

Re: [PATCH v4] irqchip/gic-v3-its: Flush ITS tables before writing GITS_BASER<n> registers in non-coherent GIC designs.

Posted by Thomas Gleixner 2 years, 1 month ago

On Sat, Nov 04 2023 at 09:56, Marc Zyngier wrote:
> On Mon, 30 Oct 2023 08:32:56 +0000,
> Fang Xiang <fangxiang3@xiaomi.com> wrote:
>> 
>> In non-coherent GIC design, ITS tables should be clean and flushed
>> to the PoV of the ITS before writing GITS_BASER<n> registers, otherwise
>> the ITS would read dirty tables and lead to UNPREDICTABLE behaviors.
>> 
>> The ITS always got clean tables in initialization with this fix, by
>> observing the signals from GIC.
>> 
>> Furthermore, hoist the quirked non-shareable attributes earlier to
>> save effort in tables setup.
>> 
>> Suggested-by: Marc Zyngier <maz@kernel.org>
>> Signed-off-by: Fang Xiang <fangxiang3@xiaomi.com>
>> Tested-by: Fang Xiang <fangxiang3@xiaomi.com>
>
> Reviewed-by: Marc Zyngier <maz@kernel.org>

Shouldn't this have a Fixes tag? My guess is:

a8707f553884 ("irqchip/gic-v3: Add Rockchip 3588001 erratum workaround")

Re: [PATCH v4] irqchip/gic-v3-its: Flush ITS tables before writing GITS_BASER<n> registers in non-coherent GIC designs.

Posted by Marc Zyngier 2 years, 1 month ago

On Sun, 05 Nov 2023 08:55:11 +0000,
Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> On Sat, Nov 04 2023 at 09:56, Marc Zyngier wrote:
> > On Mon, 30 Oct 2023 08:32:56 +0000,
> > Fang Xiang <fangxiang3@xiaomi.com> wrote:
> >> 
> >> In non-coherent GIC design, ITS tables should be clean and flushed
> >> to the PoV of the ITS before writing GITS_BASER<n> registers, otherwise
> >> the ITS would read dirty tables and lead to UNPREDICTABLE behaviors.
> >> 
> >> The ITS always got clean tables in initialization with this fix, by
> >> observing the signals from GIC.
> >> 
> >> Furthermore, hoist the quirked non-shareable attributes earlier to
> >> save effort in tables setup.
> >> 
> >> Suggested-by: Marc Zyngier <maz@kernel.org>
> >> Signed-off-by: Fang Xiang <fangxiang3@xiaomi.com>
> >> Tested-by: Fang Xiang <fangxiang3@xiaomi.com>
> >
> > Reviewed-by: Marc Zyngier <maz@kernel.org>
> 
> Shouldn't this have a Fixes tag? My guess is:
> 
> a8707f553884 ("irqchip/gic-v3: Add Rockchip 3588001 erratum workaround")

Yes, that's indeed the point where the out of sequence programming can
occur.

Thanks,

	M.

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

[tip: irq/urgent] irqchip/gic-v3-its: Flush ITS tables correctly in non-coherent GIC designs

Posted by tip-bot2 for Fang Xiang 2 years, 1 month ago

The following commit has been merged into the irq/urgent branch of tip:

Commit-ID:     d3badb15613c14dd35d3495b1dde5c90fcd616dd
Gitweb:        https://git.kernel.org/tip/d3badb15613c14dd35d3495b1dde5c90fcd616dd
Author:        Fang Xiang <fangxiang3@xiaomi.com>
AuthorDate:    Mon, 30 Oct 2023 16:32:56 +08:00
Committer:     Thomas Gleixner <tglx@linutronix.de>
CommitterDate: Mon, 06 Nov 2023 01:16:33 +01:00

irqchip/gic-v3-its: Flush ITS tables correctly in non-coherent GIC designs

In non-coherent GIC designs, the ITS tables must be flushed before writing
to the GITS_BASER<n> registers, otherwise the ITS could read dirty tables,
which results in unpredictable behavior.

Flush the tables right at the begin of its_setup_baser() to prevent that.

[ tglx: Massage changelog ]

Fixes: a8707f553884 ("irqchip/gic-v3: Add Rockchip 3588001 erratum workaround")
Suggested-by: Marc Zyngier <maz@kernel.org>
Signed-off-by: Fang Xiang <fangxiang3@xiaomi.com>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Reviewed-by: Marc Zyngier <maz@kernel.org>
Link: https://lore.kernel.org/r/20231030083256.4345-1-fangxiang3@xiaomi.com
---
 drivers/irqchip/irq-gic-v3-its.c | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index a8c89df..9a7a742 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -2379,12 +2379,12 @@ retry_baser:
 		break;
 	}
 
+	if (!shr)
+		gic_flush_dcache_to_poc(base, PAGE_ORDER_TO_SIZE(order));
+
 	its_write_baser(its, baser, val);
 	tmp = baser->val;
 
-	if (its->flags & ITS_FLAGS_FORCE_NON_SHAREABLE)
-		tmp &= ~GITS_BASER_SHAREABILITY_MASK;
-
 	if ((val ^ tmp) & GITS_BASER_SHAREABILITY_MASK) {
 		/*
 		 * Shareability didn't stick. Just use
@@ -2394,10 +2394,9 @@ retry_baser:
 		 * non-cacheable as well.
 		 */
 		shr = tmp & GITS_BASER_SHAREABILITY_MASK;
-		if (!shr) {
+		if (!shr)
 			cache = GITS_BASER_nC;
-			gic_flush_dcache_to_poc(base, PAGE_ORDER_TO_SIZE(order));
-		}
+
 		goto retry_baser;
 	}
 
@@ -2609,6 +2608,11 @@ static int its_alloc_tables(struct its_node *its)
 		/* erratum 24313: ignore memory access type */
 		cache = GITS_BASER_nCnB;
 
+	if (its->flags & ITS_FLAGS_FORCE_NON_SHAREABLE) {
+		cache = GITS_BASER_nC;
+		shr = 0;
+	}
+
 	for (i = 0; i < GITS_BASER_NR_REGS; i++) {
 		struct its_baser *baser = its->tables + i;
 		u64 val = its_read_baser(its, baser);