Re: [edk2-devel] [PATCH 3/4] ArmVirtPkg/PlatformPeiLib: implement Reset2 PPI based on PSCI

["Ard Biesheuvel" <ard.biesheuvel@linaro.org>]

On Tue, 7 Jan 2020 at 19:47, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 01/07/20 17:55, Ard Biesheuvel wrote:
> > On Tue, 7 Jan 2020 at 17:50, Laszlo Ersek <lersek@redhat.com> wrote:
> >>
> >> On 01/07/20 10:47, Ard Biesheuvel wrote:
> >>> Extend the existing DT traversal routine in PlatformPeiLib with
> >>> discovery of the PSCI method, and expose an implementation of the
> >>> Reset2 PPI based on the method found.
> >>>
> >>> This satisfies a dependency of Tcg2Pei, which needs to reset the
> >>> platform in some cases. Since there are no other uses for system
> >>> reset in PEI on ArmVirtQemu, simply expose the PPI directly rather
> >>> than using the generic ResetSystemPei and the associated plumbing.
> >>>
> >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >>> ---
> >>>  ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.inf |   3 +
> >>>  ArmVirtPkg/Library/PlatformPeiLib/PlatformPeiLib.c   | 123 ++++++++++++++++++++
> >>>  2 files changed, 126 insertions(+)
> >>
> >> Tcg2Pei uses ResetCold() from ResetSystemLib.
> >>
> >> ArmVirtPkg's existent ResetSystemLib instance
> >> (ArmVirtPsciResetSystemLib/ArmVirtPsciResetSystemLib.inf) is only
> >> suitable for DXE_DRIVER and DXE_RUNTIME_DRIVER instances. It uses our
> >> FDT Client protocol for looking up (I think) more or less the same
> >> things that you parse here.
> >>
> >> As a PEI phase replacement, this patch produces gEfiPeiReset2PpiGuid,
> >> and then in patch#4, we resolve ResetSystemLib, for PEIMs, to the
> >>
> >>   MdeModulePkg/Library/PeiResetSystemLib/PeiResetSystemLib.inf
> >>
> >> instance, which depends on the PPI installed here.
> >>
> >> I'm not too happy about installing the gEfiPeiReset2PpiGuid from
> >> PlatformPeiLib.
> >>
> >> As replacement, it's not ResetSystemPei what I'd recommend (which
> >> depends on a PEI-phase ResetSystemLib instance anyway, which we don't
> >> have, in the first place).
> >>
> >> (1) Instead, I'd recommend implementing a PEI-phase ResetSystemLib for
> >> ArmVirtQemu. (Under ArmVirtPkg/Library/ArmVirtPsciResetSystemLib -- new
> >> INF file.)
> >>
> >> Would that be a large burden? I think we'd need a helper function in
> >> that lib instance, for returning HVC versus SMC (from the FDT), and then
> >> we'd have to call the proper interface for the actual reset. Not fast,
> >> but resets don't need to be fast I think.
> >>
> >
> > That is what I started out with. The problem is that I am not 100%
> > convinced that it is safe to dereference the initial FDT base address
> > at arbitrary times during PEI,
>
> Great point; this is one of those things that I had to think about for
> many minutes before posting my email.
>
> I think it's safe. For two reasons:
>
> (i) all of the PEIMs, and the PEI_CORE, in ArmVirtQemu, use the
>
> ArmVirtPkg/Library/FdtPL011SerialPortLib/EarlyFdtPL011SerialPortLib.inf
>
> instance for writing to the serial port. This library instance re-parses
> the initial DTB at every DEBUG call, in effect, across all the PEIMs.
>
> (See SerialPortWrite() --> SerialPortGetBaseAddress() -->
> PcdGet64(PcdDeviceTreeInitialBaseAddress)).
>
> In other words, we're already doing this, at the moment.
>
> (ii) In
>
> ArmVirtPkg/Library/QemuVirtMemInfoLib/QemuVirtMemInfoPeiLibConstructor.c
>
> we have:
>
>   //
>   // We need to make sure that the machine we are running on has at least
>   // 128 MB of memory configured, and is currently executing this binary from
>   // NOR flash. This prevents a device tree image in DRAM from getting
>   // clobbered when our caller installs permanent PEI RAM, before we have a
>   // chance of marking its location as reserved or copy it to a freshly
>   // allocated block in the permanent PEI RAM in the platform PEIM.
>   //
>   ASSERT (NewSize >= SIZE_128MB);
>   ASSERT (
>     (((UINT64)PcdGet64 (PcdFdBaseAddress) +
>       (UINT64)PcdGet32 (PcdFdSize)) <= NewBase) ||
>     ((UINT64)PcdGet64 (PcdFdBaseAddress) >= (NewBase + NewSize)));
>
>
> To elaborate on this: initially we use the temporary SEC/PEI heap+stack;
> later on we use the permanent PEI RAM.
>
> (ii.1) The temp SEC/PEI heap+stack is set up in
>
>   ArmPlatformPkg/PrePeiCore/MainUniCore.c
>
> and it is based on PcdCPUCoresStackBase. The value of
> PcdCPUCoresStackBase is fixed, in ArmVirtQemu.dsc:
>
>   gArmPlatformTokenSpaceGuid.PcdCPUCoresStackBase|0x4007c000
>
> whereas the initial DTB is at the base of DRAM:
>
>   gArmTokenSpaceGuid.PcdSystemMemoryBase|0x40000000
>   gArmVirtTokenSpaceGuid.PcdDeviceTreeInitialBaseAddress|0x40000000
>
> so if the initial DTB fits in 0x7C000 bytes (496 KiB), we're good, as
> far as the temporary SEC/PEI heap+stack is concerned.
>
> (ii.2) The permanent PEI RAM is 64MB in size:
>
>   # Size of the region used by UEFI in permanent memory (Reserved 64MB)
>   gArmPlatformTokenSpaceGuid.PcdSystemMemoryUefiRegionSize|0x04000000
>
> Because of the *two* ASSERT()s in "QemuVirtMemInfoPeiLibConstructor.c"
> that I quoted above, we know that the lowest DRAM node is at least 128MB
> in size, and also that it does not overlap with the flash device.
> Consequently, the the InitializeMemory() function in
>
>   ArmPlatformPkg/MemoryInitPei/MemoryInitPeim.c
>
> will take the following branch:
>
> ------
>   } else {
>     // Check the Firmware does not overlapped with the system memory
>     ASSERT ((FdBase < SystemMemoryBase) || (FdBase >= SystemMemoryTop));
>     ASSERT ((FdTop <= SystemMemoryBase) || (FdTop > SystemMemoryTop));
>
>     UefiMemoryBase = SystemMemoryTop - FixedPcdGet32 (PcdSystemMemoryUefiRegionSize);
>   }
>
>   Status = PeiServicesInstallPeiMemory (UefiMemoryBase, FixedPcdGet32 (PcdSystemMemoryUefiRegionSize));
> ------
>
> and therefore
>
>   UefiMemoryBase == (PcdSystemMemoryBase + PcdSystemMemorySize) - PcdSystemMemoryUefiRegionSize
>                  == 1GiB + PcdSystemMemorySize - 64MiB
>
> Given that PcdSystemMemorySize >= 128 MiB, we get
>
>   UefiMemoryBase >= 1GiB + 64 MiB
>
> Which means that the permanent PEI RAM too is safely distinct from the
> initial DTB (which is at the base of DRAM: at 1 GiB).
>
> In summary: it's safe, and it's so by design. We did this intentionally.
> Originally in commit a36d531f5d56 ("ArmPlatformPkg/ArmVirtualizationPkg:
> add ArmVirtualizationPlatformLib library", 2014-09-18):
>
> commit a36d531f5d565e6cb5496ea53824e36487a227dd
> Author: Michael Casadevall <michael.casadevall@linaro.org>
> Date:   Thu Sep 18 18:05:03 2014 +0000
>
>     ArmPlatformPkg/ArmVirtualizationPkg: add ArmVirtualizationPlatformLib library
>
>     This is an implementation of ArmPlatformLib that discovers the size of system
>     DRAM from a device tree blob located at the address passed in
>     gArmTokenSpaceGuid.PcdDeviceTreeBaseAddress, which should equal the value in
>     gArmTokenSpaceGuid.PcdSystemMemoryBase.
>
>     As the device tree blob is passed in system DRAM, this library can only be used
>     if sufficient DRAM is available (>= 128 MB) and if not using shadowed NOR. The
>     reason for this is that it makes it easier to guarantee that such a device tree
>     blob at base of DRAM will not be clobbered before we get a chance to preserve it.
>
>     Contributed-under: TianoCore Contribution Agreement 1.0
>     Signed-off-by: Michael Casadevall <michael.casadevall@linaro.org>
>     Acked-by: Laszlo Ersek <lersek@redhat.com>
>     Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
>     Signed-off-By: Olivier Martin <olivier.martin@arm.com>
>
> And then we moved it to its present spot in the series that contains
> commit 048651260b66 ("ArmVirtPkg: create QemuVirtMemInfoLib version for
> ArmVirtQemu", 2017-11-23).
>
> > and so it would be better to consume
> > the FDT from the GUIDed HOB. That, however, creates another ordering
> > issue, and so we should install a PPI that signals the availability of
> > the FDT GUIDed HOB.
> >
> > At this point, I decided it would be better to just produce the PPI in
> > the PlatformPeiLib, but I agree it is not the cleanest approach.
> >
> >> BTW I think the following modules are never meant to be used together:
> >>
> >>   MdeModulePkg/Library/PeiResetSystemLib/PeiResetSystemLib.inf
> >>   MdeModulePkg/Universal/ResetSystemPei/ResetSystemPei.inf
> >>
> >> because they seem to depend mutually on each other's abstract interface
> >> (PPI vs. lib class). So I think a given platform should include *at most
> >> one* of these, on top of the "other" facility that it already exposes.
> >> In ArmVirtQemu's case, I'd suggest implementing the lib class for PEI,
> >> and then we can include "ResetSystemPei" whenever the need arises.
> >>
> >
> > The idea is that other PEIMs use the library, which is backed by the
> > PEIM, so that any hooks and notifications that occur at reset time can
> > be dispatched correctly. If every PEIM that needs to reset the system
> > calls into a library directly, this no longer works.
> >
>
> Good point -- now I realize my exclusivity argument above is wrong. I
> now recall why.
>
> The following is a quite common pattern in edk2:
>
> - there is a lib class offering some service, at the abstract level
> - there is a PEIM or DXE driver that exposes the same service as a PPI
>   or protocol, respectively
> - this PEIM or DXE driver internally calls the lib API
> - there are two lib instances: one instance does the real thing, and the
>   other instance calls out to the PPI or protocol
> - all PEIMs / DXE drivers *except* the one PEIM or DXE driver mentioned
>   above get the "shim" lib instance
> - the one PEIM / DXE driver that exposes the PPI / protocol gets the
>   "real deal" lib instance, via module-level lib class resolution
>   override in the DSC file.
>
> We can do the same here, I think:
>
> - resolve ResetSystemLib, generally for PEIMs, to
>   "MdeModulePkg/Library/PeiResetSystemLib/PeiResetSystemLib.inf",
> - include "MdeModulePkg/Universal/ResetSystemPei/ResetSystemPei.inf" in
>   the firmware binary,
> - resolve ResetSystemLib, specifically for "ResetSystemPei.inf", to our
>   new (FDT-parsing) lib instance.
>
> How does it sound to you? Yes, it's more fluff, but it's clean, and
> native to edk2.
>

Yeah, I'm fine with all of that. Thanks for reminding me why it is
safe to refer to the DT at the base of memory throughout the PEI
phase.