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 D6971AC16FB for ; Thu, 7 Sep 2023 23:58:35 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=yeiZ6VRwrq8kl1YGOq0pr06wZGpwu8kG5RoDcT/pi88=; c=relaxed/simple; d=groups.io; h=ARC-Seal:ARC-Message-Signature:ARC-Authentication-Results:Message-ID:Date:User-Agent:Subject:To:References:From:In-Reply-To:MIME-Version:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Type:Content-Language; s=20140610; t=1694131114; v=1; b=gqjY/I9+/V2idLkuC2ZBxL8B9Sc3mZebtS8c8sjk9hmYBIrXJNPkb1ecB35/tydLpZEBPXkT /eCeEHEQgqPKccI3OvEup9peWT9rwkrr+0vHebJ/m6QfOL1p1GMmZ8iABBCcLJ6h7THkTkFZ/1l 1bVLMNl5zfZ4LsxVZ2+BYJFE= X-Received: by 127.0.0.2 with SMTP id acETYY7687511xudXo4W2WqP; Thu, 07 Sep 2023 16:58:34 -0700 X-Received: from NAM11-BN8-obe.outbound.protection.outlook.com (NAM11-BN8-obe.outbound.protection.outlook.com [40.92.20.10]) by mx.groups.io with SMTP id smtpd.web10.28667.1694131113664700044 for ; Thu, 07 Sep 2023 16:58:33 -0700 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=TxsoTC+9neOloKQX0Z8xskZzUwwWwST6BpU6t4g7h0yQt3zU522jQWuwdF3eR56I6f60QuE0fydKfDQKAXSn7K7y55vKeTJXqTXTr0tHS/4z9JYcuCsB2bzGrr0OCd5EKnRk6Uh0UdTyA2jxx6Bu3V+q89Rjj47orllGfL+y8sHIyLWKU0aW9fLSmsLuVn7iPm9bcLqcQdbERyUBjXgJOqxhl1JEx/RlXCLyKGDmTn8J8n/UykBLLaQ5XcJ53+PruO3pNaqSPzk+cMKSY3s7pqNhGh5BkcQkiHyEKjdkPGYPf1QKuLeWSkFRh9+Q3UlrkXA69v14qqaAhDLZDjkgLw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-AntiSpam-MessageData-ChunkCount:X-MS-Exchange-AntiSpam-MessageData-0:X-MS-Exchange-AntiSpam-MessageData-1; bh=Bbw7lUeAyM7ex4c8qV5WIfh2gYUJuC2apppPcBFidOI=; b=Un5qv4gsaXVcfV21Iw5VbUOeaJMgC/QM6syBVxZr2YFcysIEcVytV8QS+ELG22dT96pHK8+2S99TUjSEsPmZozMo3U3ZBEZg0/4f0/tE3iEMVWcdqJFveLdGj5JFqZ/czTFP5ncFhWKh9wL+cVG3ZoVJPKT0XlpxEuwz98KWIEnkvDU7eprEL1buuLhuTPRP/lCwFVDvbQLnYFTBxVLQdWAHpa0qqM13up17Xvs9XhQU7rA1DY4H05FfKNRy7RNDeNhzlSE94x+wVxsqJrSBIsOBP4XTM485KSh1mLWkSIuGT0q8f74p9rkOkHvQFKUSZ6x5arkkQwpPaU/nwV3N4A== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=none; dmarc=none; dkim=none; arc=none X-Received: from BY3PR19MB4900.namprd19.prod.outlook.com (2603:10b6:a03:354::11) by MN0PR19MB5730.namprd19.prod.outlook.com (2603:10b6:208:375::16) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.6745.36; Thu, 7 Sep 2023 23:58:31 +0000 X-Received: from BY3PR19MB4900.namprd19.prod.outlook.com ([fe80::f5ef:669c:fea1:a213]) by BY3PR19MB4900.namprd19.prod.outlook.com ([fe80::f5ef:669c:fea1:a213%7]) with mapi id 15.20.6768.029; Thu, 7 Sep 2023 23:58:30 +0000 Message-ID: Date: Thu, 7 Sep 2023 16:58:28 -0700 User-Agent: Mozilla Thunderbird Subject: Re: [edk2-devel] [PATCH v1 1/1] ArmVirtPkg: Enable Early Serial For DxeCore To: Laszlo Ersek , 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: "Sean" In-Reply-To: <343d24ed-12e4-6f9f-c726-5ce616c75738@redhat.com> X-TMN: [PL0od1quOey5nY8K93KO4qAPo6mPDOuyCKUKF9SF1yHckuP31RmNLo2LxDwxhOO5] X-ClientProxiedBy: CYZPR14CA0038.namprd14.prod.outlook.com (2603:10b6:930:a0::26) To BY3PR19MB4900.namprd19.prod.outlook.com (2603:10b6:a03:354::11) X-Microsoft-Original-Message-ID: <0a1477e5-5394-453a-850e-376fb7bddee1@outlook.com> MIME-Version: 1.0 X-MS-Exchange-MessageSentRepresentingType: 1 X-MS-PublicTrafficType: Email X-MS-TrafficTypeDiagnostic: BY3PR19MB4900:EE_|MN0PR19MB5730:EE_ X-MS-Office365-Filtering-Correlation-Id: 9dc9bfc7-4a86-4c58-966b-08dbaffe562b X-Microsoft-Antispam-Message-Info: HBN4buD4rjhukEgPtiNZzPm9bPePSVMVbhT+JgePWZFG2x02gGHHgSSDCHyjwmVa79WhiHqzWP9wHLpdbcBeJ3yc8HcDI7Xj3K94lbAq/y5XthVGWna6+3kwcGn8RfRJOz4wNW4uTl3bLUN5mqFTkpzpJbLZLD/jE0Teqi5sSMTkmg3ATmDH3RRlqpVopglt4z76CniASgZ9Iel75F063TfJtDWWXAKW7mBlSsNuS2+BMaE99aIa9UrF7otfN/dcXoPdtUL1WFnNPsI1Lyi9jcUrsvU4akRorT+nyfxmAHkGO57wvNXaYenLdIgePkujkHX4ULtR0fMVRfMNkCxWM2vvUl+9rD03o3XDIRqmdQODz+xQWvwYl0oNgnuaHsUE060ZR/RRbgFRh/625xxtByOZ2BBryNL6RdC4iKpsO4Ia/U0fJdzNtlGSvz/zMojzZWDvkfCKWFiIUQyRxC2ea7MyoPF0T8ZcKvXVhomgLPu463XUQe/pjlM3oz+cXZjY4SJwvClACFe51YcJwsNVU5OC4l4qfxZ47vQPZbhlpCEcPm2oU3qHBpHuNz7uiNud06zU4IZl1LjUg6iEunel+0VmpeAtbUxnWsUxFmgCXPg= X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1 X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?NWkwbnJCV3JhbGNXK0ZTd2FGWTFJUVV4bVU3aWhzcHVOaEdmS29MMmxMSXF1?= =?utf-8?B?T2tDRzBYdTg0WlRnd2pvRXFVbXY1NEZQR2Nidzh2amJ1VDQ3UlNoUnBOWXZM?= =?utf-8?B?MXlJQWxGWU1WcWpaMmVURWhSTHBvVWd6V1hSd2hXV01pOEVWNFVEVzJUWDc5?= =?utf-8?B?cnlLeWhrQWJsZnlybSs4MG9DcEUyMXZUZVVFc0pYbU1YTDJiaVIvK2NscnJy?= =?utf-8?B?YWJGaHJMVGlKQ2lhS2tBa3BxRDMydHFIWWVEbUUrS2EvS2N1a0V4Mlp3N21T?= =?utf-8?B?czR3bjNJdzBaQUlyb1VMeGQzWnpiKy92bmhpL0hrMVFqdjhPK2VhM2RSd1BN?= =?utf-8?B?TGt2TS9sZXZYWUVJSmgzYXI3MmdERlpxYStDWkVuK1duWHkrVDdWblZYcTNa?= =?utf-8?B?b2hUdDR1dHh6dVJqOENKSWVQdVJNbTlIN1ZOSkxDUHV6Rk4yY1dsTXkxYXJR?= =?utf-8?B?TVcxcnU5bFNFOG02ZVhHQm1sR0hUK1ZrLzRXeitXM01KZExML1l0dFpkZUp2?= =?utf-8?B?N0t2ZlBZSmwyMlpZdGRQNTRhcUdZMTQ4Mm5ENlhLNkNab1RFUjEvOEFFTHRF?= =?utf-8?B?d0JvNnJ4NEF4UFh4Y1YxZ0RQbklhR2d5VTVxSVp6N3Q0eWtOTUJOMzl6aUd2?= =?utf-8?B?LzB3Tlg4M1NtU2I1UkMwY3NOWkJaakhSSjhVYzBFT1hTT05EWThIc01jeDZz?= =?utf-8?B?bkFKOW9lbiszUlVCajhIaEJaWXJxdUltTUdPRmpjK2hlZ0FuSTNsc2ZBODhQ?= =?utf-8?B?Yk9OdzN4NWhLRTlQa3UrU3lUVFM4Mm1wZSsyaitDMVVYRk80WGdnc0J5RG5S?= =?utf-8?B?V3FtZkhjZmVxRzFtcThlcWpTamc3ZDJPekNDb1lnYjhGNGZxVUlYZk43K243?= =?utf-8?B?aUF5ajBjaUJab1gwUG8wNTRBam4yZ29nMjFlV1MyZFRHalVVQjZyWkI4cU04?= =?utf-8?B?bi9hZ29vb2xqbE5qWk5DTDhoandya0I3dGJ6cnpBUjZua2hZZzRZSnllL1dI?= =?utf-8?B?b3o2dktFZUEvemF4MHlzT21pdjBlS1IrcFFDVlNqcjMydzBSTXNYdmovWjl1?= =?utf-8?B?TjUzNk9mWTg1TnV3NnVMM2Q2QmIzMFNLYlVEWU5WNm15YTVodWk1Wit6YUZk?= =?utf-8?B?dVZUVkhuR0pYdEtMWHhRY1NTK3VrTVBUOFovM0Q3M3QyMEFyYnlSUnhnNm1l?= =?utf-8?B?M1dEeHhpeWU1d2REdnZJbW5oU091OER2YnA0KzRVOWZ2UzhCMFJRSm5na1pm?= =?utf-8?B?ZkpVbzVnVXhPdTYyK3g2QTNjdFMzSldDb3IzcnBWU1J6YW5vaTZRMXU4OEpQ?= =?utf-8?B?akc3clVSYWlZVnRZdGtIK24yYkxqbm9JWTZ3MlJ5c05mQXpINXRWYUNmSElk?= =?utf-8?B?ZmFlZGlKUDlYbXZ1RGcyMnd6YXBLMDE5L0FLRG5mOWRjeTZsV3ZtZTNuODhw?= =?utf-8?B?RUxvUEV2alNQeUpmOUtYZUUvUlRzMHZFVDVMclA5TVk4dkx3a0N4aVJDK2RZ?= =?utf-8?B?WjdDK1RSWTNORC90dkZHd1E0SkFrbzBNMG5kVEx5WC9qaXZsSjhIRGtuUVhU?= =?utf-8?B?bTBuSTRwaXNLVWU4Q2ZIcVBNc2FhZkhKcmVPMlpyc0ZMcFJuVVlIMEVJNTg3?= =?utf-8?B?TXV5bGpLVGdZMGFWUHEwRTBuRnp2Um55ZFl2NVhEa1ovbFFZdXkvYmhIRXlv?= =?utf-8?B?UjVzUGxlRXdLejhnNDV4dzRTcnMrenpLK01rc0JmQWo2NElCZ0tiOXVnPT0=?= X-OriginatorOrg: outlook.com X-MS-Exchange-CrossTenant-Network-Message-Id: 9dc9bfc7-4a86-4c58-966b-08dbaffe562b X-MS-Exchange-CrossTenant-AuthSource: BY3PR19MB4900.namprd19.prod.outlook.com X-MS-Exchange-CrossTenant-AuthAs: Internal X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Sep 2023 23:58:30.9207 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 84df9e7f-e9f6-40af-b435-aaaaaaaaaaaa X-MS-Exchange-CrossTenant-RMS-PersistedConsumerOrg: 00000000-0000-0000-0000-000000000000 X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN0PR19MB5730 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,spbrogan@outlook.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: sM87OPVzte2zNf6SUJrzie4Lx7686176AA= Content-Type: multipart/alternative; boundary="------------5Noa8aA7p4GcA47oANZA0abT" Content-Language: en-US X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b="gqjY/I9+"; arc=reject ("signature check failed: fail, {[1] = sig:microsoft.com:reject}"); dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=outlook.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 --------------5Noa8aA7p4GcA47oANZA0abT Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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. Thanks Sean >> 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 (#108426): https://edk2.groups.io/g/devel/message/108426 Mute This Topic: https://groups.io/mt/101203427/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=- --------------5Noa8aA7p4GcA47oANZA0abT Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 8bit


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. 


Thanks

Sean



      

        
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 (#108426) |


  

|

  Mute This Topic


| New Topic







Your Subscription |
Contact Group Owner |

Unsubscribe

 [rebecca@openfw.io]


_._,_._,_



--------------5Noa8aA7p4GcA47oANZA0abT--