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 <jian.j.w...@intel.com>
> Cc: Dandan Bi <dandan...@intel.com>
> Cc: Liming Gao <gaolim...@byosoft.com.cn>
> Cc: Debkumar De <debkumar...@intel.com>
> Cc: Catharine West <catharine.w...@intel.com>
>
> Co-authored-by: Mike Turner <mik...@pobox.com>
> Signed-off-by: Kun Qin <kuqi...@gmail.com>
> ---
>
> 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.<BR><BR>
> # BIT0 - Enable NULL pointer detection for UEFI.<BR>
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 <PiPei.h>
> #include <Ppi/DxeIpl.h>
> +#include <Ppi/DelayedDispatch.h>
> +#include <Ppi/EndOfPeiPhase.h>
> #include <Ppi/MemoryDiscovered.h>
> #include <Ppi/StatusCode.h>
> #include <Ppi/Reset.h>
> @@ -41,6 +43,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> #include <IndustryStandard/PeImage.h>
> #include <Library/PeiServicesTablePointerLib.h>
> #include <Library/MemoryAllocationLib.h>
> +#include <Library/TimerLib.h>
> #include <Guid/FirmwareFileSystem2.h>
> #include <Guid/FirmwareFileSystem3.h>
> #include <Guid/AprioriFileName.h>
> @@ -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.<BR>
> (C) Copyright 2016 Hewlett Packard Enterprise Development LP<BR>
> +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
<https://mantis.uefi.org/mantis/view.php?id=2424>, 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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
