linux-kernel - Re: ITS restore/save state when HCC == 0

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Wed, 4 Dec 2019 11:43:23 +0530
From:   Ivid Suvarna <ivid.suvarna@...il.com>
To:     Marc Zyngier <maz@...nel.org>
Cc:     Yao HongBo <yaohongbo@...wei.com>,
        "Guohanjun (Hanjun Guo)" <guohanjun@...wei.com>,
        Yangyingliang <yangyingliang@...wei.com>,
        Linuxarm <linuxarm@...wei.com>,
        Kernel development list <linux-kernel@...r.kernel.org>,
        james.morse@....com
Subject: Re: ITS restore/save state when HCC == 0

On Tue, Dec 3, 2019 at 9:46 PM Marc Zyngier <maz@...nel.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