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 > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#39511): https://edk2.groups.io/g/devel/message/39511 Mute This Topic: https://groups.io/mt/31319621/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
