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 C13A8941C9C for ; Sat, 30 Sep 2023 20:04:09 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=t6InH5VxaOGTNbSr/WLDMVu2U1LuGjeHVfLnX1SGq/M=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:From:To:References: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=1696104248; v=1; b=rvCKwqcMqTmM70A9OCG2Q1OoAM243mioaHhc9ugzdb60lMKhnYQP1EctoMnYox3slKe5KPHr SsAaaNkXld8GsdLimaxkjciSjuP/j1fEHPxuwvCQy1oX04uuUTY07FfuKqIXC6lNzkAl+Df0jbU KuJt5/m+SBYZI07CiHmX59bE= X-Received: by 127.0.0.2 with SMTP id cZkSYY7687511x4aSBSWxU3w; Sat, 30 Sep 2023 13:04:08 -0700 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.web11.47783.1696104247718701154 for ; Sat, 30 Sep 2023 13:04:07 -0700 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.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-661-g7zoKYhCPRa5RXDqR24vHg-1; Sat, 30 Sep 2023 16:04:03 -0400 X-MC-Unique: g7zoKYhCPRa5RXDqR24vHg-1 X-Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.rdu2.redhat.com [10.11.54.8]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 78F3F1C0634E; Sat, 30 Sep 2023 20:04:03 +0000 (UTC) X-Received: from [10.39.192.54] (unknown [10.39.192.54]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 97DF1C15BB8; Sat, 30 Sep 2023 20:04:02 +0000 (UTC) Message-ID: <542db9e1-cd28-27a2-3a98-5b0c85cd7c79@redhat.com> Date: Sat, 30 Sep 2023 22:04:01 +0200 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v1 1/1] ArmVirtPkg: Enable Early Serial For DxeCore From: "Laszlo Ersek" To: Sean Brogan , devel@edk2.groups.io, osde@linux.microsoft.com, Ard Biesheuvel References: <27912.1694092220075253890@groups.io> <1e6f37af-f407-43f2-aa14-9c5de85eb404@linux.microsoft.com> <343d24ed-12e4-6f9f-c726-5ce616c75738@redhat.com> In-Reply-To: X-Scanned-By: MIMEDefang 3.1 on 10.11.54.8 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: EIrv8kC4xkq3wJRo1HZrQdg0x7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=rvCKwqcM; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io On 9/30/23 16:43, Laszlo Ersek wrote: > On 9/8/23 01:58, Sean Brogan wrote: >> >> On 9/7/2023 1:54 PM, Laszlo Ersek wrote: >>> On 9/7/23 19:50, Sean Brogan wrote: >>>> I would argue that by declaring that your library class supports type >>>> DXE_CORE (or any core type) that you have declared you understand the >>>> uniqueness of the environment and have accounted for it. >>>> >>>> For instances that support DXE_CORE or MM_CORE module types we often use >>>> a global variable (to the library) to determine if the init routine has >>>> been completed. This does require a single byte check on each serial >>>> message write (hot path) but given all the code on that path this is not >>>> noticeable in performance measurements. In the case below this pattern >>>> could be used by the FdtPL011SerialPortLib to detect if it's init >>>> routine has been called. >>> Good point, but then I still claim that the "init check in each API" >>> should be done in a dedicated "DxeCoreDebugLibSerialPort" instance, and >>> not in a SerialPortLib instance. Here's why: >>> >>> (1) The SerialPortLib *class* requires SerialPortInitialize() to be >>> called before the other APIs. >> >> Where do you see this? Looking at the file here: >> edk2/MdePkg/Include/Library/SerialPortLib.h at master ยท tianocore/edk2 >> (github.com) >> I don't see that. I don't necessarily disagree with you but I am just trying to find out where this is documented. >> >> >>> The FdtPL011SerialPortLib instance does >>> nothing in its implementation of that function, because it relies on the >>> constructor doing the same work. Therefore I agree that >>> FdtPL011SerialPortLib is not suitable for DXE_CORE, and I would suggest >>> removing DXE_CORE from LIBRARY_CLASS in the INF file, after the pipe >>> sign ("|"). >> Agree >>> (2) A new SerialPortLib instance should be added, very similar to >>> FdtPL011SerialPortLib -- the difference should be that it should have no >>> constructor, and the same job should be done in SerialPortInitialize(). >>> This library instance sould be suitable for *direct use* in DXE_CORE >>> (and should likely be restricted to DXE_CORE exclusively). >>> >>> The reason for that is the following. The DXE Core is entitled to >>> consume a lib instance without calling its constructor, in case the lib >>> instance declares itself DXE_CORE-capable (this is your argument). (In >>> fact such a lib instance is not supposed to have a constructor at all -- >>> it might not be called anyway.) However, the DXE Core is *not* entitled >>> to ignore library *class* restrictions, and an explicit call to >>> SerialPortInitialize() is required by the SerialPortLib *class*. IOW, if >>> the DXE Core ever wanted to use SerialPortLib *directly*, it would have >>> to call SerialPortInitialize() before calling the other SerialPortLib >>> APIs, regardless of where and when the DXE Core ran the library >>> constructor list. >>> >>> So that's why such a new FdtPL011SerialPortLib variant would be proper >>> for DXE_CORE. >>> >>> (3) In turn, the new DxeCoreDebugLibSerialPort instance -- which would >>> have no constructor -- would be responsible for tracking in each API >>> implementation whether SerialPortInitialize() had been called before. >> Agree >>> (4) This also means that the current BaseDebugLibSerialPort in MdePkg is >>> unsuitable for DXE_CORE usage, and so its LIBRARY_CLASS module type list >>> should be made explicit -- it should *exclude* the DXE_CORE. Even though >>> BaseDebugLibSerialPort has a BASE type entry point, this lib instance >>> relies on having a constructor (where it calls SerialPortInitialize()!), >>> and that rules it out for DXE_CORE usage. >> Agree >>> IOW, I agree with you; my point is only that the serial init tracking >>> belongs in a new DebugLib instance (because, at the *class* level, >>> DebugLib permits the DXE_CORE to call its APIs in any order; whereas >>> SerialPortLib requires SerialPortInitialize() to be called first, also >>> at the *class* level). >>> >>> Laszlo >>> >> Just for discussions sake you could also imagine a solution where the >> "base" instance does init tracking and then a new library instance is >> used only for XIP PEI that executes from RO memory (flash or >> otherwise). Also note that this isn't just a DXE_CORE problem. SEC, >> PEI_CORE, MM_CORE_STANDALONE and SMM_CORE types may have these similar >> restrictions. > > I've given this more thought. I think the problem cannot be solved 100% > generally in BaseDebugLibSerialPort. > > Lib instance constructors Lib instance constructors > called manually called automatically > > No RAM SEC PEIM > PEI_CORE > > RAM DXE_CORE DXE_DRIVER > SMM_CORE DXE_SMM_DRIVER > MM_CORE_STANDALONE DXE_RUNTIME_DRIVER > UEFI_DRIVER > UEFI_APPLICATION > SMM_DRIVER > MM_STANDALONE > > (The SEC, PEI_CORE and PEIM types may also run from RAM, dependent on > platform, but in their strictest definition, they may execute from > flash; i.e., with no writable global variables.) > > The lower right corner (RAM + ctors) is no problem; the DebugLib > instance constructor calls SerialPortInitialize(); done. > > The lower left corner (RAM + no ctors) can be solved in the DebugLib > instance. Just before we call SerialPortWrite() every time, check a > global boolean. If the variable is FALSE, call SerialPortInitialize(), > set the variable to TRUE, and then call SerialPortWrite(). Otherwise, > only call SerialPortWrite(). Unfortunately, even this (i.e., the DXE_CORE case) proves intractable in practice. There are several SerialPortLib instances that, regardless of whether they do anything in SerialPortInitialize(), have constructors, adn claim themselves DXE_CORE-ready. Fixing these SerialPortLib instances is just not practical. One thing is to move or copy the initialization logic from the constructor to SerialPortInitialize(). The larger problem is that these SerialPortLib instances have *further* dependencies, which *also* have constructors. So if I remove the constructor from SerialPortLib totally, that breaks the topological sorting of constructors even in other types of modules. If I keep the logic in both SerialPortInitialize() and the constructor, then the constructors of the further library dependencies are still not called from SerialPortInitialize(). So all in all this is a train wreck, it's so pervasive and recursive that it can't be fixed centrally; it has such a ripple effect. What we can do is copy BaseDebugLibSerialPort to ArmVirtPkg, and fix it up there (call SerialPortInitialize() as needed), in addition to exposing the initialization logic of FdtPL011SerialPortLibInitialize() in SerialPortInitialize() as well. Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#109235): https://edk2.groups.io/g/devel/message/109235 Mute This Topic: https://groups.io/mt/101203427/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-