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

["Laszlo Ersek" <lersek@redhat.com>]

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.

Thanks,
Laszlo