[edk2-devel] [PATCH 0/1] EDK2 Serial driver UART debug print enablement

[edk2-devel] [PATCH 0/1] EDK2 Serial driver UART debug print enablement

[Borzeszkowski, Alan]

On Intel platforms, we use LPSS UART for debug prints in DXE phase. Current implementation involves using custom driver.
In order to reduce code maintenance cost and flash usage, we want to switch to EDK2 Serial driver.
To achieve that, we need to load Serial driver shortly after DXE Core is invoked. Otherwise, this driver
will load at BDS which omits the purpose of debug prints in DXE. Separate .inf file is created
to introduce minimal changes to current implementation.
Change was tested on Intel platform, debug prints appeared shortly after DXE phase begun and console
redirection works.
https://github.com/tianocore/edk2/pull/5386

Alan Borzeszkowski (1):
  MdeModulePkg: Load Serial driver earlier in DXE

 .../PciSioSerialDxe/PciSioSerialDxeEarly.inf  | 80 +++++++++++++++++++
 1 file changed, 80 insertions(+)
 create mode 100644 MdeModulePkg/Bus/Pci/PciSioSerialDxe/PciSioSerialDxeEarly.inf

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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115658): https://edk2.groups.io/g/devel/message/115658
Mute This Topic: https://groups.io/mt/104469290/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Borzeszkowski, Alan]

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.



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115659): https://edk2.groups.io/g/devel/message/115659
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Michael D Kinney]

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



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115663): https://edk2.groups.io/g/devel/message/115663
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Borzeszkowski, Alan]

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

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

> The other option is to map the PCI UART into Report Status Code.

Could you elaborate on that?

Also, could you please explain why DXE drivers cannot use Driver Binding?

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 (#115731): https://edk2.groups.io/g/devel/message/115731
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Michael D Kinney]

DXE env is not UEFI conformant.  UEFI Drivers can not be executed
until the UEFI env is fully established which is at end of DXE
after all DXE Arch Protocols are produced and DXE Core supports the
full set of requires UEFI services. Running a UEFI Driver or UEFI
Application before all DXE Arch Protocols are produced has many 
risks. 

What do you mean by "UEFI Debug Prints" in the patch?  What service 
is being used to print and what components in DXE Phase before BDS
are using a UEFI print service?

I may have incorrectly assumed that "UEFI Debug Print" was the 
use of DEBUG() macro.

Mike

> -----Original Message-----
> From: Borzeszkowski, Alan <alan.borzeszkowski@intel.com>
> Sent: Wednesday, February 21, 2024 9:16 AM
> To: Kinney, Michael D <michael.d.kinney@intel.com>;
> devel@edk2.groups.io
> Cc: Albecki, Mateusz <mateusz.albecki@intel.com>; Gao, Zhichao
> <zhichao.gao@intel.com>; Ni, Ray <ray.ni@intel.com>
> Subject: RE: [edk2-devel] [PATCH 1/1] MdeModulePkg: Load Serial driver
> earlier in DXE
> 
> > 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.
> 
> > 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).
> 
> > The other option is to map the PCI UART into Report Status Code.
> 
> Could you elaborate on that?
> 
> Also, could you please explain why DXE drivers cannot use Driver
> Binding?
> 
> 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.
> >
> >
> >
> > 
> >



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115742): https://edk2.groups.io/g/devel/message/115742
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Laszlo Ersek]

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

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

[Albecki, Mateusz]

[-- Attachment #1.1: Type: text/plain, Size: 4525 bytes --]

Hi,

I was originally responsible for suggesting this change internally(more specifically - to switch from Intel specific LPSS UART driver to EDK2 driver which on inspection seemed to cover all relevant modes of operations of LPSS UART).

First I would like to explain how exactly we are using this driver when LPSS UART is used for debug messages:

1. LPSS UART is a PCI device(although it can be put into hidden mode where standard PCI enumerator doesn't see it, this is the mode we use for debug) integrated in a chipset.
2. PciSioSerialDxe driver will be placed in apriori section of the DXE FW to ensure it is loaded early the same goes for a special platform specific driver.
3. During EarlyDxe platform specific driver setup LPSS UART and install PCI_IO_PROTOCOL and DEVICE_PATH_PROTOCOL instance for LPSS UART. Setup includes assigning MMIO, enabling memory path to LPSS UART etc.
4. Next platform specific module will call connect controller on a handle with installed PCI_IO_PROTOCOL and DEVICE_PATH_PROTOCOL. Note that it will only connect LPSS UART(and any other critical device). It will not connect entire system. In fact connecting entire system is not possible as majority of other PCI drivers(including PciBus) are not loaded at this point.
5. PciSioSerialDxe performs normal binding flow at this point and produces SERIAL_IO_PROTOCOL
6. When other modules call DEBUG() macro our DebugLib will try to locate that SERIAL_IO_PROTOCOL and if located print over it. NOTE: there is no additional depex introduced to modules that use debug lib. If protocol is not present debug will simply not happen.

Next I want to go broadly over concerns raised in this thread:

*1. UEFI driver model violation*

To be honest this is the first time I am hearing that DXE_DRIVER shouldn't or can't install EFI_DRIVER_BINDING_PROTOCOL or use PCI_IO_PROTOCOL. I checked in PI specification to see whether it places any restrictions on DXE_DRIVERS and the protocols it can consume/produce and it seems like it doesn't. In fact it explicitly states that DXE drivers can be UEFI driver model compliant.

from: https://uefi.org/specs/PI/1.8/V2_DXE_Drivers.html#dxe-drivers

As for UEFI driver writes guide I really don't see any restrictions placed on DXE drivers in this spec, it seems to be mostly focused on describing types of UEFI drivers and their typical behavior.

*2. Rewrite the change so that it doesn't use PCI_IO_PROTOCOL and driver binding*

While possible I think this is a really bad idea that doesn't provide much value and only introduces additional interfaces for edk2 community to maintain.

- Replacing PCI_IO_PROTOCOL with PciSegment and friends would still require to pass information such as BDF, MMIO and mechanism to enable memory decode. So we still need some protocol interface. We could potentially drop PCI_IO entirely and instead say if you are in early DXE use EFI_SIO_PROTOCOL, but that one is less robust than PCI_IO. Note that in our implementation we do not require full PCI enumeration since LPSS UART is placed outside of the main PCI tree(situation is somewhat similar to IncompatiblePciDeviceSupport.c in EDK2).

- Rewriting driver binding introduces additional cost without any real benefit I believe. Having driver binding allows the platform to call ConnectController when it is finished initializing enough of the HW so that UART can work. Dropping driver binding and instead doing everything in entry point would require depending on driver dispatch order or on some additional protocol which would be placed in depex section of PciSioSerialDxe and installed by platform driver. It would also complicate the way in which we would support multiple UART devices which can be ready to operate on different stages in boot.

- Other drawbacks - if we have completely separate entry point for DXE driver we need to keep 2 EFI modules in flash - 1 DXE only UART driver and 2nd UEFI only UART driver. As it is now we can simply keep the DXE_DRIVER instance in flash and it supports both early UARTS that are used for debug and late UARTS that are used for console redirection.

Thanks,
Mateusz


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115887): https://edk2.groups.io/g/devel/message/115887
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]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #1.2: Type: text/html, Size: 5316 bytes --]

