Re: [PATCH 04/10] irqchip/gic-v5: Serialise ITS invalidation commands

[Lorenzo Pieralisi <lpieralisi@kernel.org>]

On Mon, Nov 02, 2026 at 01:32:57PM +0000, Sascha Bischoff wrote:
> The ITS uses DIDR and EIDR to select the translation to invalidate.
> These registers are shared by all interrupts targeting the ITS. Event
> invalidation can run concurrently for different IRQs, and device
> invalidation can overlap it. The per-IRQ descriptor lock does not
> serialise the commands, so one CPU can accidentally issue an
> invalidation using another CPU's DeviceID or EventID.

"Another CPU's DeviceID or EventID " can probably be explained better
(or just removed because it is unclear) I understand there is a link to
Sashiko's report but it is worth updating the log IMO.

"using the DeviceID or EventID associated with an IRQ that is invalidated
on another CPU" or something like that.

> Add a raw spinlock to each ITS and hold it across the command register
> writes and the STATUSR.IDLE poll for both invalidation paths. A raw
> spinlock is required because the forwarding path calls the event

Can you please define "the forwarding path" more explicitly ?  See above.

Thanks,
Lorenzo

> invalidation helper with the IRQ descriptor lock held.
> 
> Fixes: 57d72196dfc8 ("irqchip/gic-v5: Add GICv5 ITS support")
> Reported-by: Sashiko AI <sashiko-bot@kernel.org>
> Link: https://sashiko.dev/#/patchset/20261016155741.979985-1-sascha.bischoff@arm.com?part=11
> Cc: stable@vger.kernel.org
> Signed-off-by: Sascha Bischoff <sascha.bischoff@arm.com>
> ---
>  drivers/irqchip/irq-gic-v5-its.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/irqchip/irq-gic-v5-its.c b/drivers/irqchip/irq-gic-v5-its.c
> index 28e39b065de0e..6d98f3dbe34b6 100644
> --- a/drivers/irqchip/irq-gic-v5-its.c
> +++ b/drivers/irqchip/irq-gic-v5-its.c
> @@ -16,6 +16,7 @@
>  #include <linux/of_address.h>
>  #include <linux/of_irq.h>
>  #include <linux/slab.h>
> +#include <linux/spinlock.h>
>  
>  #include <linux/irqchip.h>
>  #include <linux/irqchip/arm-gic-v5.h>
> @@ -28,6 +29,8 @@
>  struct gicv5_its_chip_data {
>  	struct	xarray			its_devices;
>  	struct	mutex			dev_alloc_lock;
> +	/* Event and device invalidation commands share DIDR. */
> +	raw_spinlock_t			inv_lock;
>  	struct	fwnode_handle		*fwnode;
>  	struct gicv5_its_devtab_cfg	devtab_cfgr;
>  	void	__iomem			*its_base;
> @@ -129,6 +132,8 @@ static int gicv5_its_itt_cache_inv(struct gicv5_its_chip_data *its, u32 device_i
>  	eidr = FIELD_PREP(GICV5_ITS_EIDR_EVENTID, event_id);
>  	eventr = FIELD_PREP(GICV5_ITS_INV_EVENTR_I, 0x1);
>  
> +	guard(raw_spinlock_irqsave)(&its->inv_lock);
> +
>  	its_writeq_relaxed(its, didr, GICV5_ITS_DIDR);
>  	its_writel_relaxed(its, eidr, GICV5_ITS_EIDR);
>  	its_writel_relaxed(its, eventr, GICV5_ITS_INV_EVENTR);
> @@ -371,6 +376,9 @@ static int gicv5_its_device_cache_inv(struct gicv5_its_chip_data *its,
>  		  FIELD_PREP(GICV5_ITS_INV_DEVICER_EVENTID_BITS,
>  			     its_dev->itt_cfg.event_id_bits)	|
>  		  FIELD_PREP(GICV5_ITS_INV_DEVICER_L1, 0x0);
> +
> +	guard(raw_spinlock_irqsave)(&its->inv_lock);
> +
>  	its_writeq_relaxed(its, didr, GICV5_ITS_DIDR);
>  	its_writel_relaxed(its, devicer, GICV5_ITS_INV_DEVICER);
>  
> @@ -1112,6 +1120,7 @@ static int __init gicv5_its_init_bases(void __iomem *its_base, struct fwnode_han
>  		return -ENOMEM;
>  
>  	mutex_init(&its_node->dev_alloc_lock);
> +	raw_spin_lock_init(&its_node->inv_lock);
>  	xa_init(&its_node->its_devices);
>  	its_node->fwnode = handle;
>  	its_node->its_base = its_base;
> 
> -- 
> 2.34.1
>