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

[Laszlo Ersek <lersek@redhat.com>]

On 03/16/18 14:45, Ard Biesheuvel wrote:
> 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.

Are you OK with the patch with this information available?

Should I include some (or all) of the above in the commit message?

Thanks!
Laszlo