Re: [edk2-devel] [PATCH v2 3/4] MdeModulePkg: PeiMain: Introduce implementation of delayed dispatch

["Laszlo Ersek" <lersek@redhat.com>]

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.wang@intel.com>
> Cc: Dandan Bi <dandan.bi@intel.com>
> Cc: Liming Gao <gaoliming@byosoft.com.cn>
> Cc: Debkumar De <debkumar.de@intel.com>
> Cc: Catharine West <catharine.west@intel.com>
>
> Co-authored-by: Mike Turner <mikeyt@pobox.com>
> Signed-off-by: Kun Qin <kuqin12@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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-