Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: Load Serial driver earlier in DXE

["Laszlo Ersek" <lersek@redhat.com>]

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 that will not be connected yet.
> 
> With suggested change, we connect to this driver successfully in early DXE using ConnectController(). We did not observe any issues when Serial driver came online, debug messages are printed and console redirection works just 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). :)

> 
>> 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.
> 
> That's the opposite of what we are trying to accomplish, this way additional maintenance cost is required and on top of that, management of PCI device 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.)

> 
>> The other option is to map the PCI UART into Report Status Code.
> 
> Could you elaborate on that?

(I'll let Mike do that; I'm not fully certain what he may have in mind.)

> 
> 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

> 
> Regards,
> Alan
> 
> 
> 
> -----Original Message-----
> From: Kinney, Michael D <michael.d.kinney@intel.com> 
> Sent: Tuesday, February 20, 2024 6:12 PM
> To: devel@edk2.groups.io; Borzeszkowski, Alan <alan.borzeszkowski@intel.com>
> Cc: Albecki, Mateusz <mateusz.albecki@intel.com>; Gao, Zhichao <zhichao.gao@intel.com>; Ni, Ray <ray.ni@intel.com>; Kinney, Michael D <michael.d.kinney@intel.com>
> Subject: RE: [edk2-devel] [PATCH 1/1] MdeModulePkg: Load Serial driver earlier in DXE
> 
> 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 when the active consoles and boot devices are evaluated and the smallest set of drivers required to boot are connected.
> 
> 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 that will not be connected yet.
> 
> 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.
> 
> The other option is to map the PCI UART into Report Status Code.
> 
> Best regards,
> 
> Mike
> 
>> -----Original Message-----
>> From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of 
>> Borzeszkowski, Alan
>> Sent: Tuesday, February 20, 2024 4:11 AM
>> To: devel@edk2.groups.io
>> Cc: Albecki, Mateusz <mateusz.albecki@intel.com>; Gao, Zhichao 
>> <zhichao.gao@intel.com>; Ni, Ray <ray.ni@intel.com>; Borzeszkowski, 
>> Alan <alan.borzeszkowski@intel.com>
>> Subject: [edk2-devel] [PATCH 1/1] MdeModulePkg: Load Serial driver 
>> earlier in DXE
>>
>> For the purpose of UEFI debug prints enablement in DXE phase, Serial 
>> driver should load earlier. Separate .inf file is created in order to 
>> make minimal changes to current implementation.
>>
>> Signed-off-by: Alan Borzeszkowski <alan.borzeszkowski@intel.com>
>> ---
>>  .../PciSioSerialDxe/PciSioSerialDxeEarly.inf  | 80 
>> +++++++++++++++++++
>>  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 # # 
>> +Copyright (c) 2007 - 2018, Intel Corporation. All rights
>> reserved.<BR>
>> +#
>> +# SPDX-License-Identifier: BSD-2-Clause-Patent # ##
>> +
>> +[Defines]
>> +  INF_VERSION                    = 0x00010005
>> +  BASE_NAME                      = PciSioSerialDxeEarly
>> +  MODULE_UNI_FILE                = PciSioSerialDxe.uni
>> +  FILE_GUID                      = 8BCC425E-585F-4E66-ADA5-
>> FEA9A635F911
>> +  MODULE_TYPE                    = DXE_DRIVER
>> +  VERSION_STRING                 = 1.0
>> +  ENTRY_POINT                    = InitializePciSioSerial
>> +
>> +#
>> +# The following information is for reference only and not required by
>> the build tools.
>> +#
>> +#  VALID_ARCHITECTURES           = IA32 X64 EBC
>> +#
>> +#  DRIVER_BINDING                =  gSerialControllerDriver
>> +#  COMPONENT_NAME                =  gPciSioSerialComponentName
>> +#  COMPONENT_NAME2               =  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 
>> Wydzial 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 
>> ustawy z dnia 8 marca 2013 r. o przeciwdzialaniu nadmiernym 
>> opoznieniom w transakcjach handlowych.
>>
>> Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego 
>> adresata i moze zawierac informacje poufne. W razie przypadkowego 
>> otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale 
>> jej usuniecie; jakiekolwiek 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 
>> recipient, please contact the sender and delete all copies; any review 
>> or distribution by others is strictly prohibited.
>>
>>
>>
>>
>>
> 
> ---------------------------------------------------------------------
> Intel Technology Poland sp. z o.o.
> ul. Slowackiego 173 | 80-298 Gdansk | Sad Rejonowy Gdansk Polnoc | VII Wydzial 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 ustawy z dnia 8 marca 2013 r. o przeciwdzialaniu nadmiernym opoznieniom w transakcjach handlowych.
> 
> Ta wiadomosc wraz z zalacznikami jest przeznaczona dla okreslonego adresata i moze zawierac informacje poufne. W razie przypadkowego otrzymania tej wiadomosci, prosimy o powiadomienie nadawcy oraz trwale jej usuniecie; jakiekolwiek 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 recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.
> 
> 
> 
> 
> 
> 



-=-=-=-=-=-=-=-=-=-=-=-
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]
-=-=-=-=-=-=-=-=-=-=-=-