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 7C5C87803D7 for ; Thu, 7 Sep 2023 20:54:25 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=klrQeDetRCggkaD23vbh1shwogk2LZQ3S5+JmmX0ccw=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:To:References:From: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=1694120064; v=1; b=vkPVBmOAJhWD96ZeqMGSTGRqEJuqX7fKnDWmfY8/2XWMmCY8obQZy1XxaybcxGipNI936AJ/ 9yKWx7+i8L7OXjCJRwI1/Ebw3T5fcdOuOoZ+xIyh0Ng446eStWJW2wPowo8odePo1RUoaTGdxWd 7+lMhI5SHu1cJfUKd4y9FASs= X-Received: by 127.0.0.2 with SMTP id VYdPYY7687511x8XRzCA8mUB; Thu, 07 Sep 2023 13:54:24 -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.web10.24770.1694120063302360587 for ; Thu, 07 Sep 2023 13:54:23 -0700 X-Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-477-96yuJxzzNWGRFa1I8uq6Ng-1; Thu, 07 Sep 2023 16:54:19 -0400 X-MC-Unique: 96yuJxzzNWGRFa1I8uq6Ng-1 X-Received: from smtp.corp.redhat.com (int-mx07.intmail.prod.int.rdu2.redhat.com [10.11.54.7]) (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 14E1B181792D; Thu, 7 Sep 2023 20:54:19 +0000 (UTC) X-Received: from [10.39.192.43] (unknown [10.39.192.43]) by smtp.corp.redhat.com (Postfix) with ESMTPS id CBB17140E950; Thu, 7 Sep 2023 20:54:17 +0000 (UTC) Message-ID: <343d24ed-12e4-6f9f-c726-5ce616c75738@redhat.com> Date: Thu, 7 Sep 2023 22:54:16 +0200 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v1 1/1] ArmVirtPkg: Enable Early Serial For DxeCore 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> From: "Laszlo Ersek" In-Reply-To: X-Scanned-By: MIMEDefang 3.1 on 10.11.54.7 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: gUN7EAoqIC8tTUpl1GK6yQfmx7686176AA= 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=vkPVBmOA; 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/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. 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 ("|"). (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. (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. 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 > On 9/7/2023 8:24 AM, Oliver Smith-Denny wrote: >> On 9/7/2023 6:10 AM, Laszlo Ersek wrote: >>> (replying on the webui... sorry!) >>> >>> The problem is actually embedded in MdePkg and MdeModulePkg. >>> >>> - In DxeMain() (and in functions called by DxeMain()), we call >>> DebugLib APIs *before* reaching ProcessLibraryConstructorList(). >>> >>> - In ArmVirtQemu, we resolve the DXE Core's DebugLib dependency to >>> BaseDebugLibSerialPort (from MdePkg). >>> >>> - BaseDebugLibSerialPort has a constructor function >>> (BaseDebugLibSerialPortConstructor()). >>> >>> These already suffice for broken DebugLib behavior. APIs of a library >>> class should not be called because the library instance has a chance >>> to initialize. >>> >>> The rest is circumstantial. Like, BaseDebugLibSerialPortConstructor >>> calls SerialPortInitialize, but our SerialPortInitialize (in >>> FdtPL011SerialPortLib) does nothing. Well, the latter doesn't need to >>> do anything, because FdtPL011SerialPortLib has its own constructor >>> (FdtPL011SerialPortLibInitialize), thus, if constructors were called >>> properly, then BaseDebugLibSerialPort + FdtPL011SerialPortLib would >>> work properly together, regardless of SerialPortInitialize being >>> empty in the latter. >>> >>> Basically the DXE Core has a hidden requirement -- it can only use >>> such DebugLib instances that need no explicit initialization. >>> >>> The proposed patch works around the problem by satisfying that hidden >>> requirement one level lower down: in the SerialPortLib instance. The >>> initialization of BaseDebugLibSerialPort is still busted (its >>> constructor is not called, so it cannot call SerialPortInitialize >>> either), but now it is masked, because EarlyFdtPL011SerialPortLib >>> works withouth *both* SerialPortInitialize and construction. >>> >>> The real fix would be to make the DXE Core requirement explicit, by >>> introducing separate (dedicated) DebugLib and SerialPortLib *classes* >>> (whose APIs are guaranteed to work without initialization). >>> >>> Laszlo >> >> Thanks for the comprehensive breakdown! :). I completely agree that >> fixing this at the upper level (and ideally documenting this >> requirement) is the better move. >> >> I can drop this patch and take a crack at that. I'm in the last few >> weeks leading up to an extended parental leave, so we'll see if I can >> squeeze it in prior to then :). >> >> Oliver >> >> >> >> >> > -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#108418): https://edk2.groups.io/g/devel/message/108418 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] -=-=-=-=-=-=-=-=-=-=-=-