Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <CAK9=C2X1BjZCHfYM33pZQtavu7yRqxwsypWL5OWj79bJrnDMQg@mail.gmail.com>
 <CAOnJCULpa-TJuG=TtCDOxOdUviZzWheLE-GMiU1r7GWaKn0nuQ@mail.gmail.com>
 <20230525-guacamole-swimmer-68048a73baac@wendy> <CAK9=C2WUyLxZwQO37cN-i+V+A3yxmEoaj=uE8yR8nseYTDW7oQ@mail.gmail.com>
 <20230525-postnasal-monopoly-98adb96ffaa1@wendy> <CAAhSdy06nQh4H1FP_K_-VF462mhj+F2M=4AV4QSCUGe5XVqX0g@mail.gmail.com>
 <20230525-shrapnel-precut-26500fca4a48@wendy> <CAAhSdy3SqeLdAfaojUki=ht21nr4ZUPMkW_t9M6ntQCt6Ds4Nw@mail.gmail.com>
 <20230525-citric-waged-a2f78d27eb0c@wendy> <CAOnJCULfC0jmiucLNMeJZwJf4QbGAN6r4B-ubUbP16KVpxrCfA@mail.gmail.com>
 <20230525-flaring-trading-f2bf0713ae26@spud>
In-Reply-To: <20230525-flaring-trading-f2bf0713ae26@spud>
From:   Atish Patra <atishp@atishpatra.org>
Date:   Thu, 25 May 2023 11:37:40 -0700
Message-ID: <CAOnJCUK_EgX-En1QNS8yX1WA1nj8w2kpvXMQcvgWuR3dvzEQYw@mail.gmail.com>
Subject: Re: Bug report: kernel paniced when system hibernates
To:     Conor Dooley <conor@kernel.org>
Cc:     Conor Dooley <conor.dooley@microchip.com>,
        Anup Patel <anup@brainfault.org>,
        Anup Patel <apatel@ventanamicro.com>,
        Alexandre Ghiti <alex@ghiti.fr>, robh@kernel.org,
        Alexandre Ghiti <alexghiti@rivosinc.com>,
        jeeheng.sia@starfivetech.com, linux-kernel@vger.kernel.org,
        palmer@rivosinc.com, leyfoon.tan@starfivetech.com,
        mason.huo@starfivetech.com, Guo Ren <guoren@kernel.org>,
        Paul Walmsley <paul.walmsley@sifive.com>,
        Song Shuai <suagrfillet@gmail.com>,
        linux-riscv@lists.infradead.org,
        Andrew Jones <ajones@ventanamicro.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, May 25, 2023 at 11:22=E2=80=AFAM Conor Dooley <conor@kernel.org> wr=
ote:
>
> Hey Atish,
>
> On Thu, May 25, 2023 at 10:39:44AM -0700, Atish Patra wrote:
> > > How about the below?
>
> > Instead of disabling hibernate support why not revert the patch
> > 3335068 ("riscv: Use PUD/P4D/PGD pages for the linear mapping")
> > which doesn't add any "measured" value at this point.
> > However, keeping the hibernation feature on and disabling linear
> > mapping will get more testing on hibernation.
> > While disabling hibernation and keeping the above patch which
> > doesn't have any value at all.
> >
> > We don't have a regression at this point. So either approach will work =
though.
>
> I favoured this approach so that we do not release a kernel in which
> hibernate works for these versions of OpenSBI and then stops working in
> the future when we shore up how communicating this is supposed to work.
> It allows us to fix the problem "properly" in slow-time, instead of
> racing against v6.4's release.
>

Fair enough.

> I happened to be talking to Palmer and he suggested making it depend on
> NONPORTABLE:
> |> config NONPORTABLE
> |>      bool "Allow configurations that result in non-portable kernels"
> |>      help
> |>        RISC-V kernel binaries are compatible between all known systems
> |>        whenever possible, but there are some use cases that can only b=
e
> |>        satisfied by configurations that result in kernel binaries that=
 are
> |>        not portable between systems.
> |>
> |>        Selecting N does not guarantee kernels will be portable to all =
known
> |>        systems.  Selecting any of the options guarded by NONPORTABLE w=
ill
> |>        result in kernel binaries that are unlikely to be portable betw=
een
> |>        systems.
> |>
> |>        If unsure, say N.
>
> I actually think that that makes more sense, as it may actually be fine
> to use hibernation depending on what your SBI implementation does.
>

That works too.

> > If we choose to go this route, some thoughts about the commit message.
> > > -- >8 --
> > > From 1d4381290a1600eff9b29b8ace6be73955d9726c Mon Sep 17 00:00:00 200=
1
> > > From: Conor Dooley <conor.dooley@microchip.com>
> > > Date: Thu, 25 May 2023 15:09:08 +0100
> > > Subject: [PATCH] RISC-V: mark hibernation as broken
> > >
> > > Hibernation support depends on firmware marking its reserved
> > > regions as not mappable by Linux. As things stand, the de-facto SBI
> >
> > either not mappable or no save/restore capable (as We still have not
> > concluded which way we want to go in)
>
> s/mappable/accessible/? Sounds like a good catch all?
>

Yeah.

> >
> > > implementation (OpenSBI) does not do this, and other implementations =
may
> > > not do so either, resulting in kernel panics during hibernation ([1],
> > > [2]).
> > >
> >
> > we should probably add more context in the commit message.
> > How about adding something along these lines:
> >
> > As things stand, the latest version of de-facto SBI
> > implementation(OpenSBI) doesn't
> > do this any more to allow 1G huge page mappings by kernel. Other SBI
> > implementations are probably
> > doing the same. Until the commit 3335068 ("riscv: Use PUD/P4D/PGD
> > pages for the linear mapping"),
> > the first 2MB region of DRAM (where the typically firmware resides)
> > was not mappable by kernel. However,
> > enabling that mapping resulted in the kernel panics during hibernation
> > ([1], [2]) as the hibernation process
> > tries to save/restore any mapped region even though it is marked as res=
erved.
>
> SGTM, I could go with that.
>
> > > Disable support for hibernation until such time that an SBI
> > > implementation independent way to communicate what regions are reserv=
ed
> > > has been agreed upon.
> > >
> >
> > Anybody who wants to test the hibernation feature must revert the
> > above mentioned patch along with turning on
> > the config.
>
> This goes away with the use of non-portable, although I would work
> mention of the config option into the commit message.
>

Any testing of hibernation still needs to revert the patch until we
have the proper fix.

> Thanks,
> Conor.
>
> > > Reported-by: Song Shuai <suagrfillet@gmail.com>
> > > Link: https://lore.kernel.org/all/CAAYs2=3DgQvkhTeioMmqRDVGjdtNF_vhB+=
vm_1dHJxPNi75YDQ_Q@mail.gmail.com/ [1]
> > > Reported-by: JeeHeng Sia <jeeheng.sia@starfivetech.com>
> > > Link: https://groups.google.com/a/groups.riscv.org/g/sw-dev/c/ITXwaKf=
A6z8
> > > Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> > > ---
> > >  arch/riscv/Kconfig | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> > > index 13f058490608..b2495192f35a 100644
> > > --- a/arch/riscv/Kconfig
> > > +++ b/arch/riscv/Kconfig
> > > @@ -801,7 +801,7 @@ menu "Power management options"
> > >  source "kernel/power/Kconfig"
> > >
> > >  config ARCH_HIBERNATION_POSSIBLE
> > > -       def_bool y
> > > +       def_bool n
> > >
> > >  config ARCH_HIBERNATION_HEADER
> > >         def_bool HIBERNATION
>


--=20
Regards,
Atish