From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id CFAFF74003C for ; Wed, 21 Feb 2024 21:59:56 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=LWy5UKW6TlhnnztOfIQ3nLTgYvZE5ODP6OrJdRXjGt0=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1708552795; v=1; b=cZ+o0Rkqf92owextI3croAznruRPCz1lxD2im+VSCZqnTrvfjRnBvNI/DOAN8pUHYLrVR89q WPeqLNCsafGNXIehF89EBvzX5Xv3RI18CGcV0jHC4QjPx9T4+uiskx31AUTsq8KcAN59o3CTwMC jG4qubCFWX+zHolR16m6EfkU= X-Received: by 127.0.0.2 with SMTP id YXAqYY7687511x6cMI85O47c; Wed, 21 Feb 2024 13:59:55 -0800 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web10.1152.1708552794754847195 for ; Wed, 21 Feb 2024 13:59:55 -0800 X-Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-448-XAHpxkY_MT64F9HHaqFqQg-1; Wed, 21 Feb 2024 16:59:50 -0500 X-MC-Unique: XAHpxkY_MT64F9HHaqFqQg-1 X-Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id CE66B862CC2; Wed, 21 Feb 2024 21:59:49 +0000 (UTC) X-Received: from [10.39.192.46] (unknown [10.39.192.46]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 993CA8CED; Wed, 21 Feb 2024 21:59:48 +0000 (UTC) Message-ID: Date: Wed, 21 Feb 2024 22:59:47 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: Load Serial driver earlier in DXE To: devel@edk2.groups.io, alan.borzeszkowski@intel.com, "Kinney, Michael D" Cc: "Albecki, Mateusz" , "Gao, Zhichao" , "Ni, Ray" References: <20240220121045.2149320-1-alan.borzeszkowski@intel.com> <20240220121045.2149320-2-alan.borzeszkowski@intel.com> From: "Laszlo Ersek" In-Reply-To: X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.5 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: ohTlD72VcAYGCYzvgdU5YCqJx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=cZ+o0Rkq; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io On 2/21/24 18:15, Borzeszkowski, Alan wrote: >> It does not make sense to have a UEFI Driver active in early DXE because= it will not be connected yet and has dependencies on other UEFI drivers th= at will not be connected yet. >=20 > With suggested change, we connect to this driver successfully in early DX= E using ConnectController(). We did not observe any issues when Serial driv= er came online, debug messages are printed and console redirection works ju= st fine. Yes, and that ConnectController() call in early DXE -- wherever you are performing it -- is a driver model violation: one that works around the *other* driver model violation (the one Mike points out). :) >=20 >> Did you consider the use of the SerialPortLib for early DXE that can use= PCI serial devices with PcdSerialPciDeviceInfo that can be used for DEBUG(= ) messages. >=20 > That's the opposite of what we are trying to accomplish, this way additio= nal maintenance cost is required and on top of that, management of PCI devi= ce from library level is complicated (e.g., checking device state). Your approach here could be salvageable, but it must change the external interface of the driver. More precisely, you'd need a new INF file, and some extra C sources, for building the driver as a platform DXE driver -- one that installs the SerialIo protocol and the Device Path protocol on a brand new handle (or maybe on the image handle itself). At the same time, your platform DSC would have to: - remove the UEFI driver build of the driver, - dispatch the platform DXE driver build of the driver really early on, probably using the APRIORI DXE file. This is generally the model that the UEFI driver model was supposed to *supersede*. But, if you do consider the serial port a platform device, it is doable. Now, here's at least one more complication. The original driver accesses PCI config space via the PciIo protocol, from a quick grep -- that's correct, that's what a UEFI driver following the UEFI driver model should do. However, once you turn the SerialIo driver into a platform DXE driver, you cannot depend on PciIo anymore. ( And this actually goes on to show just how much of a layering violation your explicit ConnectController() call is: (1) Normally, platform firmware includes the PCI host bridge driver, which *is* a platform DXE driver, producing EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL instances in its entry point function. In edk2, this is "MdeModulePkg/Bus/Pci/PciHostBridgeDxe/PciHostBridgeDxe.inf", with platform-specific bits delegated to various library instances; such as PCI Config Space access delegated to PciSegmentLib, and root bridge enumeration delegated to PciHostBridgeLib. (2) PciBusDxe is a UEFI_DRIVER, binding EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL instances, and producing PciIo protocol instances. Importantly, the component that kicks off PciBusDxe -- i.e., the one that causes PciBusDxe to perform PCI enumeration and resource assignment -- is Platform BDS. Platform BDS *is* supposed to manually connect PciBusDxe (or, well, all possible drivers) to EFI_PCI_ROOT_BRIDGE_IO_PROTOCOL instances. Therefore, when you call ConnectController() in early DXE -- so that the serial port driver start to operate on top of PciIo instances --, you actually recursively set off that whole shebang *prematurely* -- PCI enumeration included. Full PCI enumeration should not really occur before BDS. (3) Note that UEFI drivers, unlike platform DXE drivers, have an implicit depex on *all* the architectural UEFI protocols. Such depexes are generally expected to be satisfiable by the time BDS is entered. Thus your workaround -- which causes UEFI drivers to bind devices way before BDS -- in fact depends on the relevant DXE drivers -- providing the arch protocols -- being dispatched "early enough", so that those UEFI drivers can even be dispatched. This is quite a mangling of how drivers are supposed to be dispatched by the DXE core. Anyway, digression ends; here's my larger point about PciIo: ) Rather than via PciIo protocol instances, you have to access PCI config space with one of the platform-matching PCI library classes / instances; for example, PciSegmentLib, PciLib, PciExpressLib, PciCf8Lib. Thus, you cannot depend on PCI enumeration in the first place, and you'd have to abstract away PCI config space access internally to the serial driver. In the UEFI driver build, those abstraction would boil down to PciIo member calls, while in the platform DXE driver build, to Pci*Lib calls. (You'll also have to manually compose a minimal [depex] section for the INF file, for those protocols -- provided by other platform DXE drivers, not UEFI drivers! -- that your serial port driver consumes.) Finally, the PCI config space aspect brings me to a broader platform design question. Using a *PCI* serial port for *debugging* is a terrible choice -- not a software choice, mind you, but a hardware one. That's precisely because PCI is so complex and high-level. For a simple stream of debug bytes, you want your *hardware* to include a super dumb, as-primitive-as-possible, *platform device*, like a *non-PCI* serial port. *Any hardware* should provide a platform debug device that is usable at least from PEI (before memory discovery), but preferably even from the Reset Vector / SEC. For UEFI console (terminal) purposes, a PCI based serial port is perfectly fine -- the UEFI console is not even defined as a *concept* before BDS. (DXE drivers are forbidden from trying to access the UEFI console, for example.) >=20 >> The other option is to map the PCI UART into Report Status Code. >=20 > Could you elaborate on that? (I'll let Mike do that; I'm not fully certain what he may have in mind.) >=20 > Also, could you please explain why DXE drivers cannot use Driver Binding? Because Driver Binding (the UEFI Driver Model) was designed to supersede / replace DXE drivers as comprehensively as possible (but not more than possible). The UEFI driver model takes care of protocol and driver stacking, recursive dependencies, on-demand (minimal, fast) binding (per platform BDS preferences), and so on. The UEFI Driver Writer's Guide explains this in minute detail; please find the link to your preferred format (HTML / PDF / ...) at: https://github.com/tianocore/tianocore.github.io/wiki/EDK-II-Documentation#= user-documentation Relevant sections: - "3.10 UEFI driver model" through "3.15 Platform initialization" - "5.3 Services that UEFI drivers should not use" (explains some parts of the "paradigm shift" from platform DXE drivers to UEFI drivers) - "6 UEFI Driver Categories". This is a tricky chapter: The first three sections (Device drivers, Bus drivers, Hybrid drivers) indeed describe UEFI drivers that follow the UEFI driver model. Then, while the last three chapters (service drivers, root bridge drivers, initializing drivers) *can* be taken to describe "UEFI drivers that do *not* follow the UEFI driver model"; they are much better interpreted as describing platform DXE drivers. - "7.2 UEFI Driver Model" - "7.8 Initializing Driver entry point" (cf. the last three sections of chapter 6) - "7.9 Service Driver entry point" (ditto) - "7.10 Root bridge driver entry point" (ditto) - "7.11 Runtime Drivers" (again, these are much better called DXE_RUNTIME_DRIVERs, not "UEFI Runtime Drivers"; I'm singling this section out because it talks about [depex]). Laszlo >=20 > Regards, > Alan >=20 >=20 >=20 > -----Original Message----- > From: Kinney, Michael D =20 > Sent: Tuesday, February 20, 2024 6:12 PM > To: devel@edk2.groups.io; Borzeszkowski, Alan > Cc: Albecki, Mateusz ; Gao, Zhichao ; Ni, Ray ; Kinney, Michael D > Subject: RE: [edk2-devel] [PATCH 1/1] MdeModulePkg: Load Serial driver ea= rlier in DXE >=20 > This is a UEFI Driver that depends on the Driver Binding Protocol and use= of ConnectController(). These drivers cannot be used until the BDS phase w= hen the active consoles and boot devices are evaluated and the smallest set= of drivers required to boot are connected. >=20 > It does not make sense to have a UEFI Driver active in early DXE because = it will not be connected yet and has dependencies on other UEFI drivers tha= t will not be connected yet. >=20 > Did you consider the use of the SerialPortLib for early DXE that can use = PCI serial devices with PcdSerialPciDeviceInfo that can be used for DEBUG()= messages. >=20 > The other option is to map the PCI UART into Report Status Code. >=20 > Best regards, >=20 > Mike >=20 >> -----Original Message----- >> From: devel@edk2.groups.io On Behalf Of=20 >> Borzeszkowski, Alan >> Sent: Tuesday, February 20, 2024 4:11 AM >> To: devel@edk2.groups.io >> Cc: Albecki, Mateusz ; Gao, Zhichao=20 >> ; Ni, Ray ; Borzeszkowski,=20 >> Alan >> Subject: [edk2-devel] [PATCH 1/1] MdeModulePkg: Load Serial driver=20 >> earlier in DXE >> >> For the purpose of UEFI debug prints enablement in DXE phase, Serial=20 >> driver should load earlier. Separate .inf file is created in order to=20 >> make minimal changes to current implementation. >> >> Signed-off-by: Alan Borzeszkowski >> --- >> .../PciSioSerialDxe/PciSioSerialDxeEarly.inf | 80=20 >> +++++++++++++++++++ >> 1 file changed, 80 insertions(+) >> create mode 100644 >> MdeModulePkg/Bus/Pci/PciSioSerialDxe/PciSioSerialDxeEarly.inf >> >> diff --git >> a/MdeModulePkg/Bus/Pci/PciSioSerialDxe/PciSioSerialDxeEarly.inf >> b/MdeModulePkg/Bus/Pci/PciSioSerialDxe/PciSioSerialDxeEarly.inf >> new file mode 100644 >> index 0000000000..2ead654898 >> --- /dev/null >> +++ b/MdeModulePkg/Bus/Pci/PciSioSerialDxe/PciSioSerialDxeEarly.inf >> @@ -0,0 +1,80 @@ >> +## @file >> +# Serial driver for standard UARTS on a SIO chip or PCI/PCIE card. >> +# >> +# Produces the Serial I/O protocol for standard UARTS using Super I/O >> or PCI I/O. >> +# This version is used shortly after DXE Core is invoked # #=20 >> +Copyright (c) 2007 - 2018, Intel Corporation. All rights >> reserved.
>> +# >> +# SPDX-License-Identifier: BSD-2-Clause-Patent # ## >> + >> +[Defines] >> + INF_VERSION =3D 0x00010005 >> + BASE_NAME =3D PciSioSerialDxeEarly >> + MODULE_UNI_FILE =3D PciSioSerialDxe.uni >> + FILE_GUID =3D 8BCC425E-585F-4E66-ADA5- >> FEA9A635F911 >> + MODULE_TYPE =3D DXE_DRIVER >> + VERSION_STRING =3D 1.0 >> + ENTRY_POINT =3D InitializePciSioSerial >> + >> +# >> +# The following information is for reference only and not required by >> the build tools. >> +# >> +# VALID_ARCHITECTURES =3D IA32 X64 EBC >> +# >> +# DRIVER_BINDING =3D gSerialControllerDriver >> +# COMPONENT_NAME =3D gPciSioSerialComponentName >> +# COMPONENT_NAME2 =3D gPciSioSerialComponentName2 >> +# >> + >> +[Sources] >> + ComponentName.c >> + SerialIo.c >> + SerialIoCommon.c >> + Serial.h >> + Serial.c >> + >> +[Packages] >> + MdePkg/MdePkg.dec >> + MdeModulePkg/MdeModulePkg.dec >> + >> +[LibraryClasses] >> + PcdLib >> + ReportStatusCodeLib >> + UefiBootServicesTableLib >> + MemoryAllocationLib >> + BaseMemoryLib >> + DevicePathLib >> + UefiLib >> + UefiDriverEntryPoint >> + DebugLib >> + IoLib >> + >> +[Guids] >> + gEfiUartDevicePathGuid ## SOMETIMES_CONSUMES >> ## GUID >> + >> +[Protocols] >> + gEfiSioProtocolGuid ## TO_START >> + gEfiDevicePathProtocolGuid ## TO_START >> + gEfiPciIoProtocolGuid ## TO_START >> + gEfiSerialIoProtocolGuid ## BY_START >> + gEfiDevicePathProtocolGuid ## BY_START >> + >> +[FeaturePcd] >> + gEfiMdeModulePkgTokenSpaceGuid.PcdSerialUseHalfHandshake|FALSE ## >> CONSUMES >> + >> +[Pcd] >> + gEfiMdePkgTokenSpaceGuid.PcdUartDefaultBaudRate|115200 ## >> CONSUMES >> + gEfiMdePkgTokenSpaceGuid.PcdUartDefaultDataBits|8 ## >> CONSUMES >> + gEfiMdePkgTokenSpaceGuid.PcdUartDefaultParity|1 ## >> CONSUMES >> + gEfiMdePkgTokenSpaceGuid.PcdUartDefaultStopBits|1 ## >> CONSUMES >> + gEfiMdeModulePkgTokenSpaceGuid.PcdSerialClockRate|1843200 ## >> CONSUMES >> + gEfiMdeModulePkgTokenSpaceGuid.PcdPciSerialParameters ## >> CONSUMES >> + >> +[UserExtensions.TianoCore."ExtraFiles"] >> + PciSioSerialDxeExtra.uni >> + >> +[Depex] >> + TRUE >> -- >> 2.34.1 >> >> --------------------------------------------------------------------- >> Intel Technology Poland sp. z o.o. >> ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII= =20 >> Wydzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP=20 >> 957- >> 07-52-316 | Kapital zakladowy 200.000 PLN. >> Spolka oswiadcza, ze posiada status duzego przedsiebiorcy w rozumieniu= =20 >> ustawy z dnia 8 marca 2013 r. o przeciwdzialaniu nadmiernym=20 >> opoznieniom w transakcjach handlowych. >> >> Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego=20 >> adresata i moze zawierac informacje poufne. W razie przypadkowego=20 >> otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale= =20 >> jej usuniecie; jakiekolwiek przegladanie lub rozpowszechnianie jest=20 >> zabronione. >> This e-mail and any attachments may contain confidential material for=20 >> the sole use of the intended recipient(s). If you are not the intended= =20 >> recipient, please contact the sender and delete all copies; any review= =20 >> or distribution by others is strictly prohibited. >> >> >> >> >> >=20 > --------------------------------------------------------------------- > Intel Technology Poland sp. z o.o. > ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wy= dzial Gospodarczy Krajowego Rejestru Sadowego - KRS 101882 | NIP 957-07-52-= 316 | Kapital zakladowy 200.000 PLN. > Spolka oswiadcza, ze posiada status duzego przedsiebiorcy w rozumieniu us= tawy z dnia 8 marca 2013 r. o przeciwdzialaniu nadmiernym opoznieniom w tra= nsakcjach handlowych. >=20 > Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresa= ta i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej = wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jaki= ekolwiek przegladanie lub rozpowszechnianie jest zabronione. > This e-mail and any attachments may contain confidential material for the= sole use of the intended recipient(s). If you are not the intended recipie= nt, please contact the sender and delete all copies; any review or distribu= tion by others is strictly prohibited. >=20 >=20 >=20 >=20 >=20 >=20 -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#115744): https://edk2.groups.io/g/devel/message/115744 Mute This Topic: https://groups.io/mt/104469297/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-