From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id DE1D4D8106C for ; Sun, 25 Feb 2024 16:18:38 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=7k5DoTg8JLdyOEBzLZLUMeFyLl1ezdIvJ8Mmrq4o14U=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:To:References:From:Cc:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1708877917; v=1; b=iasca8lu0VixgJC1sA0Ny26N2nGGAwCu7SmBqg7kVzNEvHPOUFJXNAQOHfN3s+2wVGFjcdWS Y7I9BOpaWyJAZH5veLKwyoH41Xbuf/VLRwiC15h19vBWSD9Y2qxWPGNdznFC+WqkZ9SW6hbRhDO 7BEcSHL18HLZXoTQ2zUS/8Ko= X-Received: by 127.0.0.2 with SMTP id 67RcYY7687511xpbxwGrcArQ; Sun, 25 Feb 2024 08:18:37 -0800 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by mx.groups.io with SMTP id smtpd.web10.49.1708877916589906547 for ; Sun, 25 Feb 2024 08:18:36 -0800 X-Received: from mimecast-mx02.redhat.com (mx-ext.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-227-LqWgl3W4MASGkLegYtOVEQ-1; Sun, 25 Feb 2024 11:18:32 -0500 X-MC-Unique: LqWgl3W4MASGkLegYtOVEQ-1 X-Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 4CC1F1C05153; Sun, 25 Feb 2024 16:18:32 +0000 (UTC) X-Received: from [10.39.192.57] (unknown [10.39.192.57]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 6898C8CE8; Sun, 25 Feb 2024 16:18:31 +0000 (UTC) Message-ID: <7a99d4e0-9fbb-9c9c-b82a-6425e1b9c9ea@redhat.com> Date: Sun, 25 Feb 2024 17:18:30 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH 1/1] MdeModulePkg: Load Serial driver earlier in DXE To: devel@edk2.groups.io, mateusz.albecki@intel.com References: <5914.1708703400468529849@groups.io> From: "Laszlo Ersek" Cc: Michael Kinney In-Reply-To: <5914.1708703400468529849@groups.io> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.5 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: Jb1xXnWEx8AhPuCk3oBUaDAWx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=iasca8lu; spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none) 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 gEfiMetronomeArchP= rotocolGuid AND gEfiMonotonicCounterArchProtocolGuid AND gEfiRealTimeClockArchProtocolGuid = AND gEfiResetArchProtocolGuid AND gEfiRuntimeArchProtocolGuid AND gEfiSecurityArchProtocolGuid AND gEfiTimerA= rchProtocolGuid AND gEfiVariableWriteArchProtocolGuid AND gEfiVariableArchProtocolGuid AND gEfi= WatchdogTimerArchProtocolGuid) ---------------------------------------------------------------------------= --------------------------------------------- >From Module INF: (None) >From Library INF: (gEfiBdsArchProtocolGuid AND gEfiCpuArchProtocolGuid AND = gEfiMetronomeArchProtocolGuid AND gEfiMonotonicCounterArchProtocolGuid AND gEfiRealTimeClockArchProtocolGuid = AND gEfiResetArchProtocolGuid AND gEfiRuntimeArchProtocolGuid AND gEfiSecurityArchProtocolGuid AND gEfiTimerA= rchProtocolGuid AND gEfiVariableWriteArchProtocolGuid AND gEfiVariableArchProtocolGuid AND gEfi= WatchdogTimerArchProtocolGuid) <--------------------------------------------------------------------------= --------------------------------------------> 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 ne= ed # 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 > > > 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 >=20 -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#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] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-