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.web12.12346.1578673432551319171 for ; Fri, 10 Jan 2020 08:23:52 -0800 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=GRiC9GlU; 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=1578673431; 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=1DyEcZLUPJ5PfahXTOzXWJKXAyKEDEisNe6bgUSQF8M=; b=GRiC9GlU7rAFzGAF+JAahYyTPrQLS8utF2te5QbpK8XSQDz9ar1jKmubI5L/jdpNTcGSRC RW2ZAgLgxhC9TYtspS+Dyi/zKyYJOIXvhAS/cSFglxjPltlLCpsiVhguHUHyuI+0D3W0ts vkAiFJ+wbQM+XHYpmGWZBX/YOjnP8AQ= 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-180-_qNcQ_dPP1mFOYlpkG6uCg-1; Fri, 10 Jan 2020 11:23:48 -0500 Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.14]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx01.redhat.com (Postfix) with ESMTPS id 8F6EB102C8D4; Fri, 10 Jan 2020 16:23:46 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-116-179.ams2.redhat.com [10.36.116.179]) by smtp.corp.redhat.com (Postfix) with ESMTP id A1E265DA60; Fri, 10 Jan 2020 16:23:44 +0000 (UTC) Subject: Re: [edk2-devel] [RFC PATCH 2/2] MdeModulePkg/BdsDxe: allow consoles on drivers loaded via Driver#### To: "Ni, Ray" , "'devel@edk2.groups.io'" , "'ard.biesheuvel@linaro.org'" Cc: 'Leif Lindholm' , "Gao, Zhichao" , "Ma, Maurice" , "Dong, Guo" , "You, Benjamin" References: <20191206143128.19371-1-ard.biesheuvel@linaro.org> <20191206143128.19371-3-ard.biesheuvel@linaro.org> <8d49cca6-b1b9-cddc-0590-4e4a84643e4f@redhat.com> <734D49CCEBEEF84792F5B80ED585239D5C39604F@SHSMSX104.ccr.corp.intel.com> <734D49CCEBEEF84792F5B80ED585239D5C399C9C@SHSMSX104.ccr.corp.intel.com> <734D49CCEBEEF84792F5B80ED585239D5C3E2179@SHSMSX104.ccr.corp.intel.com> <734D49CCEBEEF84792F5B80ED585239D5C3EFAF4@SHSMSX104.ccr.corp.intel.com> From: "Laszlo Ersek" Message-ID: Date: Fri, 10 Jan 2020 17:23:43 +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: <734D49CCEBEEF84792F5B80ED585239D5C3EFAF4@SHSMSX104.ccr.corp.intel.com> X-Scanned-By: MIMEDefang 2.79 on 10.5.11.14 X-MC-Unique: _qNcQ_dPP1mFOYlpkG6uCg-1 X-Mimecast-Spam-Score: 0 Content-Language: en-US Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable On 01/10/20 15:37, Ni, Ray wrote: > Ard, > I understand now that: BeforeConsole() needs to connect Gfx to get the GO= P device path > for ConOut updating. But the GOP driver is specified in Driver#### and it= will be loaded after > BeforeConsole() in today's code. This order makes the Gfx connection fail= s to start the GOP driver > resulting ConOut not updated. >=20 > Moving Driver#### processing before BeforeConsole() helps in your case. B= ut it may impact > other platforms: There might be platforms that update Driver#### variable= s in BeforeConsole() > and expect these drivers are processed in the same boot (not the next boo= t). I don't have the real > examples. But today's flow allows this. With your change, Driver#### will= not be processed in the first > boot. The change impacts these platforms. >=20 > One backward compatible approach can be: processing Driver#### before Bef= oreConsole() and processing the new Driver#### (added by BeforeConsole()) a= fter BeforeConsole(). >=20 > But another problem arises: If BeforeConsole() removes a Driver####, plat= form's expectation is that deleted Driver#### doesn't run. But that driver = already runs. >=20 > So actually the question is: when BDS core can consume the Driver#### and= process them? > Right now, it=E2=80=99s the time after BeforeConsole(). Just as right now= BeforeConsole() updates ConOut > in your case, BeforeConsole() is the place where platform updates all UEF= I defined boot related > variables. Processing Driver#### before BeforeConsole() requires platform= updates Driver#### > in other places. It's a new requirement to the platform. >=20 > With all what I explained in above, I cannot agree with the changes. >=20 > Another approach is: > Platform could connect the GFX in AfterConsole() and update the ConOut. T= hen platform could manually call EfiBootManagerConnectConsoleVariable (ConO= ut) to re-connect ConOut. > It's a bit ugly I agree. Let me raise three other ideas (alternatives to each other, and to the abov= e), with varying levels of annoyance. :) (1) Keep the following logic (i.e. the subject of this patch): // // Execute Driver Options // LoadOptions =3D EfiBootManagerGetLoadOptions (&LoadOptionCount, LoadOptio= nTypeDriver); ProcessLoadOptions (LoadOptions, LoadOptionCount); EfiBootManagerFreeLoadOptions (LoadOptions, LoadOptionCount); in *both* places, but gate each one with a bit in a new bitmask PCD. (Note: it's probably not the best for any platform to permit both branches,= as driver images would be loaded twice.) (2) EDKII_PLATFORM_BOOT_MANAGER_PROTOCOL has recently been added to edk2. I= t looks like a well-designed (extensible) protocol, for two reasons: - the protocol structure has a Revision field, - the only current member function, RefreshAllBootOptions(), is permitted t= o return EFI_UNSUPPORTED -- and the single call site, in the EfiBootManager= RefreshAllBootOption() function, handles that return value well (it does no= thing). The idea would be to bump the Revision field, and add a new member function= . Then call this member function (if it exists) in the spot where the curre= nt patch proposes to move the Driver#### dispatch logic to. This is almost like a new PlatformBootManagerLib interface, except it does = not require existent lib instances to be updated. And, on the EDKII_PLATFORM_BOOT_MANAGER_PROTOCOL implementation side, Refre= shAllBootOptions() would return EFI_UNSUPPORTED. (Because that is irrelevan= t to Ard's use case.) Perhaps add yet another member function that can disable the Driver#### opt= ion processing in the current location. (3) Extend the UEFI specification, section "3.1.3 Load Options". The LOAD_OPTION_CATEGORY bit-field is almost what we want, except it's spec= ified to be ignored for Driver#### options. So, - either invent the standardese to let us use LOAD_OPTION_CATEGORY for Driv= er#### options too, without breaking existing platforms, *or* - introduce a new (not too wide) distinct bitfield called LOAD_OPTION_DRIVE= R_CATEGORY. Whichever category bitfield proves acceptable, introduce a new "driver cate= gory bit" in that bitfield, called LOAD_OPTION_DRIVER_CATEGORY_CONSOLE. Specify that, if any Driver#### option has the LOAD_OPTION_DRIVER_CATEGORY_= CONSOLE attribute set, then Driver#### options will be processed in two pas= ses. In both passes, DriverOrder is to be honored, but the first pass will = only consider options with the attribute set, and the second pass will only= consider options with the attribute clear. Implement this in BdsEntry / UefiBootManagerLib. ... Maybe don't even introduce LOAD_OPTION_DRIVER_CATEGORY*; just a bitfiel= d that is expressly vendor specific? Thanks Laszlo