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 0ACC8740040 for ; Tue, 10 Oct 2023 13:15:07 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=Bjl832WwAOqi4gg2pu58Bgvi4DfXJpFyZTtagwE700Q=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:To:Cc: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=1696943706; v=1; b=NERz/iUpNLBDutI5Bx0zUjkLm8zKE/u4pdX8m8b3/se8WLhfQBpMOHqFcB6Ve652gCtm3+R2 xW3JuRWPDKyGXQTru3roSfJbhlfcYuED8Fv/HYHCaqj8Z16Dt66HVN4Bi01e/SoYyIsaGmBJWnq Gpcis8GyZp8u8jiQj+JdyCkA= X-Received: by 127.0.0.2 with SMTP id D0AHYY7687511xG8j2f0NZzQ; Tue, 10 Oct 2023 06:15:06 -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.91105.1696943705615025942 for ; Tue, 10 Oct 2023 06:15:06 -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-479-C6csO7GKNwO3LhscKL3pWw-1; Tue, 10 Oct 2023 09:14:43 -0400 X-MC-Unique: C6csO7GKNwO3LhscKL3pWw-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 01A4F185A7B9; Tue, 10 Oct 2023 13:14:43 +0000 (UTC) X-Received: from [10.39.192.135] (unknown [10.39.192.135]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 4A58A1C060DF; Tue, 10 Oct 2023 13:14:40 +0000 (UTC) Message-ID: <8c5a2aa6-c7b7-0e95-0454-31a5036f9adc@redhat.com> Date: Tue, 10 Oct 2023 15:14:39 +0200 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v2 3/4] MdeModulePkg: PeiMain: Introduce implementation of delayed dispatch To: devel@edk2.groups.io, kuqin12@gmail.com Cc: Jian J Wang , Dandan Bi , Liming Gao , Debkumar De , Catharine West , Mike Turner References: <20231010001856.1567-1-kuqin12@gmail.com> <20231010001856.1567-4-kuqin12@gmail.com> From: "Laszlo Ersek" In-Reply-To: <20231010001856.1567-4-kuqin12@gmail.com> X-Scanned-By: MIMEDefang 3.4.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: CbS5yYQXsUJHdycJ28gISU96x7686176AA= 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="NERz/iUp"; 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 10/10/23 02:18, Kun Qin wrote: > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=4496 > > This change adds the implementation that fits the needs and description > of PI spec defined Delayed Dispatch PPI in Pei Core. > > The PPI would allow minimal delay for registered callbacks. As well as > allowing other functions to wait for GUIDed delayed dispatch callbacks. > > Cc: Jian J Wang > Cc: Dandan Bi > Cc: Liming Gao > Cc: Debkumar De > Cc: Catharine West > > Co-authored-by: Mike Turner > Signed-off-by: Kun Qin > --- > > Notes: > v2: > - Fixed function documentation [Liming] > - Removed GUID declaration internal to PEI core [Liming] > - Removed max entry PCD declaration and use macro instead [Liming] > - Added "PRODUCED" in inf [Liming] > > MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c | 366 ++++++++++++++++++++ > MdeModulePkg/Core/Pei/PeiMain/PeiMain.c | 3 + > MdeModulePkg/Core/Pei/PeiMain.h | 76 ++++ > MdeModulePkg/Core/Pei/PeiMain.inf | 5 + > MdeModulePkg/MdeModulePkg.dec | 8 + > 5 files changed, 458 insertions(+) Please point your "diff.order" git config item to "BaseTools/Conf/diff.order". The "BaseTools/Scripts/SetupGit.py" script will do that (and other things) for you. Without proper logical ordering of the files in a patch, review is more difficult. I'm going to quote your hunks out of posting order (i.e., I'll quote them in logical order). > diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec > index dd182c02fdf6..cb49ef50b864 100644 > --- a/MdeModulePkg/MdeModulePkg.dec > +++ b/MdeModulePkg/MdeModulePkg.dec > @@ -994,6 +994,14 @@ [PcdsFixedAtBuild] > # @ValidList 0x80000006 | 0x03058002 > gEfiMdeModulePkgTokenSpaceGuid.PcdErrorCodeSetVariable|0x03058002|UINT32|0x30001040 > > + ## Delayed Dispatch Maximum Delay in us (microseconds) > + # Maximum delay for any particular delay request - 5 seconds > + gEfiMdeModulePkgTokenSpaceGuid.PcdDelayedDispatchMaxDelayUs|5000000|UINT32|0x3000104A > + > + ## Delayed Dispatch timeout in us (microseconds) > + # Maximum delay when waiting for completion (ie EndOfPei) - 10 seconds > + gEfiMdeModulePkgTokenSpaceGuid.PcdDelayedDispatchCompletionTimeoutUs|10000000|UINT32|0x3000104B > + > ## Mask to control the NULL address detection in code for different phases. > # If enabled, accessing NULL address in UEFI or SMM code can be caught.

