Re: ITS restore/save state when HCC == 0

[Marc Zyngier]

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