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 <[email protected]>
---
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;
--
2.25.0
On Tue, 03 Nov 2020 08:11:23 +0000,
Xu Qiang <[email protected]> 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 <[email protected]>
> ---
> 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.
--
Without deviation from the norm, progress is not possible.
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 <[email protected]> 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 <[email protected]>
>> ---
>> 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.
>
Please don't top-post.
On 2020-11-05 11:54, xuqiang (M) wrote:
> 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.
This cannot work. By doing a memset on the command queue, you are
only feeding crap to the ITS (command 0 simply does not exist).
Consider yourself lucky that it doesn't just lock-up.
What needs to happen is the restore sequence that is already in the
driver, so that the command queue is in a sane state before re-enabling
the ITS.
M.
--
Jazz is not dead. It just smells funny...
在 2020/11/5 21:12, Marc Zyngier 写道:
> Please don't top-post.
>
> On 2020-11-05 11:54, xuqiang (M) wrote:
>> 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.
>
> This cannot work. By doing a memset on the command queue, you are
> only feeding crap to the ITS (command 0 simply does not exist).
> Consider yourself lucky that it doesn't just lock-up.
>
> What needs to happen is the restore sequence that is already in the
> driver, so that the command queue is in a sane state before re-enabling
> the ITS.
>
> M.
On my platform, ITS_FLAGS_SAVE_SUSPEND_STATE is not set, thus
the first if condition in its_save_disable under list_for_each_entry goes
to the continue, however, I want to set the GITS_CWRITER to 0 at the
end of list_for_each_entry.
Do you have any suggestions about how to do this?
On 2020-11-05 14:06, xuqiang (M) wrote:
> 在 2020/11/5 21:12, Marc Zyngier 写道:
>> Please don't top-post.
>>
>> On 2020-11-05 11:54, xuqiang (M) wrote:
>>> 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.
>>
>> This cannot work. By doing a memset on the command queue, you are
>> only feeding crap to the ITS (command 0 simply does not exist).
>> Consider yourself lucky that it doesn't just lock-up.
>>
>> What needs to happen is the restore sequence that is already in the
>> driver, so that the command queue is in a sane state before
>> re-enabling
>> the ITS.
>>
>> M.
>
>
> On my platform, ITS_FLAGS_SAVE_SUSPEND_STATE is not set, thus
> the first if condition in its_save_disable under list_for_each_entry
> goes
> to the continue, however, I want to set the GITS_CWRITER to 0 at the
> end of list_for_each_entry.
>
> Do you have any suggestions about how to do this?
That's pretty much dropping the checks for ITS_FLAGS_SAVE_SUSPEND_STATE,
isn't it? But I assume your ITS is already enabled by the time you
reenter
the kernel? If so, I bet your firmware is doing more than just writing
to CBASER and CWRITER...
M.
--
Jazz is not dead. It just smells funny...
在 2020/11/5 22:24, Marc Zyngier 写道:
> On 2020-11-05 14:06, xuqiang (M) wrote:
>> 在 2020/11/5 21:12, Marc Zyngier 写道:
>>> Please don't top-post.
>>>
>>> On 2020-11-05 11:54, xuqiang (M) wrote:
>>>> 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.
>>>
>>> This cannot work. By doing a memset on the command queue, you are
>>> only feeding crap to the ITS (command 0 simply does not exist).
>>> Consider yourself lucky that it doesn't just lock-up.
>>>
>>> What needs to happen is the restore sequence that is already in the
>>> driver, so that the command queue is in a sane state before re-enabling
>>> the ITS.
>>>
>>> M.
>>
>>
>> On my platform, ITS_FLAGS_SAVE_SUSPEND_STATE is not set, thus
>> the first if condition in its_save_disable under list_for_each_entry
>> goes
>> to the continue, however, I want to set the GITS_CWRITER to 0 at the
>> end of list_for_each_entry.
>>
>> Do you have any suggestions about how to do this?
>
> That's pretty much dropping the checks for ITS_FLAGS_SAVE_SUSPEND_STATE,
> isn't it? But I assume your ITS is already enabled by the time you
> reenter
> the kernel? If so, I bet your firmware is doing more than just writing
> to CBASER and CWRITER...
>
> M.
Yes, you are right.My firmware stores GITS_CTRL,GITS_CBASER,GITS_CWRITER
and GITS_BASER<n> in the suspend, and restores these registers in the
resume.
So I will drop the checks for ITS_FLAGS_SAVE_SUSPEND_STATE. In the case
of ITS disabled,
set the GITS_CWRITER to 0.
Thanks
Xu.