Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Wed, 04 Dec 2019 08:03:33 +0000
Message-ID: <86lfrszfe2.wl-maz@kernel.org>
From:   Marc Zyngier <maz@kernel.org>
To:     Ivid Suvarna <ivid.suvarna@gmail.com>
Cc:     Yao HongBo <yaohongbo@huawei.com>,
        "Guohanjun (Hanjun Guo)" <guohanjun@huawei.com>,
        Yangyingliang <yangyingliang@huawei.com>,
        Linuxarm <linuxarm@huawei.com>,
        Kernel development list <linux-kernel@vger.kernel.org>,
        james.morse@arm.com
Subject: Re: ITS restore/save state when HCC == 0
In-Reply-To: <CABXF_AA+93p+1yVeDwmeMG3cn_2vUbvycN7TeV=8cDJ6bd8Leg@mail.gmail.com>
References: <fd89d78030914d19939a1fc1c6eb5048@huawei.com>
        <e04e35e0a14f1507ac4a3d56899adcae@www.loen.fr>
        <c8649d75-a9b8-4680-c253-3172774ac33d@huawei.com>
        <c03d0b67a814288402b90bcdba600d26@www.loen.fr>
        <CABXF_AA+93p+1yVeDwmeMG3cn_2vUbvycN7TeV=8cDJ6bd8Leg@mail.gmail.com>
User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue)
 FLIM/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL/10.8 EasyPG/1.0.0 Emacs/26
 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO)
MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue")
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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.