Re: [PATCH] EmulatorPkg/Sec: Don't install TemporaryRamSupport PPI

[Jordan Justen <jordan.l.justen@intel.com>]

On 2019-03-04 18:37:03, Ni, Ray wrote:
> 
> > -----Original Message-----
> > From: edk2-devel <edk2-devel-bounces@lists.01.org> On Behalf Of Jordan
> > Justen
> > Sent: Sunday, March 3, 2019 5:45 AM
> > To: Ni, Ray <ray.ni@intel.com>; edk2-devel@lists.01.org
> > Subject: Re: [edk2] [PATCH] EmulatorPkg/Sec: Don't install
> > TemporaryRamSupport PPI
> > 
> > On 2019-03-02 04:11:11, Ray Ni wrote:
> > >
> > > So this patch removes TemporaryRamSupport PPI implementation from
> > > EmulatorPkg/Sec module to fix the boot failure when using GCC5.
> > 
> > I don't think we should just hide the known bug with the
> > TemporaryRamSupport PPI implementation in the PEI Core.
> > 
> > I would agree that we should make this change after addressing that.
> 
> Jordan,
> I have a goal to replace Nt32 with EmulatorPkg before Q2 stable tag release.
> I understand we need more discussions on how to fix the PeiCore bug.
> So I don't want this blocks the Nt32 deletion.

Given the choice of fixing a known bug in PEI Core relating to the PI
spec TemporaryRamSupport PPI, or deleting Nt32, I would choose fixing
the bug.

But, why not fix the bug, and then make this change?

> And in my opinion, the TemporaryRamSupport PPI that requires being called
> from assembly code is a design bug.

I agree. It's unfortunate, but it is part of the PI spec, and unlikely
to be changed very soon.

Even if a new PPI is added, I would guess that the old PPI might
remain in the PI spec. Maybe it would be marked as deprecated at some
point and then removed later?

I am no fan adding assembly language unnecessarily. But, I can't see a
way around it in this case.

> I suggest to change the PI spec instead of changing PeiCore by introducing
> more assembly code to hide this design bug.

I did prototype a TemporaryRamSupport2 PPI under:

https://github.com/jljusten/edk2/tree/temp-ram-support2

My prototype still used assembly, as I don't think the
TemporaryRamSupport PPI would be quickly removed from the PI spec.
(And, therefore, I think edk2's PEI Core should support it without
known bugs.)

But, I think the new TemporaryRamSupport2 PPI should be possible to
safely implement in C code for both the PEI Core and in the PPI
implementation.

-Jordan