From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received-SPF: Pass (sender SPF authorized) identity=mailfrom; client-ip=66.187.233.73; helo=mx1.redhat.com; envelope-from=lersek@redhat.com; receiver=edk2-devel@lists.01.org Received: from mx1.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id 33490211A70FD for ; Mon, 11 Jun 2018 09:58:09 -0700 (PDT) Received: from smtp.corp.redhat.com (int-mx04.intmail.prod.int.rdu2.redhat.com [10.11.54.4]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id E3AA3401EF00 for ; Mon, 11 Jun 2018 16:58:08 +0000 (UTC) Received: from lacos-laptop-7.usersys.redhat.com (ovpn-120-125.rdu2.redhat.com [10.10.120.125]) by smtp.corp.redhat.com (Postfix) with ESMTP id 4F6822024CA1; Mon, 11 Jun 2018 16:58:08 +0000 (UTC) From: Laszlo Ersek To: Gerd Hoffmann , edk2-devel@lists.01.org References: <20180608113942.17009-1-kraxel@redhat.com> <20180608113942.17009-4-kraxel@redhat.com> Message-ID: <6e06884c-6f4f-446b-25c8-6524dea8290d@redhat.com> Date: Mon, 11 Jun 2018 18:58:07 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.7.0 MIME-Version: 1.0 In-Reply-To: X-Scanned-By: MIMEDefang 2.78 on 10.11.54.4 X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.5]); Mon, 11 Jun 2018 16:58:08 +0000 (UTC) X-Greylist: inspected by milter-greylist-4.5.16 (mx1.redhat.com [10.11.55.5]); Mon, 11 Jun 2018 16:58:08 +0000 (UTC) for IP:'10.11.54.4' DOMAIN:'int-mx04.intmail.prod.int.rdu2.redhat.com' HELO:'smtp.corp.redhat.com' FROM:'lersek@redhat.com' RCPT:'' Subject: Re: [PATCH 3/4] OvmfPkg: add QemuRamfb to platform console X-BeenThere: edk2-devel@lists.01.org X-Mailman-Version: 2.1.26 Precedence: list List-Id: EDK II Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 11 Jun 2018 16:58:10 -0000 Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 06/11/18 18:24, Laszlo Ersek wrote: > On 06/08/18 13:39, Gerd Hoffmann wrote: >> Add QemuRamfbDxe device path to the list of platform console devices, >> so ConSplitter will pick up the device even though it isn't a PCI GPU. > > This explanation is not wrong, but I'll list more details, just in case. > > By adding the devpath to "gPlatformConsole" with CONSOLE_OUT, > technically we push the devpath into the ConOut UEFI variable, as > follows: > > BdsEntry() [MdeModulePkg/Universal/BdsDxe/BdsEntry.c] > PlatformBootManagerBeforeConsole() [OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c] > PlatformInitializeConsole() [OvmfPkg/Library/PlatformBootManagerLib/BdsPlatform.c] > EfiBootManagerUpdateConsoleVariable() [MdeModulePkg/Library/UefiBootManagerLib/BmConsole.c] > > After this, BdsEntry goes on to actually connect the device (i.e., it > makes the QemuRamfbDxe driver bind the device): > > BdsEntry() [MdeModulePkg/Universal/BdsDxe/BdsEntry.c] > EfiBootManagerConnectAllDefaultConsoles() [MdeModulePkg/Library/UefiBootManagerLib/BmConsole.c] > > Binding drivers to keyboard, serial and graphics devices, so that text > input, text output, and graphics output protocols are produced, isn't > per se sufficient for ConSplitterDxe to multiplex to/from those > protocols. For that, ConPlatformDxe needs to "tag" the handles with > EfiConsole(In|Out)DeviceGuid|EfiStandardErrorDeviceGuid -- > ConSplitterDxe depends on those protocols. It's ConPlatformDxe that > needs the devpath to be in ConIn/ConOut/ErrOut, for the "tagging" to > occur. > > In total, we add the ramfb devpath to "gPlatformConsole" so that our > PlatformInitializeConsole() function puts it in ConOut, despite ramfb > not being a PCI display device. Binding the device to QemuRamFbDxe, and > multiplexing from/to it happen "automatically" from there, thanks to > BdsDxe, and ConPlatformDxe/ConSplitterDxe respectively. Whoops, managed to confuse myself a little here; some correction should be in order: The ramfb driver does not follow the UEFI driver model. The GOP is produced right in the entry point of the driver, not when platform BDS connects the device. It remains true however that ConPlatformDxe / ConSplitterDxe, which do follow the UEFI driver model, bind the GOP in EfiBootManagerConnectAllDefaultConsoles(). I guess I would re-formulate, """ In total, we add the ramfb devpath to "gPlatformConsole" so that our PlatformInitializeConsole() function puts it in ConOut, despite ramfb not being a PCI display device. Multiplexing from/to the ramfb GOP happens "automatically" from there, thanks to ConPlatformDxe/ConSplitterDxe. """ Which is basically what the current commit message says. :) Sorry for any confusion caused! Laszlo > > (1) I don't mind if you stick with the current commit message; I just > wanted to provide more details for this discussion. > > More comments below: > >> >> Contributed-under: TianoCore Contribution Agreement 1.1 >> Signed-off-by: Gerd Hoffmann >> --- >> .../Library/PlatformBootManagerLib/PlatformData.c | 44 ++++++++++++++++++++++ >> 1 file changed, 44 insertions(+) >> >> diff --git a/OvmfPkg/Library/PlatformBootManagerLib/PlatformData.c b/OvmfPkg/Library/PlatformBootManagerLib/PlatformData.c >> index a50cd7bcaf..6202516971 100644 >> --- a/OvmfPkg/Library/PlatformBootManagerLib/PlatformData.c >> +++ b/OvmfPkg/Library/PlatformBootManagerLib/PlatformData.c >> @@ -14,6 +14,7 @@ >> **/ >> >> #include "BdsPlatform.h" >> +#include >> >> // >> // Debug Agent UART Device Path structure >> @@ -37,6 +38,17 @@ typedef struct { >> } USB_KEYBOARD_DEVICE_PATH; >> #pragma pack () >> >> +// >> +// QemuRamfb Device Path structure >> +// >> +#pragma pack(1) >> +typedef struct { >> + VENDOR_DEVICE_PATH Vendor; >> + ACPI_ADR_DEVICE_PATH AcpiAdr; >> + EFI_DEVICE_PATH_PROTOCOL End; >> +} VENDOR_RAMFB_DEVICE_PATH; >> +#pragma pack() > > (2) Please add a space character between each "pack" and "(". > >> + >> ACPI_HID_DEVICE_PATH gPnpPs2KeyboardDeviceNode = gPnpPs2Keyboard; >> ACPI_HID_DEVICE_PATH gPnp16550ComPortDeviceNode = gPnp16550ComPort; >> UART_DEVICE_PATH gUartDeviceNode = gUart; >> @@ -100,6 +112,34 @@ STATIC USB_KEYBOARD_DEVICE_PATH gUsbKeyboardDevicePath = { >> gEndEntire >> }; >> >> +STATIC VENDOR_RAMFB_DEVICE_PATH gQemuRamfbDevicePath = { >> + { >> + { >> + HARDWARE_DEVICE_PATH, >> + HW_VENDOR_DP, >> + { >> + (UINT8) (sizeof (VENDOR_DEVICE_PATH)), >> + (UINT8) ((sizeof (VENDOR_DEVICE_PATH)) >> 8) >> + } >> + }, >> + QEMU_RAMFB_GUID, >> + }, >> + { >> + { >> + ACPI_DEVICE_PATH, >> + ACPI_ADR_DP, >> + { >> + (UINT8) (sizeof (ACPI_ADR_DEVICE_PATH)), >> + (UINT8) ((sizeof (ACPI_ADR_DEVICE_PATH)) >> 8) >> + } >> + }, >> + ACPI_DISPLAY_ADR (1, 0, 0, 1, 0, >> + ACPI_ADR_DISPLAY_TYPE_EXTERNAL_DIGITAL, >> + 0, 0) > > (3) Urgh. On one hand, I dislike this dump of constants; on the other > hand, I don't want to ask you to repeat all the parameter comments that > I asked for in the previous patch. > > OK, let's stick with the dump of constants, just make sure the > indentation is idiomatic. > >> + }, >> + gEndEntire >> +}; >> + >> // >> // Predefined platform default console device path >> // >> @@ -113,6 +153,10 @@ PLATFORM_CONSOLE_CONNECT_ENTRY gPlatformConsole[] = { >> CONSOLE_IN >> }, >> { >> + (EFI_DEVICE_PATH_PROTOCOL *)&gQemuRamfbDevicePath, >> + CONSOLE_OUT >> + }, > > Right, this is consistent with PreparePciDisplayDevicePath(). > > Thanks, > Laszlo > >> + { >> NULL, >> 0 >> } >> >