Re: [PATCH 1/5] ArmVirtPkg/PlatformBootManagerLib: return to "-kernel before boot devices"

[Ard Biesheuvel <ard.biesheuvel@linaro.org>]

On 16 March 2018 at 13:40, Laszlo Ersek <lersek@redhat.com> wrote:
> On 03/16/18 10:58, Ard Biesheuvel wrote:
>> On 15 March 2018 at 19:02, Laszlo Ersek <lersek@redhat.com> wrote:
>>> Move the TryRunningQemuKernel() call back to its original place.
>>
>> When was it moved and why?
>
> See, I'm at a loss about the detail you expect in commit messages. :)

I see. But in this case, I think it is rather obvious that when you
move something back to where it was moved away from earlier, you have
to justify why the original reason for moving it no longer applies, or
is overruled by something more important.

> I
> had spent 10 minutes on wording this commit message. Near the end of my
> struggle, I had exactly one sentence on each of those three commits that
> I listed at the bottom (23d04b58e27b, a78c4836ea0b, 158990b941e4). Those
> three sentences were going to give the answer to your question. I still
> worried you might file them under "personal journey" or "stream of
> consciousness", and I cut them; I only left the three commit hashes at
> the end as pointers.
>

I looked at those commit logs but it wasn't obvious to me.

> So, the executive summary is: "TryRunningQemuKernel() was moved in
> a78c4836ea0b, because the guest kernel depended on ACPI tables, which
> depended on our ACPI platform driver, which at the time depended on
> PciBusDxe itself reporting 'enumeration complete', which at the time
> depended on our BdsLibConnectAll() call".
>
> The somewhat expanded answer is, from scratch:
>
> (1) in commit 23d04b58e27b, we introduced TryRunningQemuKernel() in the
>     right spot (for boot performance), between
>     PlatformBdsConnectConsole() and BdsLibConnectAll();
>
> (2) in commit a78c4836ea0b, we moved TryRunningQemuKernel() after
>     BdsLibConnectAll(): the direct-booted kernel needed ACPI tables
>     (reflecting PCI resources correctly), and at that time, we only
>     connected the root bridges to PciBusDxe as part of
>     BdsLibConnectAll() (which signaled the ACPI platform driver via
>     "gEfiPciEnumerationCompleteProtocolGuid");
>
> (3) in commit 60dc18a17c516 and (more importantly) 158990b941e4,
>     connecting the root bridges and cueing the ACPI platform driver were
>     finally separated from BdsLibConnectAll(), but we didn't realize we
>     could move TryRunningQemuKernel() back above BdsLibConnectAll().
>
> So, after 158990b941e4, we can do it now.
>

Thanks for clearing that up.