From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-1.mimecast.com (us-smtp-1.mimecast.com [205.139.110.120]) by mx.groups.io with SMTP id smtpd.web10.5463.1575647042278911562 for ; Fri, 06 Dec 2019 07:44:02 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=FugXERNH; spf=pass (domain: redhat.com, ip: 205.139.110.120, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1575647041; 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=hDbPGs7Ong+e/icbciBr33ngqAZIYaQigXnuYOAhXao=; b=FugXERNHneuP7oZdBG2BWIxxyv8hH5ObcWMKXSi0MWnOJ1inQlYz0B3RHv8D9SWfTWhise sumwKK+yJHHAfYpoJtMHgSu2CT2QlB+nA2oN61o6m73NVO1sQZFL5WH1IfqZ6nlVZIOCg/ UEfCb8vjo9DRzY71iN/ZTTD947n8FDk= 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-222-M54D2bGMPj-ksu3I_283Xg-1; Fri, 06 Dec 2019 10:43:59 -0500 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 7EEBB593A2; Fri, 6 Dec 2019 15:43:58 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-235.ams2.redhat.com [10.36.116.235]) by smtp.corp.redhat.com (Postfix) with ESMTP id 8B7CA5C1C3; Fri, 6 Dec 2019 15:43:56 +0000 (UTC) Subject: Re: [RFC PATCH 2/2] MdeModulePkg/BdsDxe: allow consoles on drivers loaded via Driver#### To: Ard Biesheuvel , devel@edk2.groups.io Cc: Leif Lindholm , Zhichao Gao , Ray Ni , Maurice Ma , Guo Dong , Benjamin You References: <20191206143128.19371-1-ard.biesheuvel@linaro.org> <20191206143128.19371-3-ard.biesheuvel@linaro.org> From: "Laszlo Ersek" Message-ID: <8d49cca6-b1b9-cddc-0590-4e4a84643e4f@redhat.com> Date: Fri, 6 Dec 2019 16:43:55 +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: <20191206143128.19371-3-ard.biesheuvel@linaro.org> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.16 X-MC-Unique: M54D2bGMPj-ksu3I_283Xg-1 X-Mimecast-Spam-Score: 0 Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit On 12/06/19 15:31, Ard Biesheuvel wrote: > Currently, we dispatch drivers specified via Driver#### entries after > calling PlatformBootManagerBeforeConsole(), which prevents us from > loading drivers that provide consoles via Driver#### entries. I'd put this as: ... which prevents PlatformBootManagerBeforeConsole() from adding such consoles to ConIn, ConOut and StdErr that are produced by drivers loaded via Driver#### options. > This is particularly annoying on AArch64 systems, given that it prevents > us from loading AArch64 drivers from the ESP for PCIe graphics cards > that shipped with x86 drivers in the option ROM, which is the common > case today. > > So let's reorder the handling of the Driver#### entries with the > invocation of PlatformBootManagerBeforeConsole(), which results in > Driver#### entries being dispatched in the same way as option ROM > images. Ah, so you are saying the drivers will be added unconditionally to the deferred list, with this patch (because, at the new ProcessLoadOptions() call site, the platform cannot have signaled EndOfDxe yet). Then in PlatformBootManagerBeforeConsole(), the platform can signal EndOfDxe, call EfiBootManagerDispatchDeferredImages(), connect GOP drivers, see what new handles / device paths are created, and *then* update ConIn / ConOut / ConErr. Can you point out where the deferral happens, inside ProcessLoadOptions()? (If there is no deferral, then we can't / shouldn't do this, as drivers from the ESP should not be loaded before EndOfDxe, I think.) Hmmm... it looks like: BdsEntry() [MdeModulePkg/Universal/BdsDxe/BdsEntry.c] ProcessLoadOptions() [MdeModulePkg/Universal/BdsDxe/BdsEntry.c] EfiBootManagerProcessLoadOption() [MdeModulePkg/Library/UefiBootManagerLib/BmLoadOption.c] CoreLoadImage() [MdeModulePkg/Core/Dxe/Image/Image.c] -- via gBS->LoadImage() CoreLoadImageCommon() [MdeModulePkg/Core/Dxe/Image/Image.c] Security2StubAuthenticate() [MdeModulePkg/Universal/SecurityStubDxe/SecurityStub.c] -- via gSecurity2->FileAuthentication() Defer3rdPartyImageLoad() [MdeModulePkg/Universal/SecurityStubDxe/Defer3rdPartyImageLoad.c] // checks "mEndOfDxe" QueueImage() [MdeModulePkg/Universal/SecurityStubDxe/Defer3rdPartyImageLoad.c] OK. I think this series makes sense. I defer to Ray, but from my POV, it should be OK. With the patch#2 commit message updated: series Reviewed-by: Laszlo Ersek Thanks Laszlo > > Signed-off-by: Ard Biesheuvel > --- > MdeModulePkg/Universal/BdsDxe/BdsEntry.c | 15 ++++++++------- > 1 file changed, 8 insertions(+), 7 deletions(-) > > diff --git a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c > index 1fb04dcbbcda..ad4c4c0406f6 100644 > --- a/MdeModulePkg/Universal/BdsDxe/BdsEntry.c > +++ b/MdeModulePkg/Universal/BdsDxe/BdsEntry.c > @@ -850,6 +850,14 @@ BdsEntry ( > } > } > > + // > + // Execute Driver Options. Images will be loaded but dispatch will be > + // deferred for 3rd party images until EndOfDxe is signalled. > + // > + LoadOptions = EfiBootManagerGetLoadOptions (&LoadOptionCount, LoadOptionTypeDriver); > + ProcessLoadOptions (LoadOptions, LoadOptionCount); > + EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount); > + > // > // Do the platform init, can be customized by OEM/IBV > // Possible things that can be done in PlatformBootManagerBeforeConsole: > @@ -868,13 +876,6 @@ BdsEntry ( > // > EfiBootManagerStartHotkeyService (&HotkeyTriggered); > > - // > - // Execute Driver Options > - // > - LoadOptions = EfiBootManagerGetLoadOptions (&LoadOptionCount, LoadOptionTypeDriver); > - ProcessLoadOptions (LoadOptions, LoadOptionCount); > - EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount); > - > // > // Connect consoles > // >