[RFC] Remove PI spec TemporaryRamSupport PPI from PEI Core

[RFC] Remove PI spec TemporaryRamSupport PPI from PEI Core

[Jordan Justen]

Short summary: Despite being part of the Platorm Initialization
Specificication, this proposal would remove support for the
TemporaryRamSupport PPI from PEI Core. Arguably, this would mean the
PEI Core does not support the PI spec, but we do not currently know if
any platforms require this PPI.

Main question: Does anyone know of a platform that requires this PPI,
or does anyone have major concerns with removing it before the PI
specification can be updated to remove the PPI?

Alternatives: Continue to support the PPI, but we would need to merge
a bug-fix patchset which adds some assembly code into the PEI Core.

More details:

I discussed this topic with the Tianocore Stewards, Vincent and Ray.
Although the path forward was not unanimous, it appears that the
majority thought we should propose to remove support for the
TemporaryRamSupport PPI from the PEI Core.

This PPI is defined in MdePkg/Include/Ppi/TemporaryRamSupport.h. I
believe it has been present in all versions of the Platorm
Initialization Specificication, including the latest, version 1.7.
(https://uefi.org/specsandtesttools)

The TemporaryRamSupport PPI is defined as "optional" in the PI spec,
but I believe this only applied to producers of the PPI (PI
platforms). The PEI Core (which is the consumer of this PPI) should
arguably support the PPI in order to fully support PI spec based
platforms.

But, there are some subtle issues with the PPI that make it difficult
to implement for both the consumer (PEI Core) and the producer (the
platform). In fact, there is a bug currently with the PEI Core's
implementation, which I sent a patch series to address on April 10th,
with the subject of "[PATCH v2 0/6] Fix PEI Core issue during
TemporaryRamMigration".

Unfortunately the fix is not very simple, and requires adding assembly
code to the PEI Core module. Based on this, we discussed if the
TemporaryRamSupport PPI is no longer required by current PI platforms,
despite being present in the PI spec. The only known example of it
being required was a platform based on the IPF architecture, which is
no longer supported by EDK II.

Several EDK II sample platforms produce this PPI in EDK II, but only
as sample code. Clearly the removal would have to take this into
account.

There are two replacements for the TemporaryRamSupport PPI, but they
do not cover all types of potential hardware. If a platform cannot
access main memory at the same time as the Temporary RAM, then it
would not be supported by the alternatives. Nevertheless, the two
replacements are:

1. Stop producing the PPI, and the PEI Core will automatically copy
   the required memory from Temporary RAM to the main memory. This
   will work for platforms that do not require special code to shut
   down Temporary RAM.

2. Produce the MdePkg/Include/Ppi/TemporaryRamDone.h PPI. This will
   cause the PEI Core to notify the platform after it has copied
   memory, and the platform can take special actions to disable
   Temporary RAM mode if required.

Since both of these options will cause the PEI Core to do a memory
copy from Temporary RAM to the main memory, there might be platforms
that they cannot work with as described above.

Thanks for your time,

-Jordan

Re: [RFC] Remove PI spec TemporaryRamSupport PPI from PEI Core

[Laszlo Ersek]

On 04/24/19 09:51, Jordan Justen wrote:
> Short summary: Despite being part of the Platorm Initialization
> Specificication, this proposal would remove support for the
> TemporaryRamSupport PPI from PEI Core. Arguably, this would mean the
> PEI Core does not support the PI spec, but we do not currently know if
> any platforms require this PPI.
> 
> Main question: Does anyone know of a platform that requires this PPI,

You mention edk2 sample platforms below. I presume you include OVMF
among those. To me, OVMF is not a sample (or validation) platform, but
the upstream project for a *product*.

That said, I don't think OVMF "requires" this PPI. IOW, this part of
OVMF's SEC indeed qualifies as (functional) "sample code". I think
Replacement#1 applies (stop producing the PPI and let the PEI Core
fallback do the work).

I'd expect a patch series that removes the PPI definition (and its
consumption by the PEI Core) to update OVMF as well (and all the other
platforms in edk2 that currently produce the PPI).

> or does anyone have major concerns with removing it before the PI
> specification can be updated to remove the PPI?

I do, purely from a process / specs perspective.

It's one thing to design & implement an edk2 extension (one that doesn't
break compatibility with any of the relevant specs), amass evidence that
the implementation works and is robust, then standardize the extension
in PI / UEFI, and then complete the feature in edk2 by code movement /
renaming to standard locations / names. In that scenario, it's safe for
the code to advance first, and the spec(s) second, and I think I've
always encouraged (and requested) this.

With feature removal, I'm afraid we should work in the reverse order.
Minimally, I think you should please post this RFC to the PIWG list, and
file a Mantis ticket for the PI spec. Then we should start working on
the feature removal once there is "buy in" from the PIWG. IMO, we
shouldn't tag a handful of edk2-stableYYYYMM releases with the feature
removed, but the Mantis ticket *stuck* (let alone non-existent).

