Message-ID: <CAPnjgZ28+eT4ULUPnxcthJef_bi=bfFT93YJYQBT2q65Qm7qGQ@mail.gmail.com>
Date:   Thu, 7 Sep 2023 15:39:25 -0600
From:   Simon Glass <sjg@...omium.org>
To:     Ard Biesheuvel <ardb@...nel.org>
Cc:     Rob Herring <robh@...nel.org>,
        Devicetree Discuss <devicetree@...r.kernel.org>,
        Maximilian Brune <maximilian.brune@...ements.com>,
        ron minnich <rminnich@...il.com>,
        Tom Rini <trini@...sulko.com>,
        Dhaval Sharma <dhaval@...osinc.com>,
        U-Boot Mailing List <u-boot@...ts.denx.de>,
        Mark Rutland <mark.rutland@....com>,
        Yunhui Cui <cuiyunhui@...edance.com>,
        linux-acpi@...r.kernel.org, Gua Guo <gua.guo@...el.com>,
        Lean Sheng Tan <sheng.tan@...ements.com>,
        Guo Dong <guo.dong@...el.com>,
        lkml <linux-kernel@...r.kernel.org>,
        Chiu Chasel <chasel.chiu@...el.com>
Subject: Re: [PATCH v5 3/4] schemas: Add some common reserved-memory usages

Hi Ard,

On Thu, 7 Sept 2023 at 10:19, Ard Biesheuvel <ardb@...nel.org> wrote:
>
> On Thu, 7 Sept 2023 at 17:57, Simon Glass <sjg@...omium.org> wrote:
> >
> > Hi Ard,
> >
> > On Thu, 7 Sept 2023 at 09:07, Ard Biesheuvel <ardb@...nel.org> wrote:
> > >
> > > On Thu, 7 Sept 2023 at 16:50, Simon Glass <sjg@...omium.org> wrote:
> > > >
> > > > Hi Ard,
> > > >
> > > > On Thu, 7 Sept 2023 at 08:12, Ard Biesheuvel <ardb@...nel.org> wrote:
> > > > >
> > > > > On Thu, 7 Sept 2023 at 15:56, Simon Glass <sjg@...omium.org> wrote:
> > > > > >
> > > > > > Hi Ard,
> > > > > >
> > > > > > On Thu, 7 Sept 2023 at 07:31, Ard Biesheuvel <ardb@...nel.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.

Nevertheless, that is the goal.

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

Please don't widen the discussion as we are having enough trouble as
it is. Let's focus on the memory reservations.

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

That looks like Tianocore internals rather than a binding, so far as I
can tell. I do need a binding.

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

OK I'll try that patch again with compatible strings instead of node names[1]

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

OK I'll try again.

Regards,
Simon

[1] I was led down the node-name path by this text in the DT spec
"Following the generic-names recommended practice, node names should
reflect the purpose of the node (ie. “framebuffer” or “dma-pool”).
Unit address (@<address>) should be appended to the name if the node
is a static allocation."

Powered by blists - more mailing lists