[-- Attachment #2: dummyfile.0.part --]
[-- Type: image/png, Size: 74976 bytes --]

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

[Michael D Kinney]

[-- Attachment #1.1: Type: text/plain, Size: 5376 bytes --]

Since this is not a standard PCI visible to PCI Bus Driver, did you consider using MdeModulePkg\Universal\SerialDxe that
produces a Serial I/O Protocol from a SerialPortLib instance?  That was SerialPortLib can be used for DEBUG() logging
and the Serial I/O Protocol produced by SerialDxe can be used for console.

This is a widely used solution in edk2 and edk2-platforms repos.

No new interfaces.  The one SerialPortLib instance can be used in PEI/DXE/SMM for DEBUG() logging and SerialDxe provides Serial I/O that along with TerminalDxe and ConSplitter(optional) provides UEFI Console services.

Mike


From: devel@edk2.groups.io <devel@edk2.groups.io> On Behalf Of Albecki, Mateusz
Sent: Friday, February 23, 2024 7:50 AM
To: Laszlo Ersek <lersek@redhat.com>; devel@edk2.groups.io
Subject: Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: Load Serial driver earlier in DXE

Hi,

I was originally responsible for suggesting this change internally(more specifically - to switch from Intel specific LPSS UART driver to EDK2 driver which on inspection seemed to cover all relevant modes of operations of LPSS UART).

First I would like to explain how exactly we are using this driver when LPSS UART is used for debug messages:

1. LPSS UART is a PCI device(although it can be put into hidden mode where standard PCI enumerator doesn't see it, this is the mode we use for debug) integrated in a chipset.
2. PciSioSerialDxe driver will be placed in apriori section of the DXE FW to ensure it is loaded early the same goes for a special platform specific driver.
3. During EarlyDxe platform specific driver setup LPSS UART and install PCI_IO_PROTOCOL and DEVICE_PATH_PROTOCOL instance for LPSS UART. Setup includes assigning MMIO, enabling memory path to LPSS UART etc.
4. Next platform specific module will call connect controller on a handle with installed PCI_IO_PROTOCOL and DEVICE_PATH_PROTOCOL. Note that it will only connect LPSS UART(and any other critical device). It will not connect entire system. In fact connecting entire system is not possible as majority of other PCI drivers(including PciBus) are not loaded at this point.
5. PciSioSerialDxe performs normal binding flow at this point and produces SERIAL_IO_PROTOCOL
6. When other modules call DEBUG() macro our DebugLib will try to locate that SERIAL_IO_PROTOCOL and if located print over it. NOTE: there is no additional depex introduced to modules that use debug lib. If protocol is not present debug will simply not happen.

Next I want to go broadly over concerns raised in this thread:

1. UEFI driver model violation

To be honest this is the first time I am hearing that DXE_DRIVER shouldn't or can't install EFI_DRIVER_BINDING_PROTOCOL or use PCI_IO_PROTOCOL. I checked in PI specification to see whether it places any restrictions on DXE_DRIVERS and the protocols it can consume/produce and it seems like it doesn't. In fact it explicitly states that DXE drivers can be UEFI driver model compliant.
[cid:image001.png@01DA6661.429B3620]
from:

As for UEFI driver writes guide I really don't see any restrictions placed on DXE drivers in this spec, it seems to be mostly focused on describing types of UEFI drivers and their typical behavior.

2. Rewrite the change so that it doesn't use PCI_IO_PROTOCOL and driver binding

While possible I think this is a really bad idea that doesn't provide much value and only introduces additional interfaces for edk2 community to maintain.

- Replacing PCI_IO_PROTOCOL with PciSegment and friends would still require to pass information such as BDF, MMIO and mechanism to enable memory decode. So we still need some protocol interface. We could potentially drop PCI_IO entirely and instead say if you are in early DXE use EFI_SIO_PROTOCOL, but that one is less robust than PCI_IO. Note that in our implementation we do not require full PCI enumeration since LPSS UART is placed outside of the main PCI tree(situation is somewhat similar to IncompatiblePciDeviceSupport.c in EDK2).

- Rewriting driver binding introduces additional cost without any real benefit I believe. Having driver binding allows the platform to call ConnectController when it is finished initializing enough of the HW so that UART can work. Dropping driver binding and instead doing everything in entry point would require depending on driver dispatch order or on some additional protocol which would be placed in depex section of PciSioSerialDxe and installed by platform driver. It would also complicate the way in which we would support multiple UART devices which can be ready to operate on different stages in boot.

- Other drawbacks - if we have completely separate entry point for DXE driver we need to keep 2 EFI modules in flash - 1 DXE only UART driver and 2nd UEFI only UART driver. As it is now we can simply keep the DXE_DRIVER instance in flash and it supports both early UARTS that are used for debug and late UARTS that are used for console redirection.

Thanks,
Mateusz



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115895): https://edk2.groups.io/g/devel/message/115895
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]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #1.2: Type: text/html, Size: 8637 bytes --]

[-- Attachment #2: image001.png --]
[-- Type: image/png, Size: 74976 bytes --]

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

[Laszlo Ersek]

On 2/23/24 16:50, Albecki, Mateusz wrote:
> Hi,
>
> I was originally responsible for suggesting this change
> internally(more specifically - to switch from Intel specific LPSS UART
> driver to EDK2 driver which on inspection seemed to cover all relevant
> modes of operations of LPSS UART).
>
> First I would like to explain how exactly we are using this driver
> when LPSS UART is used for debug messages:
>
> 1. LPSS UART is a PCI device(although it can be put into hidden mode
> where standard PCI enumerator doesn't see it, this is the mode we use
> for debug) integrated in a chipset.
> 2. PciSioSerialDxe driver will be placed in apriori section of the DXE
> FW to ensure it is loaded early the same goes for a special platform
> specific driver.
> 3. During EarlyDxe platform specific driver setup LPSS UART and
> install PCI_IO_PROTOCOL and DEVICE_PATH_PROTOCOL instance for LPSS
> UART. Setup includes assigning MMIO, enabling memory path to LPSS UART
> etc.

Seems unusual, but I couldn't claim anything is technically wrong up to
this point, given that the hardware is hidden from PciBusDxe
(enumeration).

All the above steps are just pure platform DXE stuff.

[

... Haha, no, not at all. I'm actually jumping back here after I've
finished most of my email (see below). Before sending it off, I wanted
to re-read the list of steps. That allowed me to find the actual issue
(IMO). I'll express it at the bottom.

]

> 4. Next platform specific module will call connect controller on a
> handle with installed PCI_IO_PROTOCOL and DEVICE_PATH_PROTOCOL. Note
> that it will only connect LPSS UART(and any other critical device). It
> will not connect entire system. In fact connecting entire system is
> not possible as majority of other PCI drivers(including PciBus) are
> not loaded at this point.

Understood; still wrong, in my opinion. ConnectController will
inherently want a driver binding protocol instance from a driver that
follows the UEFI driver model, and attempting to rely on such driver at
this point is unclean.

In the first place, I'd expect any driver exposing driver binding to be
a UEFI driver, meaning it would have a depex on all the architectural
protocols -- so it shouldn't even be able to be dispatched at this
point.

I guess *technically* nothing prevents you from installing a driver
binding protocol instance from a platform DXE driver; it's just a
violation of the spirit of the UEFI spec, IMO.

> 5. PciSioSerialDxe performs normal binding flow at this point and
> produces SERIAL_IO_PROTOCOL

Aha, so the driver in question is PciSioSerialDxe, which is indeed a
regular UEFI_DRIVER (following the UEFI driver model).

But then the key question is *when* your step (4) happens. If that
platform specific module calls ConnectController before PciSioSerialDxe
can be dispatched, then nothing useful will happen. So how do you ensure
that step (4) happens late enough?

(Normally, "late enough" would mean "in BDS").

> 6. When other modules call DEBUG() macro our DebugLib will try to
> locate that SERIAL_IO_PROTOCOL and if located print over it. NOTE:
> there is no additional depex introduced to modules that use debug lib.
> If protocol is not present debug will simply not happen.

And here the question is how you ensure that any particular module you
care about *actually manage* to locate SERIAL_IO_PROTOCOL, in time for
your debugging purposes. So, I have to augment my above question:

How do you ensure that step (4) happens *both* early enough *and* late
enough?

You must do step (4) *late enough* for PciSioSerialDxe to have a
fleeting chance to be dispatched by the DXE dispatcher (due to arch
protocol deps), and *early enough* for your key modules to find
SERIAL_IO_PROTOCOL (from PciSioSerialDxe) via DebugLib, in order to
produce actual debug messages.

All in all this seems to lay out a dependency chain where you first need
to have all the arch protocols installed in the protocol database,
before you can get usable debug output.

Assuming you want to produce DEBUGs from platform DXE drivers -- i.e.,
drivers that don't themselves depend on the conjunction of all the arch
protocols -- that may load arbitrarily early, how do you satisfy that?

>
> Next I want to go broadly over concerns raised in this thread:
>
> *1. UEFI driver model violation*
>
> To be honest this is the first time I am hearing that DXE_DRIVER
> shouldn't or can't install EFI_DRIVER_BINDING_PROTOCOL or use
> PCI_IO_PROTOCOL. I checked in PI specification to see whether it
> places any restrictions on DXE_DRIVERS and the protocols it can
> consume/produce and it seems like it doesn't. In fact it explicitly
> states that DXE drivers can be UEFI driver model compliant.

That's just a naming perturbation. Neither the driver writer's guide nor
the spec you quote are consistent about "UEFI driver" vs "DXE driver".

At the base, they're both just EFI_FV_FILETYPE_DRIVER modules. There are
no separate file types for DXE Driver vs. UEFI Driver; if you check e.g.
a build report, both will get

Driver Type:          0x7 (DRIVER)

The differences lie between their behaviors.

- Platform DXE drivers have explicit DEPEXes on protocols they need to
launch, install protocols they produce immediately in their driver entry
point functions, don't install driver binding, and in case they want to
"late-bind" protocols (as those protocol instances appear), they use
RegisterProtocolNotify.

- UEFI drivers that follow the UEFI driver model have implicitly
generated DEPEXes on the conjunction of all the arch protocols, and
don't do much beyond installing component name and driver binding
protocols in their entry point functions. They don't use functionality
like RegisterProtocolNotify; their main entry points are the driver
binding members.

The part you quote from PI, "DXE Drivers that follow the UEFI driver
model" is just what we mostly call "UEFI drivers that follow the UEFI
driver model". And where the driver writers' guide speaks about
"initializing UEFI Drivers" or "service UEFI drivers", those are more
correctly called "initializing DXE drivers" or "service DXE drivers".

A key quote here can be "5.3.6 RegisterProtocolNotify()" from the DWG:

5.3   Services that UEFI drivers should not use
5.3.6 RegisterProtocolNotify()

    This service registers an event that is to be signaled whenever an
    interface is installed for a specified protocol. Using this service
    is strongly discouraged. This service was previously used by EFI
    drivers that follow the EFI 1.02 Specification, and it provided a
    simple mechanism for drivers to layer on top of another driver. The
    EFI 1.10 Specification introduced the EFI Driver Model, and is still
    supported in the current versions of the UEFI Specification. The
    UEFI Driver Model provides a more flexible mechanism for a driver to
    layer on top of another driver that eliminated the need for
    RegisterProtocolNotify(). The RegisterProtocolNotify() service is
    still supported for compatibility with previous versions of the EFI
    Specification.

So anyway, the sections you cite from PI,
- II-11.2.1 Early DXE Drivers
- II-11.2.2 DXE Drivers that Follow the UEFI Driver Model

is exactly the same dichotomy that I'm talking about, just the
nomenclature differs.

It's your step (4) that doesn't fit into this model; as you quote, the
PI spec writes under II-11.2.2:

    The set of Driver Binding Protocols are used by the Boot Device
    Selection (BDS) phase to connect the drivers to the devices that are
    required to establish consoles and provide access to boot devices.

That does not hold for your explicit ConnectController call, which
presumably occurs before BDS (because otherwise, your DebugLib instance,
linked into platform DXE drivers, could not be useful).

Further,

    DXE drivers with empty dependency expressions will not be dispatched
    by the DXE Dispatcher until all of the DXE Architectural Protocols
    have been installed.

which only seems to confirm that PciSioSerialDxe cannot be launched [*]
before all arch protocols are available -- meaning that
SERIAL_IO_PROTOCOL is not available for a large part of the DXE phase.

(

[*] BTW, when I wrote "implicitly *generated* DEPEX" above, that was
intentional; I didn't mean an empty DEPEX, but a DEPEX that explicitly
requires the conjunction of all the arch protocols. It's just a minor
detail in this discussion, but for completeness, I wanted to mention it.
Namely, if I check the OVMF build report file, I see, under
"PciSioSerialDxe":

>----------------------------------------------------------------------------------------------------------------------<
Dependency Expression (DEPEX) from INF
(gEfiBdsArchProtocolGuid AND gEfiCpuArchProtocolGuid AND gEfiMetronomeArchProtocolGuid AND
gEfiMonotonicCounterArchProtocolGuid AND gEfiRealTimeClockArchProtocolGuid AND gEfiResetArchProtocolGuid AND
gEfiRuntimeArchProtocolGuid AND gEfiSecurityArchProtocolGuid AND gEfiTimerArchProtocolGuid AND
gEfiVariableWriteArchProtocolGuid AND gEfiVariableArchProtocolGuid AND gEfiWatchdogTimerArchProtocolGuid)
------------------------------------------------------------------------------------------------------------------------
From Module INF:  (None)
From Library INF: (gEfiBdsArchProtocolGuid AND gEfiCpuArchProtocolGuid AND gEfiMetronomeArchProtocolGuid AND
gEfiMonotonicCounterArchProtocolGuid AND gEfiRealTimeClockArchProtocolGuid AND gEfiResetArchProtocolGuid AND
gEfiRuntimeArchProtocolGuid AND gEfiSecurityArchProtocolGuid AND gEfiTimerArchProtocolGuid AND
gEfiVariableWriteArchProtocolGuid AND gEfiVariableArchProtocolGuid AND gEfiWatchdogTimerArchProtocolGuid)
<---------------------------------------------------------------------------------------------------------------------->

That's because the driver depends on the UefiDriverEntryPoint lib class,
and the central instance of that class,
"MdePkg/Library/UefiDriverEntryPoint/UefiDriverEntryPoint.inf", comes
with:

#
# For UEFI drivers, these architectural protocols defined in PI 1.0 spec need
# to be appended and merged to the final dependency section.
#
[Depex.common.UEFI_DRIVER]
  gEfiBdsArchProtocolGuid AND
  gEfiCpuArchProtocolGuid AND
  gEfiMetronomeArchProtocolGuid AND
  gEfiMonotonicCounterArchProtocolGuid AND
  gEfiRealTimeClockArchProtocolGuid AND
  gEfiResetArchProtocolGuid AND
  gEfiRuntimeArchProtocolGuid AND
  gEfiSecurityArchProtocolGuid AND
  gEfiTimerArchProtocolGuid AND
  gEfiVariableWriteArchProtocolGuid AND
  gEfiVariableArchProtocolGuid AND
  gEfiWatchdogTimerArchProtocolGuid

)

>
> from: https://uefi.org/specs/PI/1.8/V2_DXE_Drivers.html#dxe-drivers
> <https://uefi.org/specs/PI/1.8/V2_DXE_Drivers.html#dxe-drivers>
>
> As for UEFI driver writes guide I really don't see any restrictions
> placed on DXE drivers in this spec, it seems to be mostly focused on
> describing types of UEFI drivers and their typical behavior.
>
> *2. Rewrite the change so that it doesn't use PCI_IO_PROTOCOL and
> driver inding*
>
> While possible I think this is a really bad idea that doesn't provide
> much value and only introduces additional interfaces for edk2
> community to maintain.
>
> - Replacing PCI_IO_PROTOCOL with PciSegment and friends would still
> require to pass information such as BDF, MMIO and mechanism to enable
> memory decode. So we still need some protocol interface. We could
> potentially drop PCI_IO entirely and instead say if you are in early
> DXE use EFI_SIO_PROTOCOL, but that one is less robust than PCI_IO.
> Note that in our implementation we do not require full PCI enumeration
> since LPSS UART is placed outside of the main PCI tree(situation is
> somewhat similar to IncompatiblePciDeviceSupport.c in EDK2).
>
> - Rewriting driver binding introduces additional cost without any real
> benefit I believe. Having driver binding allows the platform to call
> ConnectController when it is finished initializing enough of the HW so
> that UART can work.

That's exactly the problem. The platform is not supposed to call
ConnectController whenever it sees fit. ConnectController is to be
called from BDS.

As I attempted to describe above, your solution may work for your
practical purposes, but it has a dependency quirk that is not general.
It does not seem deterministic what subset of your platform DXE drivers
are able to produce *actual* DEBUG output.

... Ah, okay! This is the point where I re-read your steps, and now I
see the actual breakage.

It is your step #2.

Placing the PciSioSerialDxe driver -- which is a UEFI driver that
follows the UEFI driver model, and has a long list of dependencies on
the architectural protocols -- in the APRIORI DXE file of the firmware
volume(s) *completely violates* the DXE dispatch order.

I wrote above:

    How do you ensure that step (4) happens *both* early enough *and*
    late enough?

    You must do step (4) *late enough* for PciSioSerialDxe to have a
    fleeting chance to be dispatched by the DXE dispatcher (due to arch
    protocol deps), and *early enough* for your key modules to find
    SERIAL_IO_PROTOCOL (from PciSioSerialDxe) via DebugLib, in order to
    produce actual debug messages.

And your step#2 is the answer to that: you violate the standard DXE
dispatch order, by *overriding* the UEFI arch protocols depex in
PciSioSerialDxe, by placing the driver in the APRIORI DXE file.

The APRIORI DXE file is *only* appropriate to use when you have
*platform DXE* drivers that, for some reason, cannot be ordered
deterministically correctly, by way of depexes, against other platform
DXE drivers. APRIORI DXE is always a last resort; it serves as a crutch
when the DEPEX language is simply not expressive enough for a given
platform; APRIORI DXE is not license to *override* existent (and
standard!) depexes.

I'm lucky to have missed the significance of your step#2 on first sight;
it made me walk through the dependency chain. Which seemed
unsatisfiable, and for good reason -- the hack in step#2 is what allows
it to work.

That's certainly not an approach that should be recommended for general
use.

Laszlo

> Dropping driver binding and instead doing everything
> in entry point would require depending on driver dispatch order or on
> some additional protocol which would be placed in depex section of
> PciSioSerialDxe and installed by platform driver. It would also
> complicate the way in which we would support multiple UART devices which
> can be ready to operate on different stages in boot.
>
> - Other drawbacks - if we have completely separate entry point for DXE
> driver we need to keep 2 EFI modules in flash - 1 DXE only UART driver
> and 2nd UEFI only UART driver. As it is now we can simply keep the
> DXE_DRIVER instance in flash and it supports both early UARTS that are
> used for debug and late UARTS that are used for console redirection.
>
> Thanks,
> Mateusz
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115931): https://edk2.groups.io/g/devel/message/115931
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Laszlo Ersek]

