From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-1.mimecast.com (us-smtp-delivery-1.mimecast.com [205.139.110.61]) by mx.groups.io with SMTP id smtpd.web10.6259.1575590062179981796 for ; Thu, 05 Dec 2019 15:54:22 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=XexA5Qhi; spf=pass (domain: redhat.com, ip: 205.139.110.61, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1575590061; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=zLTnDZSUaBPxIsHdhpxrcFtW06xVbzTB6na2YITxUgc=; b=XexA5QhiA7A4OPYHbOY8Ulfk3ulIQtLKcTGxBbyb1LX9vlmV97PCXoGcGWbj3XI8sJ1KaO oIvXNSwG+FgCbcfFy8bz07Oc2x9s9uZcWup3jP7TE3w3MzEzcxORt4KTqLvQ8++n53RKRD rfAm6yGS/DeJ6QSwrChuIJluhfXgtw4= Received: from mimecast-mx01.redhat.com (mimecast-mx01.redhat.com [209.132.183.4]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-38-r1b4qe1uONyTFcrRkqPjGw-1; Thu, 05 Dec 2019 18:54:17 -0500 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.phx2.redhat.com [10.5.11.15]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 245702EDD; Thu, 5 Dec 2019 23:54:16 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-62.ams2.redhat.com [10.36.116.62]) by smtp.corp.redhat.com (Postfix) with ESMTP id 6733C5D6BB; Thu, 5 Dec 2019 23:54:14 +0000 (UTC) Subject: Re: [edk2-devel] [PATCH v1 1/1] ArmPkg: Dispatch deferred images after EndOfDxe To: devel@edk2.groups.io, ard.biesheuvel@linaro.org, Sami Mujawar Cc: Leif Lindholm , Matteo Carlini , nd References: <20190501140146.33224-1-sami.mujawar@arm.com> From: "Laszlo Ersek" Message-ID: <2689eabf-b2a4-466b-d0cc-f8786e7a35ee@redhat.com> Date: Fri, 6 Dec 2019 00:54:13 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.9.1 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.79 on 10.5.11.15 X-MC-Unique: r1b4qe1uONyTFcrRkqPjGw-1 X-Mimecast-Spam-Score: 0 Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit On 12/05/19 21:25, Ard Biesheuvel wrote: > On Wed, 1 May 2019 at 15:02, Sami Mujawar wrote: >> >> Third party driver images loaded from Option ROM get queued >> for execution after EndOfDxe. These queued images need to be >> dispatched from the PlatformBootManagerLib. >> >> Since the queued images were not dispatched, the PCI Option >> ROM drivers were not getting loaded on Juno. Therefore, >> add call to EfiBootManagerDispatchDeferredImages() for >> dispatching deferred images from PlatformBootManagerLib. >> >> Signed-off-by: Sami Mujawar >> --- >> >> The changes can be seen at https://github.com/samimujawar/edk2/tree/527_option_rom_loading_v1 >> >> ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c | 7 ++++++- >> 1 file changed, 6 insertions(+), 1 deletion(-) >> >> diff --git a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c >> index 0f91692c1a5ee6104bfef8545e4f436e53042178..71b857b5ba884c27ab870f6b75fa3e34d48e6060 100644 >> --- a/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c >> +++ b/ArmPkg/Library/PlatformBootManagerLib/PlatformBm.c >> @@ -2,7 +2,7 @@ >> Implementation for PlatformBootManagerLib library class interfaces. >> >> Copyright (C) 2015-2016, Red Hat, Inc. >> - Copyright (c) 2014, ARM Ltd. All rights reserved.
>> + Copyright (c) 2014 - 2019, ARM Ltd. All rights reserved.
>> Copyright (c) 2004 - 2018, Intel Corporation. All rights reserved.
>> Copyright (c) 2016, Linaro Ltd. All rights reserved.
>> >> @@ -554,6 +554,11 @@ PlatformBootManagerBeforeConsole ( >> EfiEventGroupSignal (&gEfiEndOfDxeEventGroupGuid); >> >> // >> + // Dispatch deferred images after EndOfDxe event. >> + // >> + EfiBootManagerDispatchDeferredImages (); >> + >> + // >> // Locate the PCI root bridges and make the PCI bus driver connect each, >> // non-recursively. This will produce a number of child handles with PciIo on >> // them. > > OK, given that other implementations of PlatformBootManagerLib also > make this call after signalling EndOfDxe, this is obviously the > correct thing to do. The right sequence of steps is, - signal EndOfDxe, - install DxeSmmReadyToLock, - dispatch deferred images. The first step gives platform drivers a last chance to do things - after all services have become available, - and SMRAM is still open. The second step locks down SMRAM before 3rd party drivers can mess around. (Sorry for the hand-wavy description.) > I'm still curious why this difference exists, What difference do you mean? (I can't see the original patch posting in my list folder, so I could be missing parts of the discussion thus far.) One interesting bit that I can see in the code is that EndOfDxe is signaled before connecting PCI root bridges. That doesn't seem productive. Until we connect root bridges, PciBusDxe won't bind them, and won't collect the drivers in the ROM BARs -- they won't be there to "defer" and "dispatch" later. Normally, step "connect root bridges" would be *prepended* to the above three steps. I don't understand where the preexistent disorder comes, between signaling EndOfDxe, and connecting the root bridges. ... Oh wait. We made a mistake in commit 9cd7d3c5ba44 ("ArmVirtPkg: signal EndOxDxe event in PlatformBsdInit", 2015-06-25). In that commit, we signaled EndOfDxe in PlatformBdsInit() -- before connecting the root bridges at the top of PlatformBdsPolicyBehavior(). (Look at the patch with a huge context, like -U300.) The BdsEntry() function in BdsDxe would call PlatformBdsInit() before calling PlatformBdsPolicyBehavior(). So that commit is where the disorder comes from. The disorder was faithfully preserved through: - dfc9514794fc ("ArmVirtPkg/PlatformIntelBdsLib: rebase to EfiEventGroupSignal", 2016-03-23) - 8d6203223ab1 ("ArmVirtPkg: duplicate PlatformIntelBdsLib to PlatformBootManagerLib", 2016-05-06) - e3fe3c0ff94d ("ArmVirtPkg/PlatformBootManagerLib: follow PlatformBootManagerLib interfaces", 2016-05-06) - e2a193b733aa ("ArmVirtPkg/PlatformBootManagerLib: init console vars in BeforeConsole()", 2016-05-06) The last commit in the list is where the disorder becomes very visible Then, the disorder was copied into ArmPkg too, in commit c976f9cb7dbd ("ArmPkg/PlatformBootManagerLib: implement new generic version", 2016-05-12) -- see the PlatformBootManagerBeforeConsole() function. Later, in commit 34cd940294e5 ("ArmVirPkg/PlatformBds: Dispatch deferred images after EndOfDxe", 2016-11-10), we added the EfiBootManagerDispatchDeferredImages() to ArmVirtPkg. However, with the pre-existent disorder in place (originally from commit 9cd7d3c5ba44, see above), that call could not be inserted in the 100% correct place. I mean: during all that time, strictly speaking, it had not been *wrong* to dispatch the deferred images before connecting PCI root bridges. The important thing was to delay 3rd party dispatch until after (at least) EndOfDxe -- that way, platform drivers could "lock down" themselves, before the platform unleashed 3rd party drivers. So the solution was correct; it just wasn't *complete* -- because one (maybe the most important?) source of the deferred images is the PCI oproms. So that's what we have today in ArmVirtPkg -- and the present patch seems to be porting 34cd940294e5 to ArmPkg. The proper solution would be the following (if I may suggest an idea): (1) fix ArmVirtPkg: hoist the following part: > // > // Locate the PCI root bridges and make the PCI bus driver connect each, > // non-recursively. This will produce a number of child handles with PciIo on > // them. > // > FilterAndProcess (&gEfiPciRootBridgeIoProtocolGuid, NULL, Connect); > > // > // Signal the ACPI platform driver that it can download QEMU ACPI tables. > // > EfiEventGroupSignal (&gRootBridgesConnectedEventGroupGuid); above this part: > // > // Signal EndOfDxe PI Event > // > EfiEventGroupSignal (&gEfiEndOfDxeEventGroupGuid); > > // > // Dispatch deferred images after EndOfDxe event. > // > EfiBootManagerDispatchDeferredImages (); This will make: - ArmVirtPkg match the order employed in OvmfPkg, - EfiBootManagerDispatchDeferredImages() pick up PCI option ROMs, - run the ACPI platform driver *before* EndOfDxe; but that's just fine. (In fact, in OvmfPkg, that's *required*. See the FACS note there.) (2) Port the same fix to ArmPkg: namely move > // > // Locate the PCI root bridges and make the PCI bus driver connect each, > // non-recursively. This will produce a number of child handles with PciIo on > // them. > // > FilterAndProcess (&gEfiPciRootBridgeIoProtocolGuid, NULL, Connect); just above > // > // Signal EndOfDxe PI Event > // > EfiEventGroupSignal (&gEfiEndOfDxeEventGroupGuid); (3) In a separate patch, call EfiBootManagerDispatchDeferredImages() just after the *new* location of signaling EndOfDxe. ... I think this disorder / bug may have remained dormant so far (ever since 9cd7d3c5ba44) because in ArmVirt, we have had practically zero use for option ROMs. With OVMF, there's always physical GPU assignment that could have triggered the problem quite soon. So now that you have actual use for oproms on Juno, the disorder was laid bare. Except... I'm not sure how the *present* patch is supposed to fix that problem, given that the dispatch of deferred (3rd party) images would still occur *before* enumerating the PCI option ROMs! So either the patch (as posted) is functionally wrong, or something is amiss in my analysis above. :) > but it doesn't make sense to block this patch from going in, > considering that I haven't been able to investigate. > > So I'll merge this - I just need to figure out how to use the new CI > based workflow :-) Short version: - prepare a local topic branch, on top of master, with the review tags in place, such that (per the earlier method) you'd ff-push that topic branch as the new edk2 master. - instead of pushing it to edk2, push it to your personal edk2 repo on github.com - log in to github.com, and initiate a pull request against edk2/master, from your personal topic branch - as soon as the pull request has been filed, locate the "labels" box to the right, and apply the "push" label. Once the CI tests complete, the branch will be merged (fast-forwarded). Thanks, Laszlo > > >