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

[Marc Zyngier]

On Wed, 04 Dec 2019 06:13:23 +0000,
Ivid Suvarna <ivid.suvarna@xxxxxxxxx> wrote:
> 
> On Tue, Dec 3, 2019 at 9:46 PM Marc Zyngier <maz@xxxxxxxxxx> 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.