On 2/25/24 17:18, Laszlo Ersek wrote:

> ... Ah, okay! This is the point where I re-read your steps, and now I
> see the actual breakage.
> 
> It is your step #2.
> 
> Placing the PciSioSerialDxe driver -- which is a UEFI driver that
> follows the UEFI driver model, and has a long list of dependencies on
> the architectural protocols -- in the APRIORI DXE file of the firmware
> volume(s) *completely violates* the DXE dispatch order.
> 
> I wrote above:
> 
>     How do you ensure that step (4) happens *both* early enough *and*
>     late enough?
> 
>     You must do step (4) *late enough* for PciSioSerialDxe to have a
>     fleeting chance to be dispatched by the DXE dispatcher (due to arch
>     protocol deps), and *early enough* for your key modules to find
>     SERIAL_IO_PROTOCOL (from PciSioSerialDxe) via DebugLib, in order to
>     produce actual debug messages.
> 
> And your step#2 is the answer to that: you violate the standard DXE
> dispatch order, by *overriding* the UEFI arch protocols depex in
> PciSioSerialDxe, by placing the driver in the APRIORI DXE file.
> 
> The APRIORI DXE file is *only* appropriate to use when you have
> *platform DXE* drivers that, for some reason, cannot be ordered
> deterministically correctly, by way of depexes, against other platform
> DXE drivers. APRIORI DXE is always a last resort; it serves as a crutch
> when the DEPEX language is simply not expressive enough for a given
> platform; APRIORI DXE is not license to *override* existent (and
> standard!) depexes.

