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

[Sascha Bischoff <sascha.bischoff@arm.com>]

On 02/11/2026 15:56, Lorenzo Pieralisi wrote:
> 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.

I've tried to clean up this commit message to give the required context.

Thanks for the feedback!

> 
> Thanks,
> Lorenzo

Thanks,
Sascha

> 
>> 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
>>