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 <[email protected]>
---
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
[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 <[email protected]>
> ---
> 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...
在 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 <[email protected]>
>> ---
>> 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.
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...
在 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.
在 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.
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.
The following commit has been merged into the irq/irqchip-next branch of irqchip:
Commit-ID: 74cde1a53368aed4f2b4b54bf7030437f64a534b
Gitweb: https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms/74cde1a53368aed4f2b4b54bf7030437f64a534b
Author: Xu Qiang <[email protected]>
AuthorDate: Sat, 07 Nov 2020 10:42:26
Committer: Marc Zyngier <[email protected]>
CommitterDate: Sun, 22 Nov 2020 12:58:35
irqchip/gic-v3-its: Unconditionally save/restore the ITS state on suspend
On systems without HW-based collections (i.e. anything except GIC-500),
we rely on firmware to perform the ITS save/restore. This doesn't
really work, as although FW can properly save everything, it cannot
fully restore the state of the command queue (the read-side is reset
to the head of the queue). This results in the ITS consuming previously
processed commands, potentially corrupting the state.
Instead, let's always save the ITS state on suspend, disabling it in the
process, and restore the full state on resume. This saves us from broken
FW as long as it doesn't enable the ITS by itself (for which we can't do
anything).
This amounts to simply dropping the ITS_FLAGS_SAVE_SUSPEND_STATE.
Signed-off-by: Xu Qiang <[email protected]>
[maz: added warning on resume, rewrote commit message]
Signed-off-by: Marc Zyngier <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
drivers/irqchip/irq-gic-v3-its.c | 16 +++-------------
1 file changed, 3 insertions(+), 13 deletions(-)
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 0418071..0598c5c 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 @@ err:
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;
/*
@@ -4794,7 +4784,10 @@ static void its_restore_enable(void)
* don't restore it since writing to CBASER or BASER<n>
* registers is undefined according to the GIC v3 ITS
* Specification.
+ *
+ * Firmware resuming with the ITS enabled is terminally broken.
*/
+ WARN_ON(readl_relaxed(base + GITS_CTLR) & GITS_CTLR_ENABLE);
ret = its_force_quiescent(base);
if (ret) {
pr_err("ITS@%pa: failed to quiesce on resume: %d\n",
@@ -5074,9 +5067,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;