I've looked again at the patch. It introduces a variant of
PciSioSerialDxe that is a DXE_DRIVER, and has

[Depex]
  TRUE

This means that step#2 (= APRIORI DXE) doesn't actually override a UEFI
driver's depex -- therefore I must agree that step#2 in itself does not
violate DXE dispatch order.

However, I continue to object to step (4), that is, to
ConnectController() being called by a platform module way before BDS is
entered. That's not how the driver binding protocol and the
ConnectController() boot service were intended, to my understanding.

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115932): https://edk2.groups.io/g/devel/message/115932
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Albecki, Mateusz]

[-- Attachment #1: Type: text/plain, Size: 3921 bytes --]

1. Using SerialDxe instead of PciSioSerialDxe - from our perspective the main idea is to avoid maintaining our own implementation of functions that actually communicate with UART controller. To use SerialDxe we would have to still maintain our own SerialPortLib that actually goes and sends data over UART. Also a note on LPSS UART - in terms of registers it is just a standard UART controller, the only quirk here is how to access it hence the custom PCI_IO_PROTOCOL implementation.

2. Using ConnectController before BDS - I noticed that the section I quoted says that BDS will use ConnectController however it doesn't say that it can't be used outside of that context. I did search the UEFI spec to see whether it provides additional restrictions and the only section that elaborates on this is the following:
"Under the UEFI Driver Model , the act of connecting and disconnecting drivers from controllers in a platform is under the platform firmware’s control. This will typically be implemented as part of the UEFI Boot Manager, but other implementations are possible. " - from  Section 2.5.6 Platform components
It seems to be rather permissive when it comes to who and when can call it.

3. How to make sure dispatch is early enough and not too late - this will depend on the overall platform implementation. For our part - we simply put it into flash as early as we can get away with. Even apriori section isn't strictly necessary if the platform is comfortable with relying on the fact that DXE core in MdeModulePkg dispatches modules in order of their placement in flash(that's not architectural as far as I know). Other platforms might choose to introduce explicit depex on gEfiSerialIoProtocolGuid. To reiterate this is the implementation that works for us in a sense that we get logs from all modules that change frequently from generation to generation, I understand that the same might not be true for platforms other than Intel however I think majority of platforms could still make at least some use of early UART debugging.

