Re: [PATCH v2 00/10] irqchip/gic-v5: Fix misc IRS and ITS host driver issues

[Fuad Tabba <fuad.tabba@linux.dev>]

Hi Sascha,

I found a couple more issues in the GICv5 host driver while looking
into pKVM support for GICv5. Rather than explain the fixes, I thought
I'd send you patches for two of them instead. If you think they're
useful, please include them in v3. They apply on top of this series.
Boot tested on the GICv5 FVP with a two-level device table.

For the third one, the thing I spotted is that if the IDLE poll after
an IRS_MAP_L2_ISTR write times out, gicv5_irs_iste_alloc() clears the
level 1 entry and frees the level 2 IST, but nothing cancels the map.
The IRS can still complete it and use the freed table. Keeping the
table isn't enough either, since the next allocation in that range
rewrites the level 1 entry while the map may still be running
(R_SDVFP). vgic_v5_alloc_l2_vmt() and the ITT frees after a timed-out
invalidate have the same shape. The question is, can an IRS miss the
poll, or is a timeout here only ever a dead IRS?

Cheers,
/fuad

--- >8 ---

From: Fuad Tabba <fuad.tabba@linux.dev>
Subject: [PATCH] irqchip/gic-v5: Invalidate the level 1 DTE when registering a device

With a two-level device table, gicv5_its_device_register() makes a
level 1 DTE valid when it allocates the level 2 table that it points
to. However, gicv5_its_device_cache_inv() invalidates the ITS caches
with ITS_INV_DEVICER.L1 clear, which doesn't invalidate the level 1
DTE. An update isn't guaranteed to be visible to the ITS until a cache
invalidation that covers it completes (R_HQFDL in the GICv5
specification). An ITS that cached the level 1 DTE while it was invalid
can keep dropping events for the DeviceIDs in its range.

Set ITS_INV_DEVICER.L1 on every device invalidation. It covers the
level 2 DTE as well, and has no effect with a linear device table.

Fixes: 57d72196dfc85 ("irqchip/gic-v5: Add GICv5 ITS support")
Signed-off-by: Fuad Tabba <fuad.tabba@linux.dev>
---

Notes:
    Setting L1 on every invalidation turns each registration and
    teardown into a range invalidate, both rare paths, and keeps the
    function simple. Setting it only when registration allocates the
    level 2 table would miss a registration that fails after the
    allocation, which leaves the level 1 DTE valid.

 drivers/irqchip/irq-gic-v5-its.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-gic-v5-its.c b/drivers/irqchip/irq-gic-v5-its.c
index 101aceec68f63..83952e30178b1 100644
--- a/drivers/irqchip/irq-gic-v5-its.c
+++ b/drivers/irqchip/irq-gic-v5-its.c
@@ -372,10 +372,11 @@ static int gicv5_its_device_cache_inv(struct gicv5_its_chip_data *its,
 	u64 didr;
 
 	didr = FIELD_PREP(GICV5_ITS_DIDR_DEVICEID, its_dev->device_id);
+	/* L1 also covers an L1 DTE that a registration may have just made valid. */
 	devicer = FIELD_PREP(GICV5_ITS_INV_DEVICER_I, 0x1)	|
 		  FIELD_PREP(GICV5_ITS_INV_DEVICER_EVENTID_BITS,
 			     its_dev->itt_cfg.event_id_bits)	|
-		  FIELD_PREP(GICV5_ITS_INV_DEVICER_L1, 0x0);
+		  FIELD_PREP(GICV5_ITS_INV_DEVICER_L1, 0x1);
 
 	guard(raw_spinlock_irqsave)(&its->inv_lock);
 

--- >8 ---

From: Fuad Tabba <fuad.tabba@linux.dev>
Subject: [PATCH] irqchip/gic-v5: Clean a level 2 device table before making it valid

gicv5_its_alloc_l2_devtab() allocates a zeroed level 2 device table and
writes the level 1 DTE that points to it with VALID set. Unlike the
other ITS tables, the new table isn't cleaned to the PoC first. On a
non-coherent ITS, the zeroes can still sit in the CPU caches when the
ITS reads the table, so it can take stale memory as a valid level 2 DTE
for a DeviceID that nothing has registered.

Clean the table before the level 1 DTE is written.

Fixes: 57d72196dfc85 ("irqchip/gic-v5: Add GICv5 ITS support")
Signed-off-by: Fuad Tabba <fuad.tabba@linux.dev>
---
 drivers/irqchip/irq-gic-v5-its.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/irqchip/irq-gic-v5-its.c b/drivers/irqchip/irq-gic-v5-its.c
index 83952e30178b1..b2d61ba3828cb 100644
--- a/drivers/irqchip/irq-gic-v5-its.c
+++ b/drivers/irqchip/irq-gic-v5-its.c
@@ -416,6 +416,8 @@ static int gicv5_its_alloc_l2_devtab(struct gicv5_its_chip_data *its,
 	if (!l2devtab)
 		return -ENOMEM;
 
+	gicv5_its_dcache_clean(its, l2devtab, BIT(span) * sizeof(*l2devtab));
+
 	its->devtab_cfgr.l2.l2ptrs[l1_index] = l2devtab;
 
 	l1dte = FIELD_PREP(GICV5_DTL1E_SPAN, span)			|