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 859E2941973 for ; Sat, 30 Sep 2023 14:43:22 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=emurHZfzILIo6tku3n6YFklgERzvtxOBuR0i8Y9obV4=; 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=1696085001; v=1; b=ncJrGVd1TqlWcEZp1tXMzPLxJsAX/FSI94OmJCXa2hxiu29PJ+iGA5yVUd3oWBMewv8FVy0s Ib2+s8SJga87qct+4r/0I1tfGwDVEiexfI9VrygG3+dODvAJAxPFfmdeZY3srfC6iqTgt9Vdfiz JxCzYJsckCii3CGj2ionV12c= X-Received: by 127.0.0.2 with SMTP id 2QhRYY7687511xi2sxDwsUSV; Sat, 30 Sep 2023 07:43:21 -0700 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web10.42048.1696085000398358769 for ; Sat, 30 Sep 2023 07:43:20 -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-302-9zRbjmqHMoe48jHcrOg2uQ-1; Sat, 30 Sep 2023 10:43:16 -0400 X-MC-Unique: 9zRbjmqHMoe48jHcrOg2uQ-1 X-Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.rdu2.redhat.com [10.11.54.2]) (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 A5EC929AA2EF; Sat, 30 Sep 2023 14:43:15 +0000 (UTC) X-Received: from [10.39.192.54] (unknown [10.39.192.54]) by smtp.corp.redhat.com (Postfix) with ESMTPS id C1B3340C6EBF; Sat, 30 Sep 2023 14:43:14 +0000 (UTC) Message-ID: Date: Sat, 30 Sep 2023 16:43:13 +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> <343d24ed-12e4-6f9f-c726-5ce616c75738@redhat.com> From: "Laszlo Ersek" In-Reply-To: X-Scanned-By: MIMEDefang 3.1 on 10.11.54.2 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: 595l8iWM6PiTOfeOQAiYDdC5x7686176AA= 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=ncJrGVd1; 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/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(). The upper right corner (No RAM + ctors) is where it starts getting messy. In the DebugLib instance, we have no way to track whether SerialPortInitialize() was called. Thankfully, we don't have to: we can call it from the constructor, guaranteeing exactly 1 call to it, before the other APIs are used. The underlying SerialPortLib can then initialize the hardware if necessary. If the SerialPortLib instance would like to cache various things in RAM, then it can't (no RAM!), but that's OK, the SerialPortLib instance has to deal with the problem internally, after all it declared itself PEIM-ready. This gets hackish, because we need to starting thinking about SerialPortLib internals in DebugLib, but ultimately it's OK. The upper left corner is the wreck. No RAM and no constructors. So in DebugLib, we cannot track SerialPortInitialize() *at all*. Either we don't call it ever (which is a library interface violation), or we call it before every SerialPortWrite() call (which is *also* a library interface violation). A SEC-compatible SerialPortLib instance may very well exist that performs such actions in SerialPortInitialize() that must be performed *exactly once* during boot. And there's no way to be compatible with such a SerialPortLib instance from a DebugLib instance that runs from flash and cannot rely on its constructor being called. So the upper left corner is unsolvable if we respect library class (responsibility) boundaries. In that corner, the DebugLib instance would have to assume either that (a) the SerialPortLib instance needed zero calls to SerialPortInitialize(), or that (b) the SerialPortLib instance tolerated infinite calls to SerialPortInitialize(). Currently, the right side column has no problem in edk2. The left side column is buggy. I'd like to propose patches for fixing the lower left corner, but the upper left corner is unfixable (in MdePkg anyway). Laszlo -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#109234): https://edk2.groups.io/g/devel/message/109234 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] -=-=-=-=-=-=-=-=-=-=-=-