4. When exactly do we connect LPSS UART and start logging - we try to be as early as possible for this interface. We miss all of DXE_CORE logs(obviously), Pcd.inf and a couple of modules that implement some of the architectural protocols(from PciSio perspective metronome is the only actually required as far as we can tell since stall has to work).

I also want to note that I get why this is a controversial change. I didn't realize it earlier but it would be the first DXE_DRIVER in EDK2 tree that implements driver binding and in general it is strange to have PCI device driver that could potentially dispatch before PCI bus driver(however it is worth noting here that PciSioSerialDxe is not just a PCI driver, it is a combo driver capable of supporting PCI and SIO). That said I still think EDK2 in general needs a way to support early device drivers and using DXE_DRIVER seems like the least invasive idea. We need early drivers not just for debug(although that is one of the most important use cases I would say) other important use case is platform management through SMBUS/I2C/other serial interface which might be required to even be able to enumerate full PCI hierarchy(for instance some of the PCI slots might be powered down and you need to power them on sending commands over I2C) or maybe flash access to EFI variable storage(nothing says that you can't have it connected to PCI SPI controller) or maybe GPIO control to do any number of platform specific things.

Thanks,
Mateusz


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115968): https://edk2.groups.io/g/devel/message/115968
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]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 4402 bytes --]

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

