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

[Laszlo Ersek <lersek@redhat.com>]

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