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