[Laszlo Ersek]

On 2/26/24 16:13, Albecki, Mateusz wrote:
> 1. Using SerialDxe instead of PciSioSerialDxe - from our perspective
> the main idea is to avoid maintaining our own implementation of
> functions that actually communicate with UART controller. To use
> SerialDxe we would have to still maintain our own SerialPortLib that
> actually goes and sends data over UART. Also a note on LPSS UART - in
> terms of registers it is just a standard UART controller, the only
> quirk here is how to access it hence the custom PCI_IO_PROTOCOL
> implementation.

Is it possible to factor out a common base library class (with just one
instance) for accessing an LPSS UART? I guess the PCI B/D/F numbers
would have to be passed to the individual functions.

Then that library could be used in PciSioSerialDxe, and also in a (thin
wrapper) SerialPortLib instance.

>
> 2. Using ConnectController before BDS - I noticed that the section I
> quoted says that BDS will use ConnectController however it doesn't say
> that it can't be used outside of that context. I did search the UEFI
> spec to see whether it provides additional restrictions and the only
> section that elaborates on this is the following:
> /"Under the UEFI Driver Model , the act of connecting and
> disconnecting drivers from controllers in a platform is under the
> platform firmware’s control. This will typically be implemented as
> part of the UEFI Boot Manager, but other implementations are possible.
> " - from  Section 2.5.6 Platform components/ It seems to be rather
> permissive when it comes to who and when can call it.
>
> 3. How to make sure dispatch is early enough and not too late - this
> will depend on the overall platform implementation. For our part - we
> simply put it into flash as early as we can get away with. Even
> apriori section isn't strictly necessary if the platform is
> comfortable with relying on the fact that DXE core in MdeModulePkg
> dispatches modules in order of their placement in flash(that's not
> architectural as far as I know). Other platforms might choose to
> introduce explicit depex on gEfiSerialIoProtocolGuid. To reiterate
> this is the implementation that works for us in a sense that we get
> logs from all modules that change frequently from generation to
> generation, I understand that the same might not be true for platforms
> other than Intel however I think majority of platforms could still
> make at least some use of early UART debugging.
>
> 4. When exactly do we connect LPSS UART and start logging - we try to
> be as early as possible for this interface. We miss all of DXE_CORE
> logs(obviously), Pcd.inf and a couple of modules that implement some
> of the architectural protocols(from PciSio perspective metronome is
> the only actually required as far as we can tell since stall has to
> work).
>
> I also want to note that I get why this is a controversial change. I
> didn't realize it earlier but it would be the first DXE_DRIVER in EDK2
> tree that implements driver binding and in general it is strange to
> have PCI device driver that could potentially dispatch before PCI bus
> driver(however it is worth noting here that PciSioSerialDxe is not
> just a PCI driver, it is a combo driver capable of supporting PCI and
> SIO). That said I still think EDK2 in general needs a way to support
> early device drivers and using DXE_DRIVER seems like the least
> invasive idea. We need early drivers not just for debug(although that
> is one of the most important use cases I would say) other important
> use case is platform management through SMBUS/I2C/other serial
> interface which might be required to even be able to enumerate full
> PCI hierarchy(for instance some of the PCI slots might be powered down
> and you need to power them on sending commands over I2C) or maybe
> flash access to EFI variable storage(nothing says that you can't have
> it connected to PCI SPI controller) or maybe GPIO control to do any
> number of platform specific things.

