Hi Yaohongbo,
In the future, please refrain from sending HTML emails, they
don't render very well and force me to reformat your email
by hand.
On 2019-12-02 12:52, yaohongbo wrote:
> Hi, marc.
>
> I met a problem with GIC ITS when I try to power off gic logic in
> suspend.
>
> In hisilicon hip08, the value of GIC_TYPER.HCC is zero, so that
> ITS_FLAGS_SAVE_SUSPEND_STATE will have no chance to be set to 1.
And that's a good thing. HCC indicates that you have collections that
are backed by registers, and not memory. Which means that once the GIC
is powered off, the state is lost.
> It goes well for s4, when I simply remove the condition judgement in
> the code.
What is "s4"? Doing so means you are reprogramming the ITS with
mappings
that already exist in the tables, and that is UNPRED territory.
<quote>
Behavior is unpredictable if there are interrupts that are mapped to
the
specified collection and the collection is currently mapped to a
Redistributor
[...]
</quote>
> --- a/drivers/irqchip/irq-gic-v3-its.c
>
> +++ b/drivers/irqchip/irq-gic-v3-its.c
>
> @@ -3670,8 +3670,8 @@ 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;
>
> + its->flags |= ITS_FLAGS_SAVE_SUSPEND_STATE;
>
> err = its_init_domain(handle, its);
>
> if (err)
>
> @@ -4005,3 +4005,17 @@ int __init its_init(struct fwnode_handle
> *handle, struct rdists *rdists,
>
> return 0;
>
> }
>
> Do you have any suggestion for this case?
The expectations are that across a GIC power-off, the firmware
will restore the state of the GIC (recondiguring the various
memory tables), and that this is enough for the ITS to be
functional again, having reloaded its state from memory.
Does firmware perform this on your machine? Or are there
implementation-specific issues that require the ITS to be
reprogrammed?
Thanks,
M.
--
Jazz is not dead. It just smells funny...
On 12/2/2019 9:22 PM, Marc Zyngier wrote:
> Hi Yaohongbo,
>
> In the future, please refrain from sending HTML emails, they
> don't render very well and force me to reformat your email
> by hand.
Sorry. I'll pay attention to this next time.
> On 2019-12-02 12:52, yaohongbo wrote:
>> Hi, marc.
>>
>> I met a problem with GIC ITS when I try to power off gic logic in
>> suspend.
>>
>> In hisilicon hip08, the value of GIC_TYPER.HCC is zero, so that
>> ITS_FLAGS_SAVE_SUSPEND_STATE will have no chance to be set to 1.
>
> And that's a good thing. HCC indicates that you have collections that
> are backed by registers, and not memory. Which means that once the GIC
> is powered off, the state is lost.
>
>> It goes well for s4, when I simply remove the condition judgement in
>> the code.
>
> What is "s4"? Doing so means you are reprogramming the ITS with mappings
> that already exist in the tables, and that is UNPRED territory.
Sorry, I didn't describe it clearly.
S4 means "suspend to disk".
In s4, The its will reinit and malloc an new its address.
My expectation is to reprogram the ITS with original mappings. If ITS_FLAGS_SAVE_SUSPEND_STATE
is not set, i'll have no chance to use the original its table mappings.
What should i do if i want to restore its state with hcc == 0?
Thanks,
Hongbo.
> <quote>
> Behavior is unpredictable if there are interrupts that are mapped to the
> specified collection and the collection is currently mapped to a Redistributor
> [...]
> </quote>
>
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>>
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>>
>> @@ -3670,8 +3670,8 @@ 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;
>>
>> + its->flags |= ITS_FLAGS_SAVE_SUSPEND_STATE;
>>
>> err = its_init_domain(handle, its);
>>
>> if (err)
>>
>> @@ -4005,3 +4005,17 @@ int __init its_init(struct fwnode_handle
>> *handle, struct rdists *rdists,
>>
>> return 0;
>>
>> }
>>
>> Do you have any suggestion for this case?
>
> The expectations are that across a GIC power-off, the firmware
> will restore the state of the GIC (recondiguring the various
> memory tables), and that this is enough for the ITS to be
> functional again, having reloaded its state from memory.
>
> Does firmware perform this on your machine? Or are there
> implementation-specific issues that require the ITS to be
> reprogrammed?
>
> Thanks,
>
> M.
+ James, who wrote most (if not all) of the arm64 hibernate code
On 2019-12-03 02:23, Yao HongBo wrote:
> On 12/2/2019 9:22 PM, Marc Zyngier wrote:
>> Hi Yaohongbo,
>>
>> In the future, please refrain from sending HTML emails, they
>> don't render very well and force me to reformat your email
>> by hand.
>
> Sorry. I'll pay attention to this next time.
>
>> On 2019-12-02 12:52, yaohongbo wrote:
>>> Hi, marc.
>>>
>>> I met a problem with GIC ITS when I try to power off gic logic in
>>> suspend.
>>>
>>> In hisilicon hip08, the value of GIC_TYPER.HCC is zero, so that
>>> ITS_FLAGS_SAVE_SUSPEND_STATE will have no chance to be set to 1.
>>
>> And that's a good thing. HCC indicates that you have collections
>> that
>> are backed by registers, and not memory. Which means that once the
>> GIC
>> is powered off, the state is lost.
>>
>>> It goes well for s4, when I simply remove the condition judgement
>>> in
>>> the code.
>>
>> What is "s4"? Doing so means you are reprogramming the ITS with
>> mappings
>> that already exist in the tables, and that is UNPRED territory.
>
> Sorry, I didn't describe it clearly.
> S4 means "suspend to disk".
> In s4, The its will reinit and malloc an new its address.
Huh, hibernate... Yeah, this is not expected to work, I'm afraid.
> My expectation is to reprogram the ITS with original mappings. If
> ITS_FLAGS_SAVE_SUSPEND_STATE
> is not set, i'll have no chance to use the original its table
> mappings.
>
> What should i do if i want to restore its state with hcc == 0?
HCC is the least of the problems, and there are plenty more issues:
- I'm not sure what guarantees that the tables are at the same
address in the booting kernel and the the resumed kernel.
That covers all the ITS tables and as well as the RDs'.
- It could well be that restoring the ITS base addresses is enough
for everything to resume correctly, but this needs some serious
investigation. Worse case, we will need to replay the whole of
the ITS programming.
- This is going to interact more or less badly with the normal suspend
to RAM code...
- The ITS is only the tip of the iceberg. The whole of the SMMU setup
needs to be replayed, or devices won't resume correctly (I just tried
on a D05).
Anyway, with the hack below, I've been able to get D05 to resume
up to the point where devices try to do DMA, and then it was dead.
There is definitely some work to be done there...
M.
diff --git a/drivers/irqchip/irq-gic-v3-its.c
b/drivers/irqchip/irq-gic-v3-its.c
index 4ba31de4a875..a05fc6bac203 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -27,6 +27,7 @@
#include <linux/of_platform.h>
#include <linux/percpu.h>
#include <linux/slab.h>
+#include <linux/suspend.h>
#include <linux/syscore_ops.h>
#include <linux/irqchip.h>
@@ -42,6 +43,7 @@
#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 ITS_FLAGS_SAVE_HIBERNATE_STATE (1ULL << 4)
#define RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING (1 << 0)
#define RDIST_FLAGS_RD_TABLES_PREALLOCATED (1 << 1)
@@ -3517,8 +3519,16 @@ static int its_save_disable(void)
raw_spin_lock(&its_lock);
list_for_each_entry(its, &its_nodes, entry) {
void __iomem *base;
+ u64 flags;
- if (!(its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE))
+ if (system_entering_hibernation())
+ its->flags |= ITS_FLAGS_SAVE_HIBERNATE_STATE;
+
+ flags = its->flags;
+ flags &= (ITS_FLAGS_SAVE_SUSPEND_STATE |
+ ITS_FLAGS_SAVE_HIBERNATE_STATE);
+
+ if (!flags)
continue;
base = its->base;
@@ -3559,11 +3569,17 @@ static void its_restore_enable(void)
raw_spin_lock(&its_lock);
list_for_each_entry(its, &its_nodes, entry) {
void __iomem *base;
+ u64 flags;
int i;
- if (!(its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE))
+ flags = its->flags;
+ flags &= (ITS_FLAGS_SAVE_SUSPEND_STATE |
+ ITS_FLAGS_SAVE_HIBERNATE_STATE);
+ if (!flags)
continue;
+ its->flags &= ~ITS_FLAGS_SAVE_HIBERNATE_STATE;
+
base = its->base;
/*
--
Jazz is not dead. It just smells funny...
On Tue, Dec 3, 2019 at 9:46 PM Marc Zyngier <[email protected]> wrote:
>
> + James, who wrote most (if not all) of the arm64 hibernate code
>
> On 2019-12-03 02:23, Yao HongBo wrote:
> > On 12/2/2019 9:22 PM, Marc Zyngier wrote:
> >> Hi Yaohongbo,
> >>
> >> In the future, please refrain from sending HTML emails, they
> >> don't render very well and force me to reformat your email
> >> by hand.
> >
> > Sorry. I'll pay attention to this next time.
> >
> >> On 2019-12-02 12:52, yaohongbo wrote:
> >>> Hi, marc.
> >>>
> >>> I met a problem with GIC ITS when I try to power off gic logic in
> >>> suspend.
> >>>
> >>> In hisilicon hip08, the value of GIC_TYPER.HCC is zero, so that
> >>> ITS_FLAGS_SAVE_SUSPEND_STATE will have no chance to be set to 1.
> >>
> >> And that's a good thing. HCC indicates that you have collections
> >> that
> >> are backed by registers, and not memory. Which means that once the
> >> GIC
> >> is powered off, the state is lost.
> >>
> >>> It goes well for s4, when I simply remove the condition judgement
> >>> in
> >>> the code.
> >>
> >> What is "s4"? Doing so means you are reprogramming the ITS with
> >> mappings
> >> that already exist in the tables, and that is UNPRED territory.
> >
> > Sorry, I didn't describe it clearly.
> > S4 means "suspend to disk".
> > In s4, The its will reinit and malloc an new its address.
>
> Huh, hibernate... Yeah, this is not expected to work, I'm afraid.
>
> > My expectation is to reprogram the ITS with original mappings. If
> > ITS_FLAGS_SAVE_SUSPEND_STATE
> > is not set, i'll have no chance to use the original its table
> > mappings.
> >
> > What should i do if i want to restore its state with hcc == 0?
>
> HCC is the least of the problems, and there are plenty more issues:
>
> - I'm not sure what guarantees that the tables are at the same
> address in the booting kernel and the the resumed kernel.
> That covers all the ITS tables and as well as the RDs'.
>
> - It could well be that restoring the ITS base addresses is enough
> for everything to resume correctly, but this needs some serious
> investigation. Worse case, we will need to replay the whole of
> the ITS programming.
>
> - This is going to interact more or less badly with the normal suspend
> to RAM code...
>
> - The ITS is only the tip of the iceberg. The whole of the SMMU setup
> needs to be replayed, or devices won't resume correctly (I just tried
> on a D05).
>
> Anyway, with the hack below, I've been able to get D05 to resume
> up to the point where devices try to do DMA, and then it was dead.
> There is definitely some work to be done there...
>
> M.
>
> diff --git a/drivers/irqchip/irq-gic-v3-its.c
> b/drivers/irqchip/irq-gic-v3-its.c
> index 4ba31de4a875..a05fc6bac203 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -27,6 +27,7 @@
> #include <linux/of_platform.h>
> #include <linux/percpu.h>
> #include <linux/slab.h>
> +#include <linux/suspend.h>
> #include <linux/syscore_ops.h>
>
> #include <linux/irqchip.h>
> @@ -42,6 +43,7 @@
> #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 ITS_FLAGS_SAVE_HIBERNATE_STATE (1ULL << 4)
>
> #define RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING (1 << 0)
> #define RDIST_FLAGS_RD_TABLES_PREALLOCATED (1 << 1)
> @@ -3517,8 +3519,16 @@ static int its_save_disable(void)
> raw_spin_lock(&its_lock);
> list_for_each_entry(its, &its_nodes, entry) {
> void __iomem *base;
> + u64 flags;
>
> - if (!(its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE))
> + if (system_entering_hibernation())
> + its->flags |= ITS_FLAGS_SAVE_HIBERNATE_STATE;
> +
> + flags = its->flags;
> + flags &= (ITS_FLAGS_SAVE_SUSPEND_STATE |
> + ITS_FLAGS_SAVE_HIBERNATE_STATE);
> +
> + if (!flags)
> continue;
>
> base = its->base;
> @@ -3559,11 +3569,17 @@ static void its_restore_enable(void)
> raw_spin_lock(&its_lock);
> list_for_each_entry(its, &its_nodes, entry) {
> void __iomem *base;
> + u64 flags;
> int i;
>
> - if (!(its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE))
> + flags = its->flags;
> + flags &= (ITS_FLAGS_SAVE_SUSPEND_STATE |
> + ITS_FLAGS_SAVE_HIBERNATE_STATE);
> + if (!flags)
> continue;
>
> + its->flags &= ~ITS_FLAGS_SAVE_HIBERNATE_STATE;
> +
> base = its->base;
>
> /*
How about this one to reinit GIC for restore:
- https://source.codeaurora.org/quic/la/kernel/msm-4.14/commit/drivers/irqchip/irq-gic-v3.c?h=msm-4.14&id=b0079fb73c08e195498ba2b2ea9623b0cc0f5fed
On Wed, 04 Dec 2019 06:13:23 +0000,
Ivid Suvarna <[email protected]> wrote:
>
> On Tue, Dec 3, 2019 at 9:46 PM Marc Zyngier <[email protected]> wrote:
> >
> > + James, who wrote most (if not all) of the arm64 hibernate code
> >
> > On 2019-12-03 02:23, Yao HongBo wrote:
> > > On 12/2/2019 9:22 PM, Marc Zyngier wrote:
> > >> Hi Yaohongbo,
> > >>
> > >> In the future, please refrain from sending HTML emails, they
> > >> don't render very well and force me to reformat your email
> > >> by hand.
> > >
> > > Sorry. I'll pay attention to this next time.
> > >
> > >> On 2019-12-02 12:52, yaohongbo wrote:
> > >>> Hi, marc.
> > >>>
> > >>> I met a problem with GIC ITS when I try to power off gic logic in
> > >>> suspend.
> > >>>
> > >>> In hisilicon hip08, the value of GIC_TYPER.HCC is zero, so that
> > >>> ITS_FLAGS_SAVE_SUSPEND_STATE will have no chance to be set to 1.
> > >>
> > >> And that's a good thing. HCC indicates that you have collections
> > >> that
> > >> are backed by registers, and not memory. Which means that once the
> > >> GIC
> > >> is powered off, the state is lost.
> > >>
> > >>> It goes well for s4, when I simply remove the condition judgement
> > >>> in
> > >>> the code.
> > >>
> > >> What is "s4"? Doing so means you are reprogramming the ITS with
> > >> mappings
> > >> that already exist in the tables, and that is UNPRED territory.
> > >
> > > Sorry, I didn't describe it clearly.
> > > S4 means "suspend to disk".
> > > In s4, The its will reinit and malloc an new its address.
> >
> > Huh, hibernate... Yeah, this is not expected to work, I'm afraid.
> >
> > > My expectation is to reprogram the ITS with original mappings. If
> > > ITS_FLAGS_SAVE_SUSPEND_STATE
> > > is not set, i'll have no chance to use the original its table
> > > mappings.
> > >
> > > What should i do if i want to restore its state with hcc == 0?
> >
> > HCC is the least of the problems, and there are plenty more issues:
> >
> > - I'm not sure what guarantees that the tables are at the same
> > address in the booting kernel and the the resumed kernel.
> > That covers all the ITS tables and as well as the RDs'.
> >
> > - It could well be that restoring the ITS base addresses is enough
> > for everything to resume correctly, but this needs some serious
> > investigation. Worse case, we will need to replay the whole of
> > the ITS programming.
> >
> > - This is going to interact more or less badly with the normal suspend
> > to RAM code...
> >
> > - The ITS is only the tip of the iceberg. The whole of the SMMU setup
> > needs to be replayed, or devices won't resume correctly (I just tried
> > on a D05).
> >
> > Anyway, with the hack below, I've been able to get D05 to resume
> > up to the point where devices try to do DMA, and then it was dead.
> > There is definitely some work to be done there...
> >
> > M.
> >
> > diff --git a/drivers/irqchip/irq-gic-v3-its.c
> > b/drivers/irqchip/irq-gic-v3-its.c
> > index 4ba31de4a875..a05fc6bac203 100644
> > --- a/drivers/irqchip/irq-gic-v3-its.c
> > +++ b/drivers/irqchip/irq-gic-v3-its.c
> > @@ -27,6 +27,7 @@
> > #include <linux/of_platform.h>
> > #include <linux/percpu.h>
> > #include <linux/slab.h>
> > +#include <linux/suspend.h>
> > #include <linux/syscore_ops.h>
> >
> > #include <linux/irqchip.h>
> > @@ -42,6 +43,7 @@
> > #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 ITS_FLAGS_SAVE_HIBERNATE_STATE (1ULL << 4)
> >
> > #define RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING (1 << 0)
> > #define RDIST_FLAGS_RD_TABLES_PREALLOCATED (1 << 1)
> > @@ -3517,8 +3519,16 @@ static int its_save_disable(void)
> > raw_spin_lock(&its_lock);
> > list_for_each_entry(its, &its_nodes, entry) {
> > void __iomem *base;
> > + u64 flags;
> >
> > - if (!(its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE))
> > + if (system_entering_hibernation())
> > + its->flags |= ITS_FLAGS_SAVE_HIBERNATE_STATE;
> > +
> > + flags = its->flags;
> > + flags &= (ITS_FLAGS_SAVE_SUSPEND_STATE |
> > + ITS_FLAGS_SAVE_HIBERNATE_STATE);
> > +
> > + if (!flags)
> > continue;
> >
> > base = its->base;
> > @@ -3559,11 +3569,17 @@ static void its_restore_enable(void)
> > raw_spin_lock(&its_lock);
> > list_for_each_entry(its, &its_nodes, entry) {
> > void __iomem *base;
> > + u64 flags;
> > int i;
> >
> > - if (!(its->flags & ITS_FLAGS_SAVE_SUSPEND_STATE))
> > + flags = its->flags;
> > + flags &= (ITS_FLAGS_SAVE_SUSPEND_STATE |
> > + ITS_FLAGS_SAVE_HIBERNATE_STATE);
> > + if (!flags)
> > continue;
> >
> > + its->flags &= ~ITS_FLAGS_SAVE_HIBERNATE_STATE;
> > +
> > base = its->base;
> >
> > /*
>
> How about this one to reinit GIC for restore:
> - https://source.codeaurora.org/quic/la/kernel/msm-4.14/commit/drivers/irqchip/irq-gic-v3.c?h=msm-4.14&id=b0079fb73c08e195498ba2b2ea9623b0cc0f5fed
That's not what we're concerned with at the moment, as there is much
more state that is currently missing. Save/restoring registers is the
easy part. What needs to be fixed is the way RD memory tables
potentially get moved around (and how they can then survive a kexec).
Once we've solved these issues, we'll look at the register state which
is likely to already be correct anyway.
M.
--
Jazz is not dead, it just smells funny.