I'm not suggesting to gate the code removal on a PI spec release, but I
think it's reasonable to require a *clear commitment* from the PIWG, to
the PPI removal, by the time we first tag an edk2-stableYYYYMM release
that no longer handles the PPI. In the release notes for that
edk2-stableYYYYMM release, we should be able to reference a TianoCore BZ
that gives the rationale, and in turn references an in-progress
(committed-to) Mantis ticket.

... My experience with the specs suggests that they move very slowly, so
even the above "clear commitment" approach (= *non-stuck* Mantis ticket)
would likely result in multiple edk2-stableYYYYMM releases that don't
conform to the latest available PI spec. While I don't consider that a
"deal breaker" (as long as the Mantis ticket is moving, see above), it's
also not optimal. Applying your v2 series now, then shepherding the
Mantis ticket to completion / spec release, *then* removing the PPI from
edk2 altogether (including your v2 series) looks safest & cleanest, for
edk2-stableYYYYMM releases.

Summary of my opinion:

- PIWG buy-in must exist and be documented on a Mantis ticket (and on a
  TianoCore bugzilla) before we tag an edk2-stableYYYYMM release with
  the PPI removed from edk2,

- the series that removes the PPI should update all platforms in edk2
  and preferably edk2-platforms too that currently produce the PPI,

- merging the v2 series first (with clearly marked ownership /
  maintenance) would be nice (but not a hard req), in order to keep
  compat with PI until PI is actually updated.

My two cents...

Thanks,
Laszlo

