Re: Bug report: kernel paniced when system hibernates

[Conor Dooley]

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@xxxxxxxxxxxxxxxx> wrote:
> > On Thu, May 18, 2023 at 5:39 PM Alexandre Ghiti <alex@xxxxxxxx> wrote:
> > > On 5/18/23 08:53, Anup Patel wrote:
> > > > On Wed, May 17, 2023 at 8:26 PM Alexandre Ghiti <alexghiti@xxxxxxxxxxxx> wrote:
> > > >> On Wed, May 17, 2023 at 1:28 PM Conor Dooley <conor.dooley@xxxxxxxxxxxxx> 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 :)

> 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.

Attachment: signature.asc
Description: PGP signature