Re: [PATCH -next] irq-chip/gic-v3-its: Fixed an issue where the ITS executes the residual commands in the queue again when the ITS wakes up from sleep mode.

["xuqiang (M)" <xuqiang36@huawei.com>]

The kernel sends three commands in the following sequence:

1.mapd(deviceA, ITT_addr1, valid:1)

2.mapti(deviceA):ITS write ITT_addr1 memory;

3.mapd(deviceA, ITT_addr1, valid:0) and kfree(ITT_addr1);

4.mapd(deviceA, ITT_addr2, valid:1);

5.mapti(deviceA):ITS write ITT_addr2 memory;

In this case, the processor enters the sleep mode. After the kernel 
performs the suspend operation, the firmware performs the store 
operation and saves GITS_CBASER and GITS_CWRITER registers.

Then, the processor is woken up, and the firmware restores GITS_CBASER 
and GITS_CWRITER registers. Because GITS_CWRITER register is not 0, ITS 
will read the above command sequence execution from the command queue, 
causing ITT_addr1 memory to be trampled.

Thanks,

                 Xu


在 2020/11/4 2:19, Marc Zyngier 写道:
> On Tue, 03 Nov 2020 08:11:23 +0000,
> Xu Qiang <xuqiang36@huawei.com> wrote:
>> During wakeup, the ATF restore interface restores the values of
>> the cbaser and cwriter registers. As a result, the ITS executes
>> the residual commands in the queue, which may cause memory corruption.
>>
>> To solve this problem, clear all data in the command queue
>> in the suspend interface of the ITS driver.
>>
>> Signed-off-by: Xu Qiang <xuqiang36@huawei.com>
>> ---
>>   drivers/irqchip/irq-gic-v3-its.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
>> index 0fec31931e11..b8487f78ac21 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -4741,6 +4741,14 @@ static int its_save_disable(void)
>>   	list_for_each_entry(its, &its_nodes, entry) {
>>   		void __iomem *base;
>>   
>> +		/*
>> +		 * Clear the command queue so that the ITS will not re-execute
>> +		 * the remaining commands in the command queue when
>> +		 * the cwriter and cbaser registers are restored
>> +		 * in the restore interface of the firmware.
>> +		 */
>> +		memset(its->cmd_base, 0, ITS_CMD_QUEUE_SZ);
>> +
>>   		if (!(its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE))
>>   			continue;
> You are wiping the ITS queue before even stopping the ITS. How well is
> that going to work? What if there is something in flight?
>
> I don't understand what you are trying to do here, nor how ATF is
> involved. So please describe the whole sequence of events, and we'll
> decide whether that's something we need to fix.
>
> Thanks,
>
> 	M.
>