Re: [Patch] UefiCpuPkg PiSmmCpuDxeSmm: Update SmiEntry function run the same position

["Gao, Liming" <liming.gao@intel.com>]

Laszlo:
  I understand your point. I agree your suggestion. BZ https://bugzilla.tianocore.org/show_bug.cgi?id=1191 has been submitted. Yes. PiSmmCpuSmiEntryFixupAddress() is called in the driver entry point to fix up the address first. 

  I will send V2 patch with the detail commit message and code comments.

> -----Original Message-----
> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Thursday, September 13, 2018 3:49 AM
> To: Gao, Liming <liming.gao@intel.com>
> Cc: edk2-devel@lists.01.org; Yao, Jiewen <jiewen.yao@intel.com>; Dong, Eric <eric.dong@intel.com>; Kinney, Michael D
> <michael.d.kinney@intel.com>; Leif Lindholm (Linaro address) <leif.lindholm@linaro.org>; Ard Biesheuvel <ard.biesheuvel@linaro.org>;
> Andrew Fish <afish@apple.com>; Cetola, Stephano <stephano.cetola@intel.com>
> Subject: Re: [edk2] [Patch] UefiCpuPkg PiSmmCpuDxeSmm: Update SmiEntry function run the same position
> 
> On 09/12/18 17:42, Gao, Liming wrote:
> > Laszlo:
> >   Before commit e21e355e2ca7fefb15b4df7078f995d3fb9c2b89, jmp _SmiHandler is commented. And below code,
> ASM_PFX(CpuSmmDebugEntry) is moved into rax, then call it. But, this code doesn't work in XCODE5 tool chain. Like you say, XCODE5
> doesn't generated the absolute address in the EFI image. So, rax stores the relative address. Once this logic is moved to another place, it
> will not work.
> > ;   jmp     _SmiHandler                 ; instruction is not needed
> > ...
> >     mov     rax, ASM_PFX(CpuSmmDebugEntry)
> >     call    rax
> >
> >  Commit e21e355e2ca7fefb15b4df7078f995d3fb9c2b89 is to support XCODE5. I choose one tricky way to fix it. Although SmiEntry
> logic is copied to another place and run, but here I enable jmp _SmiHandler to jmp the original code location, then call
> ASM_PFX(CpuSmmDebugEntry) with the relative address can work.
> >     mov     rax, strict qword 0         ;   mov     rax, _SmiHandler
> > _SmiHandlerAbsAddr:
> >     jmp     rax
> > ...
> >     call    ASM_PFX(CpuSmmDebugEntry)
> >
> >   Today, Jiewen raises the issue that SmiHandler should run in the copied address, can't run in the common address. So, I update its
> logic and remove jmp _SmiHandler, then keep code continue run in copied address. But, I still need to fix up CpuSmmDebugEntry
> address to the absolute address. They are both for this issue. They can't be separated.
> >
> > ...
> >     mov     rax, strict qword 0         ;   call    ASM_PFX(CpuSmmDebugEntry)
> > CpuSmmDebugEntryAbsAddr:
> >     call    rax
> 
> Thank you very much for the explanation. I understand now.
> 
> I also understand why I got confused so much earlier. The reason is that
> the code was not commented sufficiently. It should have been pointed out
> what part of the instruction stream was meant to be executed from the
> copied address space (that is, not from the PiSmmCpuDxeSmm image
> itself), and what part of the instruction stream was meant to execute
> from within the the PiSmmCpuDxeSmm image. Without such comments, it's
> too hard to understand the transitions.
> 
> You or Jiewen should please file a TianoCore BZ describing the current
> issue. Namely that, due to circumstances you are not allowed to share,
> no part of the _SmiHandler routine should execute from within the
> PiSmmCpuDxeSmm image; instead, all of it should execute from the
> copied-to address space. This requires eliminating the jump back into
> the PiSmmCpuDxeSmm image, and also patching up the relative addresses
> (to absolute addresses), for those instructions that used to run from
> within the PiSmmCpuDxeSmm image, but now no longer will. The necessary
> changes should not affect the behavior of platforms that already consume
> PiSmmCpuDxeSmm from edk2.
> 
> The historical overview you provide above is also valuable, please
> include it in the commit message and/or the BZ as well.
> 
> It would still be nice to comment what parts of the source file run from
> within the PiSmmCpuDxeSmm image. For example,
> PiSmmCpuSmiEntryFixupAddress() does, right?
> 
> --*--
> 
> Please understand that my issue here is more serious than just missing
> the explanation / motivation, for this specific patch. My issue is that
> the posting of this work to edk2-devel should have *started* with the
> above explanation. You and Jiewen understand the issue. Noone else on
> the list (that doesn't work at your office) does. I've wasted half a day
> because you didn't write up the background up-front.
> 
> I don't need to know your specific internal proprietary feature that
> requires this change. I do need to know that an internal feature with
> such a requirement exists, and that it is your motivation. Every ten
> minutes you save for yourselves on documentation is amplified to several
> hours of struggle, for each reviewer. And we specifically discussed this
> scenario with Mike at one of the earlier stewards' meetings.
> 
> (The general topic back then was my raising that you [plural] liberally
> commit whatever you want to MdePkg / MdeModulePkg, without really naming
> the use cases, let alone adding client code to edk2. Whereas non-Intel
> contributors with a demonstrated need are heavily gated from adding code
> to MdePkg / MdeModulePkg.
> 
> Compare: the addition of the "PciSegmentLibSegmentInfo" instances
> (commit 5c9bb86f171c), without any consumer in edk2 whatsoever, versus
> BZ#957, where the new libraries -- originally proposed for MdePkg -- had
> demonstrated consumers, just not from Intel. Do you see the double
> standard?)
> 
> I wish I could just ignore UefiCpuPkg. I can't do that; it is very
> brittle / sensitive, and has caused numerous regressions for OVMF. I
> must keep a close eye on it. But patches from you [plural] certainly
> don't make that easy for me.
> 
> It kills me that after years of us begging on the list, you [plural]
> still don't care about non-Intel participants as first class citizens
> up-front, until they raise a stink.
> 
> Here's another recent example:
> 
> https://bugzilla.tianocore.org/show_bug.cgi?id=1116#c3
> https://bugzilla.tianocore.org/show_bug.cgi?id=1116#c4
> 
> Please start actually believing in, and practicing, Open Development.
> You no longer write the code and the docs for Intel only. Convincing
> your colleagues is easy -- that's no different from the proprietary /
> closed source days of firmware development. It was Intel's decision to
> turn edk2 into an open source project; please live that decision. Now
> your primary audience is the contributors that depend on collaborating
> with you *without* working for your employer.
> 
> Well I've probably overstated my case. It's just that I've hoped for
> better commit messages since that stewards' meeting.
> 
> Laszlo