> Alternatives: Continue to support the PPI, but we would need to merge
> a bug-fix patchset which adds some assembly code into the PEI Core.
> 
> More details:
> 
> I discussed this topic with the Tianocore Stewards, Vincent and Ray.
> Although the path forward was not unanimous, it appears that the
> majority thought we should propose to remove support for the
> TemporaryRamSupport PPI from the PEI Core.
> 
> This PPI is defined in MdePkg/Include/Ppi/TemporaryRamSupport.h. I
> believe it has been present in all versions of the Platorm
> Initialization Specificication, including the latest, version 1.7.
> (https://uefi.org/specsandtesttools)
> 
> The TemporaryRamSupport PPI is defined as "optional" in the PI spec,
> but I believe this only applied to producers of the PPI (PI
> platforms). The PEI Core (which is the consumer of this PPI) should
> arguably support the PPI in order to fully support PI spec based
> platforms.
> 
> But, there are some subtle issues with the PPI that make it difficult
> to implement for both the consumer (PEI Core) and the producer (the
> platform). In fact, there is a bug currently with the PEI Core's
> implementation, which I sent a patch series to address on April 10th,
> with the subject of "[PATCH v2 0/6] Fix PEI Core issue during
> TemporaryRamMigration".
> 
> Unfortunately the fix is not very simple, and requires adding assembly
> code to the PEI Core module. Based on this, we discussed if the
> TemporaryRamSupport PPI is no longer required by current PI platforms,
> despite being present in the PI spec. The only known example of it
> being required was a platform based on the IPF architecture, which is
> no longer supported by EDK II.
> 
> Several EDK II sample platforms produce this PPI in EDK II, but only
> as sample code. Clearly the removal would have to take this into
> account.
> 
> There are two replacements for the TemporaryRamSupport PPI, but they
> do not cover all types of potential hardware. If a platform cannot
> access main memory at the same time as the Temporary RAM, then it
> would not be supported by the alternatives. Nevertheless, the two
> replacements are:
> 
> 1. Stop producing the PPI, and the PEI Core will automatically copy
>    the required memory from Temporary RAM to the main memory. This
>    will work for platforms that do not require special code to shut
>    down Temporary RAM.
> 
> 2. Produce the MdePkg/Include/Ppi/TemporaryRamDone.h PPI. This will
>    cause the PEI Core to notify the platform after it has copied
>    memory, and the platform can take special actions to disable
>    Temporary RAM mode if required.
> 
> Since both of these options will cause the PEI Core to do a memory
> copy from Temporary RAM to the main memory, there might be platforms
> that they cannot work with as described above.
> 
> Thanks for your time,
> 
> -Jordan
> 

Re: [edk2-devel] [RFC] Remove PI spec TemporaryRamSupport PPI from PEI Core

[Nate DeSimone]

Hi Jordan,

The TemporaryRamSupport PPI is mandatory for FSP. The reason why is the stack migration algorithm for FSP is quite different compared to the standard method implemented in PEI core.

During FspMemoryInit(), PEI core runs on top of the bootloader stack. PEI core is not the root of the stack during FSP-M, which violates a lot of assumptions in PEI core's implementation of shadowing to permanent memory. During the process of migrating FSP to permanent memory, we break the single stack into two stacks. The custom implementation for FSP is here:

https://github.com/tianocore/edk2/blob/master/IntelFsp2Pkg/FspSecCore/SecMain.c

Thanks,
Nate

-----Original Message-----
From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Jordan Justen
Sent: Wednesday, April 24, 2019 12:51 AM
To: devel@edk2.groups.io
Cc: Ni, Ray <ray.ni@intel.com>; Andrew Fish <afish@apple.com>; Laszlo Ersek <lersek@redhat.com>; Leif Lindholm <leif.lindholm@linaro.org>; Kinney, Michael D <michael.d.kinney@intel.com>; Zimmer, Vincent <vincent.zimmer@intel.com>
Subject: [edk2-devel] [RFC] Remove PI spec TemporaryRamSupport PPI from PEI Core

Short summary: Despite being part of the Platorm Initialization Specificication, this proposal would remove support for the TemporaryRamSupport PPI from PEI Core. Arguably, this would mean the PEI Core does not support the PI spec, but we do not currently know if any platforms require this PPI.

Main question: Does anyone know of a platform that requires this PPI, or does anyone have major concerns with removing it before the PI specification can be updated to remove the PPI?

Alternatives: Continue to support the PPI, but we would need to merge a bug-fix patchset which adds some assembly code into the PEI Core.

More details:

I discussed this topic with the Tianocore Stewards, Vincent and Ray.
Although the path forward was not unanimous, it appears that the majority thought we should propose to remove support for the TemporaryRamSupport PPI from the PEI Core.

This PPI is defined in MdePkg/Include/Ppi/TemporaryRamSupport.h. I believe it has been present in all versions of the Platorm Initialization Specificication, including the latest, version 1.7.
(https://uefi.org/specsandtesttools)

The TemporaryRamSupport PPI is defined as "optional" in the PI spec, but I believe this only applied to producers of the PPI (PI platforms). The PEI Core (which is the consumer of this PPI) should arguably support the PPI in order to fully support PI spec based platforms.

But, there are some subtle issues with the PPI that make it difficult to implement for both the consumer (PEI Core) and the producer (the platform). In fact, there is a bug currently with the PEI Core's implementation, which I sent a patch series to address on April 10th, with the subject of "[PATCH v2 0/6] Fix PEI Core issue during TemporaryRamMigration".

Unfortunately the fix is not very simple, and requires adding assembly code to the PEI Core module. Based on this, we discussed if the TemporaryRamSupport PPI is no longer required by current PI platforms, despite being present in the PI spec. The only known example of it being required was a platform based on the IPF architecture, which is no longer supported by EDK II.

Several EDK II sample platforms produce this PPI in EDK II, but only as sample code. Clearly the removal would have to take this into account.

There are two replacements for the TemporaryRamSupport PPI, but they do not cover all types of potential hardware. If a platform cannot access main memory at the same time as the Temporary RAM, then it would not be supported by the alternatives. Nevertheless, the two replacements are:

1. Stop producing the PPI, and the PEI Core will automatically copy
   the required memory from Temporary RAM to the main memory. This
   will work for platforms that do not require special code to shut
   down Temporary RAM.

2. Produce the MdePkg/Include/Ppi/TemporaryRamDone.h PPI. This will
   cause the PEI Core to notify the platform after it has copied
   memory, and the platform can take special actions to disable
   Temporary RAM mode if required.

Since both of these options will cause the PEI Core to do a memory copy from Temporary RAM to the main memory, there might be platforms that they cannot work with as described above.

Thanks for your time,

-Jordan

Re: [edk2-devel] [RFC] Remove PI spec TemporaryRamSupport PPI from PEI Core

[Ni, Ray]

[-- Attachment #1: Type: text/plain, Size: 1446 bytes --]

Nate,
GCC generates assembly code that uses `RBP` to store `Private` local variable for below C code in PeiCore/Dispatcher.c

```c
      if (StackOffsetPositive) {
        SecCoreData = (CONST EFI_SEC_PEI_HAND_OFF *)((UINTN)(VOID *)SecCoreData + StackOffset);
        Private     = (PEI_CORE_INSTANCE *)((UINTN)(VOID *)Private + StackOffset);
      } else {
        SecCoreData = (CONST EFI_SEC_PEI_HAND_OFF *)((UINTN)(VOID *)SecCoreData - StackOffset);
        Private     = (PEI_CORE_INSTANCE *)((UINTN)(VOID *)Private - StackOffset);
      }

      TemporaryRamSupportPpi->TemporaryRamMigration (
                                PeiServices,
                                TemporaryRamBase,
                                (EFI_PHYSICAL_ADDRESS)(UINTN)(TopOfNewStack - TemporaryStackSize),
                                TemporaryRamSize
                                );

      PeiCore (SecCoreData, NULL, Private);
```

If `TemporaryRamMigration()` updates the `RBP` to point to physical memory by adding/subtracting
the `StackOffset`, that results the Private is added/subtracted by `StackOffset` twice: One in the C code before calling
TemporayRamSupport PPI, the other in `TemporaryRamMigration ()`.

Since `FspSecMain.SecSwitchStack()` does update the `RBP`, have you met the similar issue?

The issue doesn't always happen. It depends on whether `RBP` is used to store either `SecCoreData` or `Private`.

[-- Attachment #2: Type: text/html, Size: 1659 bytes --]