To my eyes, this series saves a bit of refactoring now and the
maintenance of a thin wrapper library instance later, in exchange for a
very unconventional solution in MdeModulePkg that is not easily reusable
by other platforms. I don't like that trade-off; it makes me
uncomfortable.

If Liming and Mike accept this solution, I won't try to block it. In
that case however, please document the design (all we've discussed thus
far) in the "PciSioSerialDxeEarly.inf" file, in a (long) series of
comments.

Thanks
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#115971): https://edk2.groups.io/g/devel/message/115971
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Albecki, Mateusz]

[-- Attachment #1: Type: text/plain, Size: 2184 bytes --]

Is the idea to refactor PciSioSerialDxe to extract functions that access the HW and wrap it in the SerialPortLib instance? That solution would still save us some maintenance cost. However exploring the idea further I see following problems:

1. Relying on driver binding produces a fairly nice flow: platform driver initializes enough platform HW for UART to work -> platform driver calls ConnectController -> driver initializes UART. With SerialDxe a depex would have to be injected through our instance of SerialPortLib to stop the SerialDxe dispatch until platform driver made the platform ready.
2. I am wondering how it would work in case we want to allow dynamic configuration of debug port(basically selecting which UART controller would be used). With current solution we can select which one(or which ones) will be used by simply installing and connecting corresponding handles. With library solution I guess library would have to locate some protocols(possibly the same PCI_IO and DEVICE_PATH) that were installed by platform driver. It would also need to install notify on those protocols installation in case platform wants to add more uarts later on in the boot flow.
3. We still end up with 2 UART drivers in flash since PciSioSerialDxe is needed for PCI UARTs.

I also think this solution is comparable in effort to refactoring the PciSioSerialDxe so that it doesn't use driver binding when used as a DXE driver. In this solution driver would have one .c file for code with driver binding and one .c file for code when everything is done in entry point, it would still use DEVICE_PATH and PCI_IO/SIO. While I still think using the driver as is in DXE is best for us, in case that solution gets blocked I would like to understand if everyone would be ok with such refactoring.

Thanks,
Mateusz


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116055): https://edk2.groups.io/g/devel/message/116055
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]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 2638 bytes --]

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

[Laszlo Ersek]

On 2/27/24 18:15, Albecki, Mateusz wrote:
> Is the idea to refactor PciSioSerialDxe to extract functions that access
> the HW and wrap it in the SerialPortLib instance? That solution would
> still save us some maintenance cost. However exploring the idea further
> I see following problems:
> 
> 1. Relying on driver binding produces a fairly nice flow: platform
> driver initializes enough platform HW for UART to work -> platform
> driver calls ConnectController -> driver initializes UART. With
> SerialDxe a depex would have to be injected through our instance of
> SerialPortLib to stop the SerialDxe dispatch until platform driver made
> the platform ready.
> 2. I am wondering how it would work in case we want to allow dynamic
> configuration of debug port(basically selecting which UART controller
> would be used). With current solution we can select which one(or which
> ones) will be used by simply installing and connecting corresponding
> handles. With library solution I guess library would have to locate some
> protocols(possibly the same PCI_IO and DEVICE_PATH) that were installed
> by platform driver. It would also need to install notify on those
> protocols installation in case platform wants to add more uarts later on
> in the boot flow.

