* Re: ITS restore/save state when HCC == 0 [not found] <fd89d78030914d19939a1fc1c6eb5048@huawei.com> @ 2019-12-02 13:22 ` Marc Zyngier 2019-12-03 2:23 ` Yao HongBo 0 siblings, 1 reply; 5+ messages in thread From: Marc Zyngier @ 2019-12-02 13:22 UTC (permalink / raw) To: yaohongbo; +Cc: Guohanjun (Hanjun Guo), Yangyingliang, Linuxarm, linux-kernel 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... ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: ITS restore/save state when HCC == 0 2019-12-02 13:22 ` ITS restore/save state when HCC == 0 Marc Zyngier @ 2019-12-03 2:23 ` Yao HongBo 2019-12-03 15:58 ` Marc Zyngier 0 siblings, 1 reply; 5+ messages in thread From: Yao HongBo @ 2019-12-03 2:23 UTC (permalink / raw) To: Marc Zyngier Cc: Guohanjun (Hanjun Guo), Yangyingliang, Linuxarm, linux-kernel 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: ITS restore/save state when HCC == 0 2019-12-03 2:23 ` Yao HongBo @ 2019-12-03 15:58 ` Marc Zyngier 2019-12-04 6:13 ` Ivid Suvarna 0 siblings, 1 reply; 5+ messages in thread From: Marc Zyngier @ 2019-12-03 15:58 UTC (permalink / raw) To: Yao HongBo Cc: Guohanjun (Hanjun Guo), Yangyingliang, Linuxarm, linux-kernel, james.morse + 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... ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: ITS restore/save state when HCC == 0 2019-12-03 15:58 ` Marc Zyngier @ 2019-12-04 6:13 ` Ivid Suvarna 2019-12-04 8:03 ` Marc Zyngier 0 siblings, 1 reply; 5+ messages in thread From: Ivid Suvarna @ 2019-12-04 6:13 UTC (permalink / raw) To: Marc Zyngier Cc: Yao HongBo, Guohanjun (Hanjun Guo), Yangyingliang, Linuxarm, Kernel development list, james.morse On Tue, Dec 3, 2019 at 9:46 PM Marc Zyngier <maz@kernel.org> 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: ITS restore/save state when HCC == 0 2019-12-04 6:13 ` Ivid Suvarna @ 2019-12-04 8:03 ` Marc Zyngier 0 siblings, 0 replies; 5+ messages in thread From: Marc Zyngier @ 2019-12-04 8:03 UTC (permalink / raw) To: Ivid Suvarna Cc: Yao HongBo, Guohanjun (Hanjun Guo), Yangyingliang, Linuxarm, Kernel development list, james.morse On Wed, 04 Dec 2019 06:13:23 +0000, Ivid Suvarna <ivid.suvarna@gmail.com> wrote: > > On Tue, Dec 3, 2019 at 9:46 PM Marc Zyngier <maz@kernel.org> 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-12-04 8:03 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <fd89d78030914d19939a1fc1c6eb5048@huawei.com>
2019-12-02 13:22 ` ITS restore/save state when HCC == 0 Marc Zyngier
2019-12-03 2:23 ` Yao HongBo
2019-12-03 15:58 ` Marc Zyngier
2019-12-04 6:13 ` Ivid Suvarna
2019-12-04 8:03 ` Marc Zyngier
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox