public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Ni, Ray" <ray.ni@intel.com>
To: "Justen, Jordan L" <jordan.l.justen@intel.com>,
	"Gao, Liming" <liming.gao@intel.com>,
	"edk2-devel@lists.01.org" <edk2-devel@lists.01.org>
Cc: "Wu, Hao A" <hao.a.wu@intel.com>,
	Anthony Perard <anthony.perard@citrix.com>,
	Laszlo Ersek <lersek@redhat.com>,
	"Zeng, Star" <star.zeng@intel.com>, Andrew Fish <afish@apple.com>
Subject: Re: [PATCH 00/10] Fix PEI Core issue during TemporaryRamMigration
Date: Thu, 21 Feb 2019 04:43:05 +0000	[thread overview]
Message-ID: <734D49CCEBEEF84792F5B80ED585239D5C0296EE@SHSMSX104.ccr.corp.intel.com> (raw)
In-Reply-To: <155071099391.27635.17148748494622597613@jljusten-skl>



> -----Original Message-----
> From: Justen, Jordan L <jordan.l.justen@intel.com>
> Sent: Thursday, February 21, 2019 9:03 AM
> To: Gao, Liming <liming.gao@intel.com>; Ni, Ray <ray.ni@intel.com>; edk2-
> devel@lists.01.org
> Cc: Wu, Hao A <hao.a.wu@intel.com>; Anthony Perard
> <anthony.perard@citrix.com>; Laszlo Ersek <lersek@redhat.com>; Zeng,
> Star <star.zeng@intel.com>; Andrew Fish <afish@apple.com>
> Subject: RE: [PATCH 00/10] Fix PEI Core issue during TemporaryRamMigration
> 
> On 2019-02-20 16:15:59, Ni, Ray wrote:
> > > -----Original Message-----
> > > From: Justen, Jordan L <jordan.l.justen@intel.com>
> > > Sent: Thursday, February 21, 2019 1:44 AM
> > > To: Gao, Liming <liming.gao@intel.com>; Ni, Ray <ray.ni@intel.com>;
> > > edk2- devel@lists.01.org
> > > Cc: Wu, Hao A <hao.a.wu@intel.com>; Anthony Perard
> > > <anthony.perard@citrix.com>; Laszlo Ersek <lersek@redhat.com>; Zeng,
> > > Star <star.zeng@intel.com>
> > > Subject: Re: [PATCH 00/10] Fix PEI Core issue during
> > > TemporaryRamMigration
> > >
> > > On 2019-02-20 05:27:21, Ni, Ray wrote:
> > > > On 2/19/2019 9:25 PM, Gao, Liming wrote:
> > > > > Ray:
> > > > >
> > > > >      Now, real platform has no side effect. So, only TempRamDone
> > > > >      PPI is produced. For emulator platform, is there any side
> > > > >      effect when both Temporary RAM and Permanent RAM are
> enabled?
> > > > >
> > > >
> > > > No side effect when both of T-RAM and P-RAM are enabled.
> > > > Which means no side effect when neither of the PPIs is produced.
> > > > But for demo purpose, I think producing TemporaryRamDone PPI
> makes
> > > sense.
> > >
> > > In addition to being a demo/sample, it also provides a check that no
> > > modules are accessing temp-ram after temp-ram should no longer be
> used.
> > >
> > > > I will work out patches for EmulatorPkg to produce TemoraryRamDone.
> > >
> > > I think we should first fix TemporaryRamSupport usage. Otherwise, we
> > > are just hiding the bug.
> > >
> > > Have you tried these patches to verify that they fix the issue in your
> setup?
> > > Have you taken a look at the patches to see what problem is being fixed?
> >
> > I feel the change to PeiCore is a bit complex and introduce additional deps
> (assembly).
> 
> I asked about this last November, and as I recall, Andrew said we can (and
> should) add assembly to PEI Core if it is required to meet the specs.
> 
> > Behavior of ARM and x86 becomes different. (The patch only fixes x86
> > issue.)
> 
> Yes. Similar code should be written for ARM, but I don't have experience
> with ARM assembly code.
> 
> > Given there is no real requirement on this,
> 
> Isn't the requirement to be compatible with the PI specification?
> 
> It seems that at least you and Liu Yu have encountered a build environment
> that produces code for PEI Core that isn't compatible with the PI specification.
> 
> It doesn't seem like the best response to this is to just change the platform
> boot path and ignore the bug.