I think these requirements are *way* out of scope, and too clever, for
producing debug output. I'm now tempted to think that you are actually
best served by your existent separate driver.

SerialDxe and SerialPortLib are ill-suited if your system has multiple
serial ports, not to mention if you want different configurations from
boot to boot.

I think I'm permitted to disagree with a proposed patch without having
to counter-propose an alternative myself (i.e., the burden is on you, to
find an alternative). But, I'll brainstorm one more:

a. I'll assume that you store the debug port config in some non-volatile
UEFI variable.

b. In the PEI phase, have a PEIM turn that variable into a GUID HOB.
(This is doable because PEI can have read-only variable access.) Or
produce that HOB in some other way.

c. In the early platform DXE driver (= debug driver), read the HOB, and
publish a single instance of a new gEdkiiLpssUartDebugProtocolGuid.

d. The protocol should have a single member function,
WriteToAllConfiguredDebugPorts(). The debug driver should replicate the
message that is passed to this API to all ports enabled in the HOB.

e. Introduce a new DebugLib instance that, for DXE_DRIVER modules, has a
DEPEX on gEdkiiLpssUartDebugProtocolGuid:

[Depex.common.DXE_DRIVER]
  gEdkiiLpssUartDebugProtocolGuid

f. The debug ports should never appear in the system as EFI handles
(i.e., neither SerialIo nor PciIo nor DevicePath protocols -- no handles
at all).

g. The debug driver should use PciLib for configuring and accessing the
ports. Configuration includes any necessary "platform configuration" too.

h. If there is any LPSS UART access logic that's worth sharing -- for
maintenance reasons -- between PciSioSerialDxe and the debug driver,
then that should be extracted to new library class, and *two* library
instances (same directory, though). Both instances would nearly be
identical, but they'd have to *internally* abstract away PCI access. The
DXE instance (underlying the debug driver) would use PciLib, the UEFI
instance (underlying PciSioSerialDxe) would use PciIo. The code that
interacted with the LPSS UARTs would be common though.

i. The debug driver could register protocol notifies for the HII
protocols (which would become available much later), and once those
appeared, the debug driver could install HII forms, for managing the
non-volatile UEFI variable that configures the debug ports. (This could
perhaps be used for truly dynamic configuration, i.e., without having to
reboot; although personally I'd steer very clear of that avenue.)


As lower-hanging fruit, you could probably just implement step (h) in
isolation, and rebase both your existent driver, and PciSioSerialDxe,
onto that new library class (and its two library instances).

Other opinions are very welcome, of course! Personally, I'm out of
cycles on this topic.

Laszlo

> 3. We still end up with 2 UART drivers in flash since PciSioSerialDxe is
> needed for PCI UARTs.
> 
> I also think this solution is comparable in effort to refactoring the
> PciSioSerialDxe so that it doesn't use driver binding when used as a DXE
> driver. In this solution driver would have one .c file for code with
> driver binding and one .c file for code when everything is done in entry
> point, it would still use DEVICE_PATH and PCI_IO/SIO. While I still
> think using the driver as is in DXE is best for us, in case that
> solution gets blocked I would like to understand if everyone would be ok
> with such refactoring.
> 
> Thanks,
> Mateusz
> 



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116111): https://edk2.groups.io/g/devel/message/116111
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]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Albecki, Mateusz]

[-- Attachment #1: Type: text/plain, Size: 813 bytes --]

Sorry, I didn't want to make an impression that I expected solution to be delivered, I was merely trying to explain some of the complexity we are trying to handle on our side and why we didn't went for SerialDxe and instead opted to make PciSioSerialDxe work for our use case. Anyway thanks for the ideas, we will definitely look into implementing step h you mentioned in case other maintainers also disagree with this patch.

Thanks,
Mateusz


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116122): https://edk2.groups.io/g/devel/message/116122
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]
-=-=-=-=-=-=-=-=-=-=-=-



[-- Attachment #2: Type: text/html, Size: 1237 bytes --]

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

[Laszlo Ersek]

On 2/28/24 17:27, Albecki, Mateusz wrote:
> Sorry, I didn't want to make an impression that I expected solution to
> be delivered, I was merely trying to explain some of the complexity we
> are trying to handle on our side and why we didn't went for SerialDxe
> and instead opted to make PciSioSerialDxe work for our use case. Anyway
> thanks for the ideas, we will definitely look into implementing step h
> you mentioned in case other maintainers also disagree with this patch.

... BTW IIRC Mike recommended producing SerialIo protocol instances in
your platform driver, rather than PciIo, for your debug ports. I feel a
bit torn about that; on one hand, the uniformity of SerialIo protocols
looks flexible, on the other hand, how would you distinguish the debug
ports from the "normal" serial ports (mixing debug output with UEFI
console IO is not great; speaking from experience). Then you'd need to
get into DevicePath parsing (or call PciIo->GetLocation), and/or make
sure your ConIn/ConOut/StdErr variables wouldn't include the debug
ports... It's hard to form an opinion not knowing the platform and the
goals specifically. But, anyway, I've kind of run out of steam on this.

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#116137): https://edk2.groups.io/g/devel/message/116137
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]
-=-=-=-=-=-=-=-=-=-=-=-