> # BIT0 - Enable NULL pointer detection for UEFI.
Making these fixed-at-build PCDs is not optimal; more on that later. > diff --git a/MdeModulePkg/Core/Pei/PeiMain.inf b/MdeModulePkg/Core/Pei/PeiMain.inf > index 0cf357371a16..b2c1e53949de 100644 > --- a/MdeModulePkg/Core/Pei/PeiMain.inf > +++ b/MdeModulePkg/Core/Pei/PeiMain.inf > @@ -66,6 +66,7 @@ [LibraryClasses] > PeCoffLib > PeiServicesTablePointerLib > PcdLib > + TimerLib > > [Guids] > gPeiAprioriFileNameGuid ## SOMETIMES_CONSUMES ## File > @@ -100,6 +101,8 @@ [Ppis] > gEfiPeiReset2PpiGuid ## SOMETIMES_CONSUMES > gEfiSecHobDataPpiGuid ## SOMETIMES_CONSUMES > gEfiPeiCoreFvLocationPpiGuid ## SOMETIMES_CONSUMES > + gEfiPeiDelayedDispatchPpiGuid ## PRODUCES > + gEfiEndOfPeiSignalPpiGuid ## CONSUMES > > [Pcd] > gEfiMdeModulePkgTokenSpaceGuid.PcdPeiCoreMaxPeiStackSize ## CONSUMES > @@ -112,6 +115,8 @@ [Pcd] > gEfiMdeModulePkgTokenSpaceGuid.PcdShadowPeimOnBoot ## CONSUMES > gEfiMdeModulePkgTokenSpaceGuid.PcdInitValueInTempStack ## CONSUMES > gEfiMdeModulePkgTokenSpaceGuid.PcdMigrateTemporaryRamFirmwareVolumes ## CONSUMES > + gEfiMdeModulePkgTokenSpaceGuid.PcdDelayedDispatchMaxDelayUs ## CONSUMES > + gEfiMdeModulePkgTokenSpaceGuid.PcdDelayedDispatchCompletionTimeoutUs ## CONSUMES > > # [BootMode] > # S3_RESUME ## SOMETIMES_CONSUMES OK, end-of-PEI-signal is produced by the DXE IPL PEIM (except on the S3 resume path), and by S3Resume2Pei (on the S3 boot path). > diff --git a/MdeModulePkg/Core/Pei/PeiMain.h b/MdeModulePkg/Core/Pei/PeiMain.h > index 556beddad533..542b680c099c 100644 > --- a/MdeModulePkg/Core/Pei/PeiMain.h > +++ b/MdeModulePkg/Core/Pei/PeiMain.h > @@ -11,6 +11,8 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > > #include > #include > +#include > +#include > #include > #include > #include > @@ -41,6 +43,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent > #include > #include > #include > +#include > #include > #include > #include > @@ -207,6 +210,29 @@ EFI_STATUS > > #define PEI_CORE_HANDLE_SIGNATURE SIGNATURE_32('P','e','i','C') > > +#define GET_TIME_IN_US() ((UINT32)DivU64x32(GetTimeInNanoSecond(GetPerformanceCounter ()), 1000)) This is wrong; it violates *both* the GetPerformanceCounter() *and* the GetTimeInNanoSecond() TimerLib API contracts. - GetPerformanceCounter() may return tick values in an increasing or decreasing order, - GetPerformanceCounter() may wrap around (in the direction that it is counting in), - the interval from which GetPerformanceCounter() returns tick values may not be based on zero. These characteristics can be retrieved with GetPerformanceCounterProperties(). Furthermore, GetTimeInNanoSecond() takes a parameter of *elapsed* ticks (that is, a difference between tick values); it cannot be used on raw GetPerformanceCounter() output, in the general case. In August 2017, I wrote a library called TimerTickDiffLib, and proposed it for MdePkg: http://mid.mail-archive.com/8cba2a58-1333-7733-031d-0883dbd844c6@redhat.com The core of that library is the GetTickDifference() function, which calculates the elapsed tick count (a difference of tick values) accounting for both wrap-around and counting direction. The result of such a circumspect subtraction can be then passed to GetTimeInNanoSecond(). However... that isn't even the main issue here. The main issue with this patch is that it attempts to add such scheduling to the PEI core that is based on an *absolute*, *non-wrapping* time scale. That cannot work, because in PEI, we have no Real Time (or at least Monotonic Time) abstraction. In PI / UEFI, there is EFI_REAL_TIME_CLOCK_ARCH_PROTOCOL. (Side comment: that protocol is installed on a handle with a NULL interface pointer -- the driver producing EFI_REAL_TIME_CLOCK_ARCH_PROTOCOL is supposed to set gRT->GetTime, gRT->SetTime, gRT->GetWakeupTime and gRT->SetWakeupTime directly.) However, as far as I know, there is no such abstraction in PEI. One PPI that comes close is the Required Additional PPI EFI_PEI_STALL_PPI -- but the Delayed Dispatch PPI is supposed to supersede *exactly that*. So for implementing the Delayed Dispatch PPI, there are two options: - either introduce a RealTime PPI, require platforms to provide it, and base the Delayed Dispatch PPI on that, - or build an absolute timescale-based dispatcher in the PEI core (for the Delayed Dispatch PPI) *manually*. The present patch attempts to do the second one, but in a wrong way. The GET_TIME_IN_US() *usage* (calls) actually seem sensible (from a superficial skim), but GET_TIME_IN_US() is not implemented correctly at all. A correct implementation has to: - initially call GetPerformanceCounterProperties(), - periodically (frequently) call GetPerformanceCounter(), - each time GetPerformanceCounter() is called, calculate the tick *difference* (= elapsed tick count) relative to the last call, using logic like my function GetTickDifference() -- this relies on the performance counter properties fetched earlier, - translate the tick difference to nanoseconds with GetTimeInNanoSecond() --> this gives us a *time* difference, - *accumulate* the time difference into a monotonically increasing "time" global variable that is internal to the PEI Core, - use this "time" variable as the basis for scheduling delayed events (calculating timestamps into the future, and comparing timestamps against the current time). If you want an absolute time-based scheduler, this is the way. Now, you may want a *remaining time*-based scheduler. In my opinion, that would be easier. Here's how that might look like: - Whenever a callback is scheduled, don't try to calculate its absolute timestamp after which it should "fire". Instead, only store the *remaining* time left, for the callback. - The way to fetch and calculate small time advances is identical to the above (i.e., GetPerformanceCounter(), GetTickDifference(), GetTimeInNanoSecond()). - Rather than accumulating the most recent time difference in each iteration, *subtract* the time difference from *every pending* event's remaining time. Whichever fall down to, or below, zero, should "fire". In the absolute approach, you move the global "time" forward, and compare it against absolute timestamps. (An extra complication with the absolute time / timestamp approach is that, as time progresses, your "time" variable may grow (theoretically) without bounds. However, this complication turns out to be academic. If your time variable counts in microseconds and is based at zero, then 2^64-1 microseconds amount to approximately 584,942 years by my count; that's probably enough for any platform's PEI phase to complete. :) ) In the remaining time approach, you shrink all the remaining times together, and compare them against zero. With a naive data structure, such as an unordered array or list, a linear scanning of all pending events is necessary in both approaches. With a more advanced data structure, such as a priority queue, the absolute time approach is faster, because once you hit an event at the front of the queue whose "time has not come yet", you know you can stop scanning, whereas with the remaining time approach, you still need to touch all the other events, for subtracting the elapsed number of nanoseconds. With only 8 events allowed for pending at any time, a simple array should be plenty good, even with the remaining times (i.e., subtraction) approach. Either way, the current patch is inappropriate. It could lead to lost notifications or other timing misbehavior, because it violates the TimerLib API contracts. So, either stick with the absolute idea (GET_TIME_IN_US), but implement it properly, or else switch to the "remaining time" (= subtractive) approach. And then I can get to why the PCDs you propose above are not optimal, in my opinion. Both time scales above (absolute versus remaining) rest on time differences, or put more directly, on elapsed tick counts. Calculating an elapsed tick count is only possible if the performance counter wraps around *at most once* -- that is, zero times, or one time. If it wraps around *at least twice*, then a unique elapsed tick count cannot be calculated. For example, the performance counter may wrap around three times, but we may only perceive that the value jumped from near the high-end near the low-end, and therefore decide that the elapsed tick count is relatively small. This further leads us to decide that there is more time to wait before a pending event should be triggered -- meanwhile, that event may have *seriously* timed out! So the reason I frown at the PCDs is that, if you put the PEI Core to *sleep* for such long times (because all events are that far out in the future, like 4.5 seconds!), then the performance counter may easily wrap around multiple times while the PEI Core is sleeping. However... now I realize that the PEI Core actually never sleeps, and the timeouts are checked in every iteration of the PEI Dispatcher -- it's effectively a busy loop. Knowing that, I agree that the PCDs are actually safe. One catch remains though. If a PEIM takes too long before it returns to the dispatcher -- for example because it interacts with a a slow device, or manipulates a lot of data in RAM, or because it calls EFI_PEI_STALL_PPI.Stall() with a very high number of microseconds --, then the performance counter may wrap around twice or more, again. Therefore I think the EFI_PEI_STALL_PPI specification should preferably be extended to warn against long delays. (Similarly to how the UEFI spec recommends doing any work at the lowest possible TPL for that kind of work.) Okay, summary: - I think the PCDs are viable, after all - GET_TIME_IN_US() is incorrectly implemented; you certainly need something like GetTickDifference from my patch (or just write it from scratch), and then either rewrite GET_TIME_IN_US(), accumulating time differences, or replace it altogether with a "remaining time" approach. - Represent microsecond counts in UINT64. The outermost UINT32 cast is unsettling. 2^32-1 microseconds is approximately 72 minutes. That makes me very uncomfortable. Let us not shoot ourselves in the foot like that. (I figure a 32-bit microsecond resolution might work if you used the "remaining time" approach.) > + > +// > +// Internal structure for delayed dispatch entries. > +// > +#pragma pack (push, 1) > + > +typedef struct { > + EFI_GUID UniqueId; > + UINT64 Context; > + EFI_DELAYED_DISPATCH_FUNCTION Function; > + UINT32 DispatchTime; "DispatchTime" should be UINT64, per above. > + UINT32 MicrosecondDelay; As far as I can tell, from the rest of the code, storing this field / value permanantly is superfluous; it is a waste of HOB space. > +} DELAYED_DISPATCH_ENTRY; > + > +typedef struct { > + UINT32 Count; > + UINT32 DispCount; > + DELAYED_DISPATCH_ENTRY Entry[1]; // Actual size based on usage, and cannot exceed DELAYED_DISPATCH_MAX_ENTRIES; > +} DELAYED_DISPATCH_TABLE; > + > +#pragma pack (pop) > + > /// > /// Pei Core private data structure instance > /// - I guess these structures are packed because they're going to be stored in a HOB, and we're trying to conserve space. OK, but please document that reason here. - Using the "struct hack", that is, declaring the Entry array with 1 element, is *both* a wart, *and* unnecessary here. One, it is a wart because the "struct hack" requires over-indexing the array, and that is undefined behavior. (In C99, we have the "flexible array member", such as Entry[]; which is well defined.) Two, it is unnecessary because you only intend to permit 8 entries in the array (see DELAYED_DISPATCH_MAX_ENTRIES below), so all the "dynamic" sizing and indexing is just overkill. I recommend hoisting the DELAYED_DISPATCH_MAX_ENTRIES macro to the header file as well, and declaring this field as Entry[DELAYED_DISPATCH_MAX_ENTRIES]. > @@ -307,6 +333,11 @@ struct _PEI_CORE_INSTANCE { > // Those Memory Range will be migrated into physical memory. > // > HOLE_MEMORY_DATA HoleData[HOLE_MAX_NUMBER]; > + > + // > + // Table of delayed dispatch requests > + // > + DELAYED_DISPATCH_TABLE *DelayedDispatchTable; > }; > > /// > @@ -2038,4 +2069,49 @@ PeiReinitializeFv ( > IN PEI_CORE_INSTANCE *PrivateData > ); > > +/** > +Register a callback to be called after a minimum delay has occurred. > + > +This service is the single member function of the EFI_DELAYED_DISPATCH_PPI > + Wrong indentation. > + @param[in] This Pointer to the EFI_DELAYED_DISPATCH_PPI instance > + @param[in] Function Function to call back > + @param[in] Context Context data > + @param[in] UniqueId GUID for this Delayed Dispatch request. > + @param[in] Delay Delay interval > + > + @retval EFI_SUCCESS Function successfully loaded > + @retval EFI_INVALID_PARAMETER One of the Arguments is not supported > + @retval EFI_OUT_OF_RESOURCES No more entries > + > +**/ > +EFI_STATUS > +EFIAPI > +PeiDelayedDispatchRegister ( > + IN EFI_DELAYED_DISPATCH_PPI *This, > + IN EFI_DELAYED_DISPATCH_FUNCTION Function, > + IN UINT64 Context, > + IN EFI_GUID *UniqueId OPTIONAL, > + IN UINT32 Delay > + ); > + > +/** > + Wait on a registered Delayed Dispatch unit that has a UniqueId. Continue > + to dispatch all registered delayed dispatch entries until *ALL* entries with > + UniqueId have completed. > + > + @param[in] This The Delayed Dispatch PPI pointer. > + @param[in] UniqueId UniqueId of delayed dispatch entry. > + > + @retval EFI_SUCCESS The operation succeeds. > + @retval EFI_INVALID_PARAMETER The parameters are invalid. > + > +**/ > +EFI_STATUS > +EFIAPI > +PeiDelayedDispatchWaitOnUniqueId ( > + IN EFI_DELAYED_DISPATCH_PPI *This, > + IN EFI_GUID *UniqueId > + ); > + > #endif Well, calling the GUID "UniqueId" is a bug in the PI spec then -- it is not supposed to be unique at all! It's similar to a UEFI event *group* ID. Anyway. > > diff --git a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c > index 5f32ebb560ae..f7f4cce84174 100644 > --- a/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c > +++ b/MdeModulePkg/Core/Pei/Dispatcher/Dispatcher.c > @@ -3,14 +3,354 @@ > > Copyright (c) 2006 - 2019, Intel Corporation. All rights reserved.
> (C) Copyright 2016 Hewlett Packard Enterprise Development LP
> +Copyright (c) Microsoft Corporation. > SPDX-License-Identifier: BSD-2-Clause-Patent > > **/ > > #include "PeiMain.h" > > +// > +// Maximal number of Delayed Dispatch entries supported > +// > +#define DELAYED_DISPATCH_MAX_ENTRIES 8 > + > +// > +// Utility global variables > +// > + > +// Delayed Dispatch table GUID > +EFI_GUID gEfiDelayedDispatchTableGuid = { > + 0x4b733449, 0x8eff, 0x488c, { 0x92, 0x1a, 0x15, 0x4a, 0xda, 0x25, 0x18, 0x07 } > +}; > + This is non-idiomatic. Even if we don't mean this HOB GUID for public consumption, GUID definitions like this belong in the package DEC file. > +/** > + DelayedDispatchDispatcher > + > + ayed Dispach cycle (ie one pass) through each entry, calling functions when their > + e has expired. When UniqueId is specified, if there are any of the specified entries > + the dispatch queue during dispatch, repeat the DelayedDispatch cycle. Three vertical columns, on the left hand side of this comment block, are missing. > + > + @param DelayedDispatchTable Pointer to dispatch table > + @param OPTIONAL UniqueId used to insure particular time is met. > + > + @return BOOLEAN > +**/ > +BOOLEAN > +DelayedDispatchDispatcher ( > + IN DELAYED_DISPATCH_TABLE *DelayedDispatchTable, > + IN EFI_GUID *UniqueId OPTIONAL > + ); > + > +/** > + DelayedDispatch End of PEI callback function. Insure that all of the delayed dispatch > + entries are complete before exiting PEI. > + > + @param[in] PeiServices - Pointer to PEI Services Table. > + @param[in] NotifyDesc - Pointer to the descriptor for the Notification event that > + caused this function to execute. > + @param[in] Ppi - Pointer to the PPI data associated with this function. > + > + @retval EFI_STATUS - Always return EFI_SUCCESS > +**/ > +EFI_STATUS > +EFIAPI > +PeiDelayedDispatchOnEndOfPei ( > + IN EFI_PEI_SERVICES **PeiServices, > + IN EFI_PEI_NOTIFY_DESCRIPTOR *NotifyDesc, > + IN VOID *Ppi > + ); > + > +EFI_DELAYED_DISPATCH_PPI mDelayedDispatchPpi = { PeiDelayedDispatchRegister, PeiDelayedDispatchWaitOnUniqueId }; > +EFI_PEI_PPI_DESCRIPTOR mDelayedDispatchDesc = { > + (EFI_PEI_PPI_DESCRIPTOR_PPI | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST), > + &gEfiPeiDelayedDispatchPpiGuid, > + &mDelayedDispatchPpi > +}; > + > +EFI_PEI_NOTIFY_DESCRIPTOR mDelayedDispatchNotifyDesc = { > + EFI_PEI_PPI_DESCRIPTOR_NOTIFY_CALLBACK | EFI_PEI_PPI_DESCRIPTOR_TERMINATE_LIST, > + &gEfiEndOfPeiSignalPpiGuid, > + PeiDelayedDispatchOnEndOfPei > +}; > + > +/** > + Helper function to look up DELAYED_DISPATCH_TABLE published in HOB. > + > + @return Pointer to DELAYED_DISPATCH_TABLE from HOB > +**/ > +DELAYED_DISPATCH_TABLE * > +GetDelayedDispatchTable ( > + VOID > + ) > +{ > + EFI_HOB_GUID_TYPE *GuidHob; > + > + GuidHob = GetFirstGuidHob (&gEfiDelayedDispatchTableGuid); > + if (NULL == GuidHob) { > + DEBUG ((DEBUG_ERROR, "Delayed Dispatch PPI ERROR - Delayed Dispatch Hob not available.\n")); > + ASSERT (FALSE); > + return NULL; > + } I don't understand either the DEBUG_ERROR here, *or* the ASSERT(). All call sites of GetDelayedDispatchTable() check the return value against NULL, and return EFI_UNSUPPORTED outwards, if the retval is NULL. I.e., those call sites seem careful enough, so the ASSERT() here is uncalled for. > + > + return (DELAYED_DISPATCH_TABLE *)GET_GUID_HOB_DATA (GuidHob); > +} > + > +/** > + Register a callback to be called after a minimum delay has occurred. > + > + This service is the single member function of the EFI_DELAYED_DISPATCH_PPI > + > + @param[in] This Pointer to the EFI_DELAYED_DISPATCH_PPI instance > + @param[in] Function Function to call back > + @param[in] Context Context data > + @param[in] UniqueId GUID for this Delayed Dispatch request. > + @param[in] Delay Delay interval > + > + @retval EFI_SUCCESS Function successfully loaded > + @retval EFI_INVALID_PARAMETER One of the Arguments is not supported > + @retval EFI_OUT_OF_RESOURCES No more entries > + > +**/ > +EFI_STATUS > +EFIAPI > +PeiDelayedDispatchRegister ( > + IN EFI_DELAYED_DISPATCH_PPI *This, > + IN EFI_DELAYED_DISPATCH_FUNCTION Function, > + IN UINT64 Context, > + IN EFI_GUID *UniqueId OPTIONAL, > + IN UINT32 Delay > + ) > +{ > + DELAYED_DISPATCH_TABLE *DelayedDispatchTable; > + DELAYED_DISPATCH_ENTRY *Entry; > + > + // Check input parameters > + if ((NULL == Function) || (Delay > FixedPcdGet32 (PcdDelayedDispatchMaxDelayUs)) || (NULL == This)) { > + DEBUG ((DEBUG_ERROR, "%a Invalid parameter\n", __func__)); Not sure if you really need this DEBUG, but if you keep it, make it more informative, I'd suggest. Log the function name, UniqueId if any, and Delay. > + return EFI_INVALID_PARAMETER; > + } > + > + // Get delayed dispatch table > + DelayedDispatchTable = GetDelayedDispatchTable (); > + if (NULL == DelayedDispatchTable) { > + DEBUG ((DEBUG_ERROR, "%a Unable to locate dispatch table\n", __func__)); > + return EFI_UNSUPPORTED; > + } DEBUG_ERROR superfluous, GetDelayedDispatchTable() already logs an error; *plus* a DEBUG_ERROR here is inconsistent with *other* call sites of GetDelayedDispatchTable(), which does not log an error. > + > + // Check for available entry slots > + if (DelayedDispatchTable->Count >= DELAYED_DISPATCH_MAX_ENTRIES) { > + ASSERT (DelayedDispatchTable->Count < DELAYED_DISPATCH_MAX_ENTRIES); > + DEBUG ((DEBUG_ERROR, "%a Too many entries requested\n", __func__)); > + return EFI_OUT_OF_RESOURCES; > + } Wrong for multiple reasons. - Count > MAX is impossible, so if you want to assert something, assert Count <= MAX, *before* the "if". - Accordingly, the condition of the "if" should only check for equality (==), "table full". - the ASSERT() *inside* the "if" is totally bogus; please remove it. Note that, as proposed, the ASSERT()'s condition is the exact opposite of the "if"'s condition, so once you enter the "if" body, the ASSERT is sure to fire! - Asserting that Count be *strictly smaller* than MAX is wrong in itself. The caller may well call this function with the table full (8 entries), which will trigger the ASSERT(). But such a call is not an invariant violation! Simply return EFI_OUT_OF_RESOURCES. In short: ASSERT (DelayedDispatchTable->Count <= DELAYED_DISPATCH_MAX_ENTRIES); if (DelayedDispatchTable->Count == DELAYED_DISPATCH_MAX_ENTRIES) { DEBUG ((DEBUG_ERROR, "%a Too many entries requested\n", __func__)); return EFI_OUT_OF_RESOURCES; } > + > + Entry = &(DelayedDispatchTable->Entry[DelayedDispatchTable->Count]); The parens are weird. > + Entry->Function = Function; > + Entry->Context = Context; > + Entry->DispatchTime = GET_TIME_IN_US () + Delay; Independently of everything else said thus far, you should please use SafeIntLib for all timestamp calculations in this patch. That's why SafeIntLib exists. > + if (NULL != UniqueId) { Personally I love Yoda conditionals, but in edk2, they are not idiomatic. Also, a *negative* comparison in such an "if" that has an "else" branch is questionable style. Rather use the positive comparison, and swap the branches around. > + CopyGuid (&Entry->UniqueId, UniqueId); > + } else { > + ZeroMem (&Entry->UniqueId, sizeof (EFI_GUID)); > + } > + > + Entry->MicrosecondDelay = Delay; > + DelayedDispatchTable->Count++; > + > + DEBUG ((DEBUG_INFO, "%a Adding dispatch Entry\n", __func__)); > + DEBUG ((DEBUG_INFO, " Requested Delay = %d\n", Delay)); "Delay" is UINT32, please log it with %u. > + DEBUG ((DEBUG_INFO, " Trigger Time = %d\n", Entry->DispatchTime)); "DispatchTime" should be UINT64 (per above), so the right way to log it is %Lu. > + DEBUG ((DEBUG_INFO, " Context = 0x%08lx\n", Entry->Context)); "Context" is UINT64, so %lx is fine, but why 8 for the minimum width rather than 16? > + DEBUG ((DEBUG_INFO, " Function = %p\n", Entry->Function)); %p is for logging a pointer-to-void, not a pointer-to-function. The pristine way is this: DEBUG ((DEBUG_INFO, " Function = %Lx\n", (UINT64)(UINTN)Entry->Function)); because: - the funcptr can be converted to an integer, per ISO C, - UINTN has the same width as our function pointers, so no complaints from compilers, and all function addresses can be represented in UINTN - printing a UINTN portably requires us to cast it to UINT64 (a no-op on 64-bit platforms and an actual widening on 32-bit platforms), because that way we can print the value with the *common* format specifier %Lx. > + DEBUG ((DEBUG_INFO, " GuidHandle = %g\n", &(Entry->UniqueId))); - The parens are weird. - The parameter is called "UniqueI", why log it as "GuidHandle"? > + > + if (0 == Delay) { More Yoda. > + // Force early dispatch point > + DelayedDispatchDispatcher (DelayedDispatchTable, NULL); > + } > + > + return EFI_SUCCESS; > +} (I can't even look at the higher logic at this point.) > + > +/** > + DelayedDispatchDispatcher > + > + Delayed Dispach cycle (ie one pass) through each entry, calling functions when their > + time has expired. When UniqueId is specified, if there are any of the specified entries > + in the dispatch queue during dispatch, repeat the DelayedDispatch cycle. > + > + @param DelayedDispatchTable Pointer to dispatch table > + @param OPTIONAL UniqueId used to insure particular time is met. > + > + @return BOOLEAN > +**/ > +BOOLEAN > +DelayedDispatchDispatcher ( > + IN DELAYED_DISPATCH_TABLE *DelayedDispatchTable, > + IN EFI_GUID *UniqueId OPTIONAL > + ) > +{ > + BOOLEAN Dispatched; > + UINT32 TimeCurrent; > + UINT32 MaxDispatchTime; > + UINTN Index1; > + BOOLEAN UniqueIdPresent; > + DELAYED_DISPATCH_ENTRY *Entry; > + > + Dispatched = FALSE; > + UniqueIdPresent = TRUE; > + MaxDispatchTime = GET_TIME_IN_US () + FixedPcdGet32 (PcdDelayedDispatchCompletionTimeoutUs); > + while ((DelayedDispatchTable->Count > 0) && (UniqueIdPresent)) { > + UniqueIdPresent = FALSE; > + DelayedDispatchTable->DispCount++; > + > + // If dispatching is messed up, clear DelayedDispatchTable and exit. > + TimeCurrent = GET_TIME_IN_US (); > + if (TimeCurrent > MaxDispatchTime) { > + DEBUG ((DEBUG_ERROR, "%a - DelayedDispatch Completion timeout!\n", __func__)); > + ReportStatusCode ((EFI_ERROR_MAJOR | EFI_ERROR_CODE), (EFI_SOFTWARE_PEI_CORE | EFI_SW_EC_ABORTED)); > + ASSERT (FALSE); > + DelayedDispatchTable->Count = 0; > + break; > + } > + > + // Check each entry in the table for possible dispatch > + for (Index1 = 0; Index1 < DelayedDispatchTable->Count;) { > + Entry = &(DelayedDispatchTable->Entry[Index1]); > + // If UniqueId is present, insure there is an additional check of the table. > + if (NULL != UniqueId) { > + if (CompareGuid (UniqueId, &Entry->UniqueId)) { > + UniqueIdPresent = TRUE; > + } > + } > + > + TimeCurrent = GET_TIME_IN_US (); > + if (TimeCurrent >= Entry->DispatchTime) { > + // Time expired, invoked the function > + DEBUG (( > + DEBUG_ERROR, > + "Delayed dispatch entry %d @ %p, Target=%d, Act=%d Disp=%d\n", > + Index1, > + Entry->Function, > + Entry->DispatchTime, > + TimeCurrent, > + DelayedDispatchTable->DispCount > + )); > + Dispatched = TRUE; > + Entry->MicrosecondDelay = 0; > + Entry->Function ( > + &Entry->Context, > + &Entry->MicrosecondDelay > + ); > + DEBUG ((DEBUG_ERROR, "Delayed dispatch Function returned delay=%d\n", Entry->MicrosecondDelay)); > + if (0 == Entry->MicrosecondDelay) { > + // NewTime = 0 = delete this entry from the table > + DelayedDispatchTable->Count--; > + CopyMem (Entry, Entry+1, sizeof (DELAYED_DISPATCH_ENTRY) * (DelayedDispatchTable->Count - Index1)); > + } else { > + if (Entry->MicrosecondDelay > FixedPcdGet32 (PcdDelayedDispatchMaxDelayUs)) { > + DEBUG ((DEBUG_ERROR, "%a Illegal new delay %d requested\n", __func__, Entry->MicrosecondDelay)); > + ASSERT (FALSE); > + Entry->MicrosecondDelay = FixedPcdGet32 (PcdDelayedDispatchMaxDelayUs); > + } > + > + // NewTime != 0 - update the time from us to Dispatch time > + Entry->DispatchTime = GET_TIME_IN_US () + Entry->MicrosecondDelay; > + Index1++; > + } > + } else { > + Index1++; > + } > + } > + } > + > + return Dispatched; > +} Side comment: I have now filed , because the PI v1.8 specification includes EFI_DELAYED_DISPATCH_PPI in the wrong volume (and there are some other issues with it, too). My main point: this function, DelayedDispatchDispatcher() does not do *at all* what it is supposed to do, per spec. Note that PeiDelayedDispatchWaitOnUniqueId() -- below -- is the function that we expose as "EFI_DELAYED_DISPATCH_PPI.WaitOnEvent" (the WaitOnEvent member function is introduced in patch#2 of this series). In turn, PeiDelayedDispatchWaitOnUniqueId() delegates the bulk of the work to this function, DelayedDispatchDispatcher(). Therefore, the PI spec's language *applies* to this function. Let me quote the spec: | WaitOnEvent | | Pause current process and allow other components to continue to | dispatch until all entries with specified “Unique ID” have | completed. | | [...] | | Summary | | Pause current process and allow other components to continue to | dispatch until all entries with specified “Unique ID” have | completed. | | [...] | | Description | | This function is invoked by a PEIM to wait until all specified | UniqueId events have been dispatched. The other events will continue | to dispatch while this process is being paused. But that's not what DelayedDispatchDispatcher() implements at all. DelayedDispatchDispatcher() implements a busy loop, one that spins until all entries marked with the GUID "time out" (i.e., can be invoked and dequeued). There is a safety time limit on this busy loop (PcdDelayedDispatchCompletionTimeoutUs), but that's irrelevant now. The problem is that this busy loop does not transfer control to "other events" or "other components" *at all*. The PEI Dispatcher has no chance to load further PEIMs, and to notify waiters if new PPIs are installed. So I don't understand what this function tries to do. It does not do what the spec says it should do. Per spec, this function should reactivate the PEI Core Dispatcher loop, via direct call or longjump (see my earlier questions under this patch series), and allow the PEI Core Dispatcher loop to proceed with *both* the loading of independent PEIMs *and* the delayed dispatch functions. ... Even if we assume this function does the right thing (which I don't believe it does), there are a number of warts in it: - The DispCount field seems to count the number of iterations around the *outermost* loops. What is that good for? This field is only consumed in some DEBUG messages. The field certainly doesn't count the number of *actually dispatching* a queued function. - Incorrect format specifiers in DEBUG messages. - The whole "dispatching is messed up" branch, with an ASSERT(FALSE) in it. I figure that is meant to defend against a situation when some delayed function keeps re-queueing itself, and so the outer loop just never ends. But this circumstance is not described in the PI spec at all, and handling it with an ASSERT(FALSE) is certainly a recipe for disaster. More precisely, handling it differently between DEBUG and RELEASE builds is a disaster. In DEBUG builds, execution will stop, and you'll know something is wrong with your PEI phase. In RELEASE builds however, the caller will see EFI_DELAYED_DISPATCH_PPI.WaitOnEvent() return as if everything were fine (i.e., as if all delayed functions had completed), and happily proceed to consume potentially uninitialized memory, used potentially uninitialized devices, and... hope for the best? Bad. - Issues I pointed out earlier: broken implementation of GET_TIME_IN_US, "MicrosecondDelay" being a superfluous field (wasting HOB space), etc. - More weird and unneeded parens after the address-of operator &. - I may have missed several, because I'm too tired now. > + > +/** > + Wait on a registered Delayed Dispatch unit that has a UniqueId. Continue > + to dispatch all registered delayed dispatch entries until *ALL* entries with > + UniqueId have completed. > + > + @param[in] This The Delayed Dispatch PPI pointer. > + @param[in] UniqueId UniqueId of delayed dispatch entry. > + > + @retval EFI_SUCCESS The operation succeeds. > + @retval EFI_INVALID_PARAMETER The parameters are invalid. > + > +**/ > +EFI_STATUS > +EFIAPI > +PeiDelayedDispatchWaitOnUniqueId ( > + IN EFI_DELAYED_DISPATCH_PPI *This, > + IN EFI_GUID *UniqueId > + ) > +{ > + PERF_FUNCTION_BEGIN (); > + DELAYED_DISPATCH_TABLE *DelayedDispatchTable; > + > + // Get delayed dispatch table > + DelayedDispatchTable = GetDelayedDispatchTable (); > + if (NULL == DelayedDispatchTable) { > + PERF_FUNCTION_END (); > + return EFI_UNSUPPORTED; > + } > + > + if ((NULL == UniqueId) || (IsZeroGuid (UniqueId))) { > + ASSERT (FALSE); > + PERF_FUNCTION_END (); > + return EFI_UNSUPPORTED; > + } > + > + DEBUG ((DEBUG_INFO, "Delayed dispatch on %g. Count=%d, DispatchCount=%d\n", UniqueId, DelayedDispatchTable->Count, DelayedDispatchTable->DispCount)); > + PERF_EVENT_SIGNAL_BEGIN (UniqueId); > + DelayedDispatchDispatcher (DelayedDispatchTable, UniqueId); > + PERF_EVENT_SIGNAL_END (UniqueId); > + > + PERF_FUNCTION_END (); > + return EFI_SUCCESS; > +} > + Rather than multiplicating PERF_FUNCTION_END(), this function should have an "Exit:" label, and use gotos, a Status variable, and move PERF_FUNCTION_END after "Exit:". Also, what good is the ASSERT(FALSE) when the PI spec explicitly documents EFI_INVALID_PARAMETER? Even the function leading comment specifies EFI_INVALID_PARAMETER -- but the code returns -- after ASSERT(FALSE)... -- EFI_UNSUPPORTED. > /** > + DelayedDispatch End of PEI callback function. Insure that all of the delayed dispatch > + entries are complete before exiting PEI. > > + @param[in] PeiServices - Pointer to PEI Services Table. > + @param[in] NotifyDesc - Pointer to the descriptor for the Notification event that > + caused this function to execute. > + @param[in] Ppi - Pointer to the PPI data associated with this function. > + > + @retval EFI_STATUS - Always return EFI_SUCCESS > +**/ > +EFI_STATUS > +EFIAPI > +PeiDelayedDispatchOnEndOfPei ( > + IN EFI_PEI_SERVICES **PeiServices, > + IN EFI_PEI_NOTIFY_DESCRIPTOR *NotifyDesc, > + IN VOID *Ppi > + ) > +{ > + DELAYED_DISPATCH_TABLE *DelayedDispatchTable; > + > + // Get delayed dispatch table > + DelayedDispatchTable = GetDelayedDispatchTable (); > + if (NULL == DelayedDispatchTable) { > + return EFI_UNSUPPORTED; > + } > + > + PERF_INMODULE_BEGIN ("PerfDelayedDispatchEndOfPei"); > + while (DelayedDispatchTable->Count > 0) { > + DelayedDispatchDispatcher (DelayedDispatchTable, NULL); > + } > + > + DEBUG ((DEBUG_ERROR, "%a Count of dispatch cycles is %d\n", __func__, DelayedDispatchTable->DispCount)); > + PERF_INMODULE_END ("PerfDelayedDispatchEndOfPei"); > + > + return EFI_SUCCESS; > +} > + > +/** > Discover all PEIMs and optional Apriori file in one FV. There is at most one > Apriori file in one FV. > > @@ -1365,12 +1705,31 @@ PeiDispatcher ( > EFI_PEI_FILE_HANDLE SaveCurrentFileHandle; > EFI_FV_FILE_INFO FvFileInfo; > PEI_CORE_FV_HANDLE *CoreFvHandle; > + EFI_HOB_GUID_TYPE *GuidHob; > + UINT32 TableSize; > > PeiServices = (CONST EFI_PEI_SERVICES **)&Private->Ps; > PeimEntryPoint = NULL; > PeimFileHandle = NULL; > EntryPoint = 0; > > + if (NULL == Private->DelayedDispatchTable) { > + GuidHob = GetFirstGuidHob (&gEfiDelayedDispatchTableGuid); > + if (NULL != GuidHob) { > + Private->DelayedDispatchTable = (DELAYED_DISPATCH_TABLE *)(GET_GUID_HOB_DATA (GuidHob)); > + } else { > + TableSize = sizeof (DELAYED_DISPATCH_TABLE) + ((DELAYED_DISPATCH_MAX_ENTRIES - 1) * sizeof (DELAYED_DISPATCH_ENTRY)); > + Private->DelayedDispatchTable = BuildGuidHob (&gEfiDelayedDispatchTableGuid, TableSize); > + if (NULL != Private->DelayedDispatchTable) { > + ZeroMem (Private->DelayedDispatchTable, TableSize); > + Status = PeiServicesInstallPpi (&mDelayedDispatchDesc); > + ASSERT_EFI_ERROR (Status); > + Status = PeiServicesNotifyPpi (&mDelayedDispatchNotifyDesc); > + ASSERT_EFI_ERROR (Status); > + } > + } > + } > + > if ((Private->PeiMemoryInstalled) && > (PcdGetBool (PcdMigrateTemporaryRamFirmwareVolumes) || > (Private->HobList.HandoffInformationTable->BootMode != BOOT_ON_S3_RESUME) || Terrible ereror handling with ASSERT_EFI_ERROR(). Sorry, this review tired me out; I've been at it for like half a day now, and this patch is extremely immature. I don't think I'd like to review the next version. Laszlo > @@ -1621,6 +1980,13 @@ PeiDispatcher ( > } > } > } > + > + // Dispatch pending delalyed dispatch requests > + if (NULL != Private->DelayedDispatchTable) { > + if (DelayedDispatchDispatcher (Private->DelayedDispatchTable, NULL)) { > + ProcessDispatchNotifyList (Private); > + } > + } > } > > // > diff --git a/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c b/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c > index bf1719d7941a..e5643adf7027 100644 > --- a/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c > +++ b/MdeModulePkg/Core/Pei/PeiMain/PeiMain.c > @@ -277,6 +277,9 @@ PeiCore ( > OldCoreData->TempFileHandles = (EFI_PEI_FILE_HANDLE *)((UINT8 *)OldCoreData->TempFileHandles - OldCoreData->HeapOffset); > } > > + // Force relocating the dispatch table > + OldCoreData->DelayedDispatchTable = NULL; > + > // > // Fixup for PeiService's address > // -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#109494): https://edk2.groups.io/g/devel/message/109494 Mute This Topic: https://groups.io/mt/101865810/7686176 Group Owner: devel+owner@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io] -=-=-=-=-=-=-=-=-=-=-=-