Re: [PATCH v5 3/4] schemas: Add some common reserved-memory usages

[Ard Biesheuvel <ardb@kernel.org>]

On Thu, 7 Sept 2023 at 17:57, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Ard,
>
> On Thu, 7 Sept 2023 at 09:07, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Thu, 7 Sept 2023 at 16:50, Simon Glass <sjg@chromium.org> wrote:
> > >
> > > Hi Ard,
> > >
> > > On Thu, 7 Sept 2023 at 08:12, Ard Biesheuvel <ardb@kernel.org> wrote:
> > > >
> > > > On Thu, 7 Sept 2023 at 15:56, Simon Glass <sjg@chromium.org> wrote:
> > > > >
> > > > > Hi Ard,
> > > > >
> > > > > On Thu, 7 Sept 2023 at 07:31, Ard Biesheuvel <ardb@kernel.org> wrote:
> > > > > >
...
> > > > > >
> > > > > > I'm happy to help flesh this out, but you still have not provided us
> > > > > > with an actual use case, so I can only draw from my own experience
> > > > > > putting together firmware for virtual and physical ARM machines.
> > > > >
> > > > > I did explain that this is needed when Tianocore is on both sides of
> > > > > the interface, since Platform Init places some things in memory and
> > > > > the Payload needs to preserve them there, and/or know where they are.
> > > > >
> > > > > I think the problem might be that you don't agree with that, but it
> > > > > seems to be a fact, so I am not sure how I can alter it.
> > > > >
> > > > > Please can you clearly explain which part of the use case you are missing.
> > > > >
> > > >
> > > > 'Tianocore on both sides of the interface' means that Tianocore runs
> > > > as the platform init code, and uses a bespoke DT based protocol to
> > > > launch another instance of Tianocore as the payload, right?
> > >
> > > Not another instance, no. Just the other half of Tianocore. The first
> > > half does platform init and the second half does the loading of the
> > > OS.
> > >
> >
> > That doesn't make any sense to me.
> >
> > > >
> > > > Tianocore/EDK2 already implements methods to reinvoke itself if needed
> > > > (e.g., during a firmware update), and does so by launching a new DXE
> > > > core. So the boot sequence looks like
> > > >
> > > > SEC -> PEI -> DXE -> BDS -> app that invokes UpdateCapsule() -> DXE ->
> > > > firmware update
> > > >
> > > > So please elaborate on how this Tianocore on both sides of the
> > > > interface is put together when it uses this DT based handover. We
> > > > really need a better understanding of this in order to design a DT
> > > > binding that meets its needs.
> > >
> > > Are you familiar with building Tianocore as a coreboot payload, for
> > > example? That shows Tianocore running as just the Payload, with
> > > coreboot doing the platform init. So the use case I am talking about
> > > is similar to that.
> > >
> >
> > Yes I am familiar with that, and it is a completely different thing.
>
> Right, but that is my use case.
>

OK. You alluded to Tianocore <-> Tianocore being your use case, which
is why I kept asking for clarification, as using a DT with this
binding seems unusual at the very least.

So coreboot does the platform init, and then hands over to Tianocore.

I take it we are not talking about x86 here, so there are no Intel FSP
blobs that may have dependencies on Tianocore/EDK2 pieces, right? So
there are no UEFI semantics in the memory descriptions that coreboot
provides to Tianocore.

So coreboot provides information to TIanocore about
- the platform topology (DT as usual)
- DRAM memory banks
- memory reservations
- secure firmware services perhaps?
- anything else?


> >
> > As i explained before, there is already prior art for this in
> > Tianocore, i.e., launching a Tianocore build based on a DT description
> > of the platform, including /memory and /reserved-memory nodes.
>
> By prior art do you mean code, or an existing binding? In either case,
> can you please point me to it? Is this a generic binding used on x86
> as well, or just for ARM?
>
> My goal here is to augment the binding.
>

No I mean code.

There is

https://github.com/tianocore/edk2/tree/master/EmbeddedPkg/Drivers/FdtClientDxe

which encapsulates the DT received from the previous boot stage, and
exposes it as a DXE protocol.

There are other drivers that depend on this protocol, e.g., to
discover additional memory nodes, virtio-mmio drivers and PCI host
bridges.

https://github.com/tianocore/edk2/tree/master/OvmfPkg/Fdt

The bindings used are the ones documented in the Linux kernel tree -
no ad-hoc bindings are being used as far as I know.

But the point I was making before re prior art was really about using
existing bindings rather than inventing new ones. Since we are now
talking about augmenting /reserved-memory, I think we're already on
the same page in this regard (with the caveat that the EDK2 code does
not actually honour /reserved-memory at this point, but this is
because none of the platforms it is being used on today uses that
node)

> >
> > I argued that Tianocore never consumes memory reservations with UEFI
> > semantics, given that it supplants whatever UEFI functionality the
> > previous stage may have provided. But it shouldn't step on the code
> > and data regions used by the previous stage if it is still running in
> > the background (e.g., OS at EL1 and PSCI at EL2 on ARM)
> >
> > So this brings me back to the things I proposed in my previous reply:
> > - memory reservations should be described in detail so the consumer
> > knows what to do with it
>
> Yes I can add more detail, if that is all that is needed. But we seem
> to still not be aligned on the goal.
>
>

I do think we're converging, actually - it is just taking me some time
to get a clear mental picture of how this will be used.

> > - memory reservations should have attributes that describe how the
> > memory may be used if not for the described purpose
> >
> > I still don't see a reason for things like runtime-code and
> > runtime-data etc based on the above. If stage N describes the memory
> > it occupies itself as system memory, it should reserve it as well if
> > it needs to be preserved after stage N+1 has taken over, so perhaps it
> > should be described as a discardable memory reservation but I don't
> > think it necessarily needs a type in that case.
>
> Well if you can find another way to do this in the DT, that is fine.
>

It will all be under /reserved-memory, as far as I understand. We just
need to get to the right level of abstraction.