Re: [edk2-devel] [PATCH] UefiCpuPkg:Fixed AsmRelocateApLoopStart and ensure allocated memory <4GB

["Laszlo Ersek" <lersek@redhat.com>]

On 1/6/23 09:43, Xie, Yuanhao wrote:
> Hi all,
>
> Thanks for feedbacks. I will revert the original ones, and send the
> new version.

OK, thanks.

Please pay attention the ordering of the reverts.

The original series was merged in the following order:

(a) 1  7bda8c648192 UefiCpuPkg: Duplicated AsmRelocateApLoop as AsmRelocateApLoopAmd
    2  73ccde8f6d04 UefiCpuPkg: Has APs in 64 bit long-mode before booting to OS.
    3  4a8642422460 OvmfPkg: Add CpuPageTableLib required by MpInitLib.
    4  3f378450dfaf UefiPayloadPkg: Add CpuPageTableLib required by MpInitLib.

Commit #2 introduced a new lib class dependency. The dependency was
resolved for OvmfPkg and UefiPayloadPkg only in patches #3 and #4,
respectively.

This means that, if someone checks out the tree at #2 or #3, then at #2,
neither the OvmfPkg nor UefiPayloadPkg platforms build, and at #3, the
UefiPayloadPkg platforms don't build:

> $ git checkout 73ccde8f6d04
>
> $ build -a X64 -b NOOPT -p OvmfPkg/OvmfPkgX64.dsc -t GCC5
>
> [...]
>
> .../OvmfPkg/OvmfPkgX64.dsc(...): error 4000: Instance of library class [CpuPageTableLib] is not found
>         in [.../UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf] [X64]
>         consumed by module [.../UefiCpuPkg/CpuDxe/CpuDxe.inf]

This is a problem. It's a problem because a broken build interferes with
the "git bisect" command's ability to narrow down a functional (runtime)
issue to a specific patch. If you can't build the tree at a particular
commit, then you cannot test whether that commit already contains the
regression, or not.

Usually when a series is reverted, the revert commits are ordered in
reverse to the original commits. However, in this case, we shouldn't do
that, because then we'd introduce two more commits into the git history
at which the tree doesn't build.

The proper original order (for keeping the tree buildable at all times)
would have been the following (moving (a)/#2 to the end, so that by the
time the CpuPageTableLib dependency is introduced to CpuDxe, all
CpuDxe-dependent DSC files in the tree have a CpuPageTableLib
resolution):

(b) 1  7bda8c648192 UefiCpuPkg: Duplicated AsmRelocateApLoop as AsmRelocateApLoopAmd
    2  4a8642422460 OvmfPkg: Add CpuPageTableLib required by MpInitLib.
    3  3f378450dfaf UefiPayloadPkg: Add CpuPageTableLib required by MpInitLib.
    4  73ccde8f6d04 UefiCpuPkg: Has APs in 64 bit long-mode before booting to OS.

Therefore the right order to revert is the inverse of (b), and not the
inverse of (a):

git revert 73ccde8f6d04 # UefiCpuPkg: Has APs in 64 bit long-mode before booting to OS.
git revert 3f378450dfaf # UefiPayloadPkg: Add CpuPageTableLib required by MpInitLib.
git revert 4a8642422460 # OvmfPkg: Add CpuPageTableLib required by MpInitLib.
git revert 7bda8c648192 # UefiCpuPkg: Duplicated AsmRelocateApLoop as AsmRelocateApLoopAmd

This keeps the tree buildable at every stage of the revert.

The importance of this cannot be over-emphasized. If someone
investigates a completely unrelated problem in edk2 in a year from now,
and they don't even know what package the issue could be in, so they
can't restrict "git-bisect" to a particular package, then their
git-bisect run could very well land on one of the non-building commits,
at some point. The present UefiCpuPkg commits may be totally irrelevant
for their problem, but they won't be able to tell or test that, because
the tree will simply not build for them. "git-bisect" supports a command
called "git bisect skip", which more or less deals with such situations,
by picking a "nearby" commit to try, but that's really just a kludge.

Laszlo