The environment Liu Yu and I encountered is the same Emulator environment.
I think the code change you did to PeiCore is great. It re-wrote the C code in
assembly to make sure the implementation doesn't rely on C compiler's
behavior.
But looking back, if a PI spec interface should depend on assembly
Implementation, I will pursue to change the PI spec interface directly.
Given the fact that no platform is producing this RamMigration PPI,
changing PI spec then changing the PeiCore to use new PPI interface
has no impact.

To be honest, I don't want to introduce complexity to PeiCore or edk2. That's
my only concern.

> 
> I do agree that after this issue is fixed, we can then consider changing the
> platform. The downside to changing it then will be to leave an untested code
> path, but at least we won't leave it with a known bug present.
> 
> -Jordan
> 
> > I prefer to not change PeiCore.

  reply	other threads:[~2019-02-21  4:43 UTC|newest]

Thread overview: 29+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-02-18  4:11 [PATCH 00/10] Fix PEI Core issue during TemporaryRamMigration Jordan Justen
2019-02-18  4:11 ` [PATCH 01/10] EmulatorPkg/build.sh: Fix missing usage of -b BUILDTARGET parameter Jordan Justen
2019-02-18  4:11 ` [PATCH 02/10] EmulatorPkg/Unix/Host: Use PcdInitValueInTempStack to init temp-ram Jordan Justen
2019-02-18  4:11 ` [PATCH 03/10] EmulatorPkg/Sec: Replace assembly temp-ram support with C code Jordan Justen
2019-02-18  4:11 ` [PATCH 04/10] EmulatorPkg/Sec: Disable optimizations for TemporaryRamMigration function Jordan Justen
2019-02-18  4:11 ` [PATCH 05/10] OvmfPkg/Sec: Swap TemporaryRam Stack and Heap locations Jordan Justen
2019-02-18 12:58   ` Laszlo Ersek
2019-02-18  4:11 ` [PATCH 06/10] OvmfPkg/Sec: Disable optimizations for TemporaryRamMigration Jordan Justen
2019-02-18  7:53   ` Ard Biesheuvel
2019-02-18  9:08     ` Jordan Justen
2019-02-18  9:32       ` Ard Biesheuvel
2019-02-18 13:01         ` Laszlo Ersek
2019-02-19 22:50         ` Brian J. Johnson
2019-02-19 23:58           ` Jordan Justen
2019-02-20  8:52         ` Jordan Justen
2019-02-20  8:59           ` Ard Biesheuvel
2019-02-18  4:11 ` [PATCH 07/10] MdeModePkg/Core/Pei: Add code path to allow assembly temp-ram migration Jordan Justen
2019-02-18  4:11 ` [PATCH 08/10] MdeModulePkg/Core/Pei: Use assembly for X64 TemporaryRamMigration Jordan Justen
2019-02-18  4:11 ` [PATCH 09/10] MdeModulePkg/Core/Pei: Use assembly for IA32 TemporaryRamMigration Jordan Justen
2019-02-18  4:11 ` [PATCH 10/10] OvmfPkg/Sec: Fill Temp Ram after TemporaryRamMigration Jordan Justen
2019-02-18 13:15   ` Laszlo Ersek
2019-02-19  2:46 ` [PATCH 00/10] Fix PEI Core issue during TemporaryRamMigration Ni, Ray
2019-02-19 13:25   ` Gao, Liming
2019-02-20 13:27     ` Ni, Ray
2019-02-20 17:43       ` Jordan Justen
2019-02-21  0:15         ` Ni, Ray
2019-02-21  1:03           ` Jordan Justen
2019-02-21  4:43             ` Ni, Ray [this message]
2019-02-19 19:27   ` Jordan Justen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=734D49CCEBEEF84792F5B80ED585239D5C0296EE@SHSMSX104.ccr.corp.intel.com \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox