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

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

[Xu Qiang]

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

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.

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

-- 
Without deviation from the norm, progress is not possible.

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

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

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.

[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.
-- 
Jazz is not dead. It just smells funny...

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

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

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.

[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.
-- 
Jazz is not dead. It just smells funny...

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

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

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

[Xu Qiang]

On my platform, ITS_FLAGS_SAVE_SUSPEND_STATE is not set,thus do nothing
in its suspend and resuse function.On the other hand,firmware stores
GITS_CTRL,GITS_CBASER,GITS_CWRITER and GITS_BASER<n> in the suspend,
and restores these registers in the resume. As a result, the ITS executes
the residual commands in the queue.

Memory corruption may occur in the following scenarios:

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;

To solve this problem,dropping the checks for ITS_FLAGS_SAVE_SUSPEND_STATE.

Signed-off-by: Xu Qiang <xuqiang36@huawei.com>
---
 drivers/irqchip/irq-gic-v3-its.c | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 0fec31931e11..06f2c1c252b9 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -42,7 +42,6 @@
 #define ITS_FLAGS_CMDQ_NEEDS_FLUSHING		(1ULL << 0)
 #define ITS_FLAGS_WORKAROUND_CAVIUM_22375	(1ULL << 1)
 #define ITS_FLAGS_WORKAROUND_CAVIUM_23144	(1ULL << 2)
-#define ITS_FLAGS_SAVE_SUSPEND_STATE		(1ULL << 3)
 
 #define RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING	(1 << 0)
 #define RDIST_FLAGS_RD_TABLES_PREALLOCATED	(1 << 1)
@@ -4741,9 +4740,6 @@ static int its_save_disable(void)
 	list_for_each_entry(its, &its_nodes, entry) {
 		void __iomem *base;
 
-		if (!(its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE))
-			continue;
-
 		base = its->base;
 		its->ctlr_save = readl_relaxed(base + GITS_CTLR);
 		err = its_force_quiescent(base);
@@ -4762,9 +4758,6 @@ static int its_save_disable(void)
 		list_for_each_entry_continue_reverse(its, &its_nodes, entry) {
 			void __iomem *base;
 
-			if (!(its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE))
-				continue;
-
 			base = its->base;
 			writel_relaxed(its->ctlr_save, base + GITS_CTLR);
 		}
@@ -4784,9 +4777,6 @@ static void its_restore_enable(void)
 		void __iomem *base;
 		int i;
 
-		if (!(its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE))
-			continue;
-
 		base = its->base;
 
 		/*
@@ -5074,9 +5064,6 @@ static int __init its_probe_one(struct resource *res,
 		ctlr |= GITS_CTLR_ImDe;
 	writel_relaxed(ctlr, its->base + GITS_CTLR);
 
-	if (GITS_TYPER_HCC(typer))
-		its->flags |= ITS_FLAGS_SAVE_SUSPEND_STATE;
-
 	err = its_init_domain(handle, its);
 	if (err)
 		goto out_free_tables;
-- 
2.25.0

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.

[Marc Zyngier]

[dropping Jason, whose email address has been bouncing for weeks now]

On 2020-11-07 10:42, Xu Qiang wrote:
> On my platform, ITS_FLAGS_SAVE_SUSPEND_STATE is not set,thus do nothing

Which platform?

> in its suspend and resuse function.On the other hand,firmware stores
> GITS_CTRL,GITS_CBASER,GITS_CWRITER and GITS_BASER<n> in the suspend,
> and restores these registers in the resume. As a result, the ITS 
> executes
> the residual commands in the queue.

Which firmware are you using? I just had a look at the trusted firmware 
source
code, and while it definitely does something that *looks* like what you 
are
describing, it doesn't re-enable the ITS on resume.

So what are you running?

> 
> Memory corruption may occur in the following scenarios:
> 
> 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);

The ITS doesn't 'kfree' stuff.

> 4.mapd(deviceA, ITT_addr2, valid:1);
> 5.mapti(deviceA):ITS write ITT_addr2 memory;

I don't think this example is relevant. The core of the problem is that
the ITS gets re-enabled by your firmware. What are the affected systems?

> 
> To solve this problem,dropping the checks for 
> ITS_FLAGS_SAVE_SUSPEND_STATE.
> 
> Signed-off-by: Xu Qiang <xuqiang36@huawei.com>
> ---
>  drivers/irqchip/irq-gic-v3-its.c | 13 -------------
>  1 file changed, 13 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c 
> b/drivers/irqchip/irq-gic-v3-its.c
> index 0fec31931e11..06f2c1c252b9 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -42,7 +42,6 @@
>  #define ITS_FLAGS_CMDQ_NEEDS_FLUSHING		(1ULL << 0)
>  #define ITS_FLAGS_WORKAROUND_CAVIUM_22375	(1ULL << 1)
>  #define ITS_FLAGS_WORKAROUND_CAVIUM_23144	(1ULL << 2)
> -#define ITS_FLAGS_SAVE_SUSPEND_STATE		(1ULL << 3)
> 
>  #define RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING	(1 << 0)
>  #define RDIST_FLAGS_RD_TABLES_PREALLOCATED	(1 << 1)
> @@ -4741,9 +4740,6 @@ static int its_save_disable(void)
>  	list_for_each_entry(its, &its_nodes, entry) {
>  		void __iomem *base;
> 
> -		if (!(its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE))
> -			continue;
> -
>  		base = its->base;
>  		its->ctlr_save = readl_relaxed(base + GITS_CTLR);
>  		err = its_force_quiescent(base);
> @@ -4762,9 +4758,6 @@ static int its_save_disable(void)
>  		list_for_each_entry_continue_reverse(its, &its_nodes, entry) {
>  			void __iomem *base;
> 
> -			if (!(its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE))
> -				continue;
> -
>  			base = its->base;
>  			writel_relaxed(its->ctlr_save, base + GITS_CTLR);
>  		}
> @@ -4784,9 +4777,6 @@ static void its_restore_enable(void)
>  		void __iomem *base;
>  		int i;
> 
> -		if (!(its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE))
> -			continue;
> -
>  		base = its->base;
> 
>  		/*
> @@ -5074,9 +5064,6 @@ static int __init its_probe_one(struct resource 
> *res,
>  		ctlr |= GITS_CTLR_ImDe;
>  	writel_relaxed(ctlr, its->base + GITS_CTLR);
> 
> -	if (GITS_TYPER_HCC(typer))
> -		its->flags |= ITS_FLAGS_SAVE_SUSPEND_STATE;
> -
>  	err = its_init_domain(handle, its);
>  	if (err)
>  		goto out_free_tables;

I'm OK with the patch itself, but I don't want to paper over broken 
firmware.
I'll get TF-A fixed one way or another, but I want to be sure yours is 
too.
If firmware does its job correctly, we shouldn't have to do all of this.

         M.
-- 
Jazz is not dead. It just smells funny...

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

在 2020/11/8 0:54, Marc Zyngier 写道:
> [dropping Jason, whose email address has been bouncing for weeks now]
>
> On 2020-11-07 10:42, Xu Qiang wrote:
>> On my platform, ITS_FLAGS_SAVE_SUSPEND_STATE is not set,thus do nothing
>
> Which platform?
Hisi Ascend platform
>
>> in its suspend and resuse function.On the other hand,firmware stores
>> GITS_CTRL,GITS_CBASER,GITS_CWRITER and GITS_BASER<n> in the suspend,
>> and restores these registers in the resume. As a result, the ITS 
>> executes
>> the residual commands in the queue.
>
> Which firmware are you using? I just had a look at the trusted 
> firmware source
> code, and while it definitely does something that *looks* like what 
> you are
> describing, it doesn't re-enable the ITS on resume.
>
> So what are you running?

I am using ATF. Since ITS_FLAGS_SAVE_SUSPEND_STATE is not set,ITS driver 
of OS will

not re-enable ITS in th resume. To make ITS work properly, we changed 
the ATF code

to re-enable ITS on resume.

>
>>
>> Memory corruption may occur in the following scenarios:
>>
>> 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);
>
> The ITS doesn't 'kfree' stuff.
ITS driver kfree ITT_addr1.
>
>> 4.mapd(deviceA, ITT_addr2, valid:1);
>> 5.mapti(deviceA):ITS write ITT_addr2 memory;
>
> I don't think this example is relevant. The core of the problem is that
> the ITS gets re-enabled by your firmware. What are the affected systems?
>
>>
>> To solve this problem,dropping the checks for 
>> ITS_FLAGS_SAVE_SUSPEND_STATE.
>>
>> Signed-off-by: Xu Qiang <xuqiang36@huawei.com>
>> ---
>>  drivers/irqchip/irq-gic-v3-its.c | 13 -------------
>>  1 file changed, 13 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c 
>> b/drivers/irqchip/irq-gic-v3-its.c
>> index 0fec31931e11..06f2c1c252b9 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -42,7 +42,6 @@
>>  #define ITS_FLAGS_CMDQ_NEEDS_FLUSHING        (1ULL << 0)
>>  #define ITS_FLAGS_WORKAROUND_CAVIUM_22375    (1ULL << 1)
>>  #define ITS_FLAGS_WORKAROUND_CAVIUM_23144    (1ULL << 2)
>> -#define ITS_FLAGS_SAVE_SUSPEND_STATE        (1ULL << 3)
>>
>>  #define RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING    (1 << 0)
>>  #define RDIST_FLAGS_RD_TABLES_PREALLOCATED    (1 << 1)
>> @@ -4741,9 +4740,6 @@ static int its_save_disable(void)
>>      list_for_each_entry(its, &its_nodes, entry) {
>>          void __iomem *base;
>>
>> -        if (!(its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE))
>> -            continue;
>> -
>>          base = its->base;
>>          its->ctlr_save = readl_relaxed(base + GITS_CTLR);
>>          err = its_force_quiescent(base);
>> @@ -4762,9 +4758,6 @@ static int its_save_disable(void)
>>          list_for_each_entry_continue_reverse(its, &its_nodes, entry) {
>>              void __iomem *base;
>>
>> -            if (!(its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE))
>> -                continue;
>> -
>>              base = its->base;
>>              writel_relaxed(its->ctlr_save, base + GITS_CTLR);
>>          }
>> @@ -4784,9 +4777,6 @@ static void its_restore_enable(void)
>>          void __iomem *base;
>>          int i;
>>
>> -        if (!(its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE))
>> -            continue;
>> -
>>          base = its->base;
>>
>>          /*
>> @@ -5074,9 +5064,6 @@ static int __init its_probe_one(struct resource 
>> *res,
>>          ctlr |= GITS_CTLR_ImDe;
>>      writel_relaxed(ctlr, its->base + GITS_CTLR);
>>
>> -    if (GITS_TYPER_HCC(typer))
>> -        its->flags |= ITS_FLAGS_SAVE_SUSPEND_STATE;
>> -
>>      err = its_init_domain(handle, its);
>>      if (err)
>>          goto out_free_tables;
>
> I'm OK with the patch itself, but I don't want to paper over broken 
> firmware.
> I'll get TF-A fixed one way or another, but I want to be sure yours is 
> too.
> If firmware does its job correctly, we shouldn't have to do all of this.
>
>         M.

Thanks

         Xu.

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.

[Marc Zyngier]

On 2020-11-09 03:05, xuqiang (M) wrote:
> 在 2020/11/8 0:54, Marc Zyngier 写道:
>> [dropping Jason, whose email address has been bouncing for weeks now]
>> 
>> On 2020-11-07 10:42, Xu Qiang wrote:
>>> On my platform, ITS_FLAGS_SAVE_SUSPEND_STATE is not set,thus do 
>>> nothing
>> 
>> Which platform?
> Hisi Ascend platform
>> 
>>> in its suspend and resuse function.On the other hand,firmware stores
>>> GITS_CTRL,GITS_CBASER,GITS_CWRITER and GITS_BASER<n> in the suspend,
>>> and restores these registers in the resume. As a result, the ITS 
>>> executes
>>> the residual commands in the queue.
>> 
>> Which firmware are you using? I just had a look at the trusted 
>> firmware source
>> code, and while it definitely does something that *looks* like what 
>> you are
>> describing, it doesn't re-enable the ITS on resume.
>> 
>> So what are you running?
> 
> I am using ATF. Since ITS_FLAGS_SAVE_SUSPEND_STATE is not set,ITS
> driver of OS will
> 
> not re-enable ITS in th resume. To make ITS work properly, we changed
> the ATF code
> 
> to re-enable ITS on resume.

I don't think the words "work properly" apply here.

The kernel didn't do what you wanted, so instead of fixing the kernel, 
you
introduced a bug that results in memory corruption from the firmware.

What are you plans to fix your firmware? Because from an upstream ATF
compatibility PoV, all there is to do is to fixup the command queue and
enable the ITS.

         M.
-- 
Jazz is not dead. It just smells funny...

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

在 2020/11/9 18:43, Marc Zyngier 写道:
> On 2020-11-09 03:05, xuqiang (M) wrote:
>> 在 2020/11/8 0:54, Marc Zyngier 写道:
>>> [dropping Jason, whose email address has been bouncing for weeks now]
>>>
>>> On 2020-11-07 10:42, Xu Qiang wrote:
>>>> On my platform, ITS_FLAGS_SAVE_SUSPEND_STATE is not set,thus do 
>>>> nothing
>>>
>>> Which platform?
>> Hisi Ascend platform
>>>
>>>> in its suspend and resuse function.On the other hand,firmware stores
>>>> GITS_CTRL,GITS_CBASER,GITS_CWRITER and GITS_BASER<n> in the suspend,
>>>> and restores these registers in the resume. As a result, the ITS 
>>>> executes
>>>> the residual commands in the queue.
>>>
>>> Which firmware are you using? I just had a look at the trusted 
>>> firmware source
>>> code, and while it definitely does something that *looks* like what 
>>> you are
>>> describing, it doesn't re-enable the ITS on resume.
>>>
>>> So what are you running?
>>
>> I am using ATF. Since ITS_FLAGS_SAVE_SUSPEND_STATE is not set,ITS
>> driver of OS will
>>
>> not re-enable ITS in th resume. To make ITS work properly, we changed
>> the ATF code
>>
>> to re-enable ITS on resume.
>
> I don't think the words "work properly" apply here.
>
> The kernel didn't do what you wanted, so instead of fixing the kernel, 
> you
> introduced a bug that results in memory corruption from the firmware.
>
> What are you plans to fix your firmware? Because from an upstream ATF
> compatibility PoV, all there is to do is to fixup the command queue and
> enable the ITS.
>
>         M.


I'm sorry I didn't make it clear how to do this. I'm going to reset commit

which re-enable ITS  on the ATF, and drop the checks for 
ITS_FLAGS_SAVE_SUSPEND_STATE

in the OS.

In other words, the ATF does not re-enable ITS, and OS itself re-enables 
ITS when it resumes.

To do this, I have to remove the check of ITS_FLAGS_SAVE_SUSPEND_STATE 
because it is not set.


Thanks

         Xu.

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

在 2020/11/10 17:09, xuqiang (M) 写道:
>
> 在 2020/11/9 18:43, Marc Zyngier 写道:
>> On 2020-11-09 03:05, xuqiang (M) wrote:
>>> 在 2020/11/8 0:54, Marc Zyngier 写道:
>>>> [dropping Jason, whose email address has been bouncing for weeks now]
>>>>
>>>> On 2020-11-07 10:42, Xu Qiang wrote:
>>>>> On my platform, ITS_FLAGS_SAVE_SUSPEND_STATE is not set,thus do 
>>>>> nothing
>>>>
>>>> Which platform?
>>> Hisi Ascend platform
>>>>
>>>>> in its suspend and resuse function.On the other hand,firmware stores
>>>>> GITS_CTRL,GITS_CBASER,GITS_CWRITER and GITS_BASER<n> in the suspend,
>>>>> and restores these registers in the resume. As a result, the ITS 
>>>>> executes
>>>>> the residual commands in the queue.
>>>>
>>>> Which firmware are you using? I just had a look at the trusted 
>>>> firmware source
>>>> code, and while it definitely does something that *looks* like what 
>>>> you are
>>>> describing, it doesn't re-enable the ITS on resume.
>>>>
>>>> So what are you running?
>>>
>>> I am using ATF. Since ITS_FLAGS_SAVE_SUSPEND_STATE is not set,ITS
>>> driver of OS will
>>>
>>> not re-enable ITS in th resume. To make ITS work properly, we changed
>>> the ATF code
>>>
>>> to re-enable ITS on resume.
>>
>> I don't think the words "work properly" apply here.
>>
>> The kernel didn't do what you wanted, so instead of fixing the 
>> kernel, you
>> introduced a bug that results in memory corruption from the firmware.
>>
>> What are you plans to fix your firmware? Because from an upstream ATF
>> compatibility PoV, all there is to do is to fixup the command queue and
>> enable the ITS.
>>
>>         M.
>
>
> I'm sorry I didn't make it clear how to do this. I'm going to reset 
> commit
>
> which re-enable ITS  on the ATF, and drop the checks for 
> ITS_FLAGS_SAVE_SUSPEND_STATE
>
> in the OS.
>
> In other words, the ATF does not re-enable ITS, and OS itself 
> re-enables ITS when it resumes.
>
> To do this, I have to remove the check of ITS_FLAGS_SAVE_SUSPEND_STATE 
> because it is not set.
>
>
> Thanks
>
>         Xu.
>
Hi Marc

     I have been waiting for your reply. Are there any questions about 
this modification plan?

Thanks

         Xu.

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.

[Marc Zyngier]

On Sat, 7 Nov 2020 10:42:26 +0000, Xu Qiang wrote:
> On my platform, ITS_FLAGS_SAVE_SUSPEND_STATE is not set,thus do nothing
> in its suspend and resuse function.On the other hand,firmware stores
> GITS_CTRL,GITS_CBASER,GITS_CWRITER and GITS_BASER<n> in the suspend,
> and restores these registers in the resume. As a result, the ITS executes
> the residual commands in the queue.
> 
> Memory corruption may occur in the following scenarios:
> 
> [...]

Applied to irq/irqchip-next, thanks!

[1/1] irqchip/gic-v3-its: Unconditionally save/restore the ITS state on suspend
      commit: a51f7296f38f498c6f186c82ae3aa25ae10bb266

Cheers,

	M.
-- 
Without deviation from the norm, progress is not possible.