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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
