Message-ID: <CAK9=C2WUyLxZwQO37cN-i+V+A3yxmEoaj=uE8yR8nseYTDW7oQ@mail.gmail.com>
Date:   Thu, 25 May 2023 18:51:28 +0530
From:   Anup Patel <apatel@...tanamicro.com>
To:     Conor Dooley <conor.dooley@...rochip.com>
Cc:     Atish Patra <atishp@...shpatra.org>,
        Alexandre Ghiti <alex@...ti.fr>, robh@...nel.org,
        Alexandre Ghiti <alexghiti@...osinc.com>,
        jeeheng.sia@...rfivetech.com, Anup Patel <anup@...infault.org>,
        linux-kernel@...r.kernel.org, palmer@...osinc.com,
        leyfoon.tan@...rfivetech.com, mason.huo@...rfivetech.com,
        Guo Ren <guoren@...nel.org>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Song Shuai <suagrfillet@...il.com>,
        linux-riscv@...ts.infradead.org,
        Andrew Jones <ajones@...tanamicro.com>
Subject: Re: Bug report: kernel paniced when system hibernates

On Thu, May 25, 2023 at 6:39 PM Conor Dooley <conor.dooley@...rochip.com> wrote:
>
> On Wed, May 24, 2023 at 04:45:36PM -0700, Atish Patra wrote:
> > On Thu, May 18, 2023 at 7:04 AM Anup Patel <apatel@...tanamicro.com> wrote:
> > > On Thu, May 18, 2023 at 5:39 PM Alexandre Ghiti <alex@...ti.fr> wrote:
> > > > On 5/18/23 08:53, Anup Patel wrote:
> > > > > On Wed, May 17, 2023 at 8:26 PM Alexandre Ghiti <alexghiti@...osinc.com> wrote:
> > > > >> On Wed, May 17, 2023 at 1:28 PM Conor Dooley <conor.dooley@...rochip.com> wrote:
>
> > > > > I think we have two possible approaches:
> > > > >
> > > > > 1) Update OpenSBI to set "no-map" DT property for firmware
> > > > >      reserved regions. We were doing this previously but removed
> > > > >      it later for performance reasons mentioned by Alex. It is also
> > > > >      worth mentioning that ARM Trusted Firmware also sets "no-map"
> > > > >      DT property for firmware reserved regions.
> > > > >
> > > > > 2) Add a new "no-save-restore" DT property in the reserved
> > > > >      memory DT bindings. The hibernate support of Linux arch/riscv
> > > > >      will use this DT property to exclude memory regions from
> > > > >      save-restore. The EFI implementation of EDK2 and U-Boot
> > > > >      should do the following:
> > > > >      1) Treat all memory having "no-map" DT property as EFI
> > > > >          reserved memory
> > > > >      2) Treat all memory not having "no-map" DT property and
> > > > >          not having "no-save-restore" DT property as EfiBootServicesData
> > > > >      3) Treat all memory not having "no-map" DT property and
> > > > >           having "no-save-restore" DT property as EfiRuntimeServiceData
> > > > >           (Refer,
> > > > > https://devicetree-specification.readthedocs.io/en/latest/chapter3-devicenodes.html#reserved-memory-and-uefi)
> > > > >
> > > > > Personally, I am leaning towards approach#1 since approach#2
> > > > > will require changing DeviceTree specification as well.
> > > >
> > > >
> > > > If needed, indeed #1 is the simplest, but I insist, to me it is not
> > > > needed (and we don't have it in the current opensbi), if you have
> > > > another opinion, I'm open to discuss it!
> > >
> >
> > The problem with relying on the "mmode_resv" name is that there will be
> > other use cases where a portion of the memory must be reserved and accessing
> > that from the kernel will result in fault. CoVE is such a use case where
> > TSM will probably run from a memory region with confidential memory
> > which the kernel must not access.
>
> We should only rely on this node name for known bad versions of opensbi
> IMO. Going forward, if something needs to be reserved for firmware, the
> firmware should make sure that it is reserved by using the property for
> that purpose :)

There is no issue with OpenSBI since it does the right thing by marking
memory as reserved in the DT. This real issue is with the kernel handling
of reserved memory for hibernate.

Like Atish mentioned, not just OpenSBI, there will be other entities
(like TSM) or some other M-mode firmware which will also reserve
memory in DT/ACPI so clearly kernel needs a SBI implementation
independent way of handling reserved memory for hibernate.

Regards,
Anup

>
> > We have to name it "mmode_resv" as well or mark it as "no-map" which will
> > present a hole in mappings. We will end up in same 1GB hugepage issue
> > if we choose "no-map".
> >
> > Another option is to use compatible string or label property to indicate
> > that this memory region is not to be saved/restored during hibernation.
> > This can be documented in RISC-V DT bindings as well as the booting guide
> > doc that alex was talking about.
>
> Sure, a dt-binding for sbi reserved regions doesn't immediately sound
> like an awful idea... But we still have to work around the borked
> firmware - be that disabling hibernation or using the mmode_resv node
> when we know that the version of OpenSBI is one of those with the
> problem.
>
> > > I agree with you, backward compatibility with older firmwares
> > > is important.
> > >
> > This does break the compatibility with the older firmware w.r.to hibernation
> > feature. However
> >
> > It is specific to hibernation only. So hibernation will fail to work
> > if an user is running kernel > 6.4 but 0.8 < OpenSBI < 1.2
> >
> > The same problem lies if users use other firmware that don't have
> > no-map property today. IMO, this can be documented as a known problem.
>
> I'd rather we disabled it than documented it as broken.
> Or disable _and_ document it.
>
> > > Let's go with your proposed change to treat reserved DT nodes
> > > with "mmode_resv*" name as M-mode firmware memory (it could
> > > be any M-mode firmware). We will certainly need to document it
> > > somewhere as an expectation of Linux RISC-V kernel.
> > >
> > > @Sunil How about treating "mmode_resv*" as
> > > EfiRuntimeServiceData in EDK2 ? Other reserved memory
> > > nodes can follow the device tree specification.
> > >
> >
> > Either way, we also need to fix U-Boot to match the behavior.  Currently,
> > it treats any reserved memory without no-map property as EFI_BOOT_SERVICES_DATA.
>
> Cheers,
> Conor.

Powered by blists - more mailing lists