Re: [PATCH V2 4/4] MdeModulePkg/DxeMain: Implement common event protocol

["Gao, Zhichao" <zhichao.gao@intel.com>]

> -----Original Message-----
> From: Wang, Jian J
> Sent: Tuesday, July 9, 2019 5:24 PM
> To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
> Cc: Sean Brogan <sean.brogan@microsoft.com>; Wu, Hao A
> <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Zeng, Star
> <star.zeng@intel.com>; Gao, Liming <liming.gao@intel.com>; Michael Turner
> <Michael.Turner@microsoft.com>; Bret Barkelew
> <Bret.Barkelew@microsoft.com>
> Subject: RE: [PATCH V2 4/4] MdeModulePkg/DxeMain: Implement common
> event protocol
> 
> Hi Zhichao,
> 
> One common comment: please update copy right year.

I got the copyright remind before. Refer to https://edk2.groups.io/g/devel/topic/31828852#41576.
This patch is almost provided from MU project and contributed by Microsoft developers, 'From:...' is the author. And they didn't add their copyright. So I think it is fine to keep the copyright.

> 
> See another comment at the almost the end of the email.
> 
> > -----Original Message-----
> > From: Gao, Zhichao
> > Sent: Tuesday, July 09, 2019 4:40 PM
> > To: devel@edk2.groups.io
> > Cc: Sean Brogan <sean.brogan@microsoft.com>; Wang, Jian J
> > <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray
> > <ray.ni@intel.com>; Zeng, Star <star.zeng@intel.com>; Gao, Liming
> > <liming.gao@intel.com>; Michael Turner
> <Michael.Turner@microsoft.com>;
> > Bret Barkelew <Bret.Barkelew@microsoft.com>
> > Subject: [PATCH V2 4/4] MdeModulePkg/DxeMain: Implement common
> event
> > protocol
> >
> > From: Sean Brogan <sean.brogan@microsoft.com>
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1400
> >
> > If an interrupt happens between CheckEvent and gIdleLoopEvent, there
> > would be a event pending during cpu sleep. So it is required to check
> > the gEventPending with the interrupt disabled.
> >
> > Implement a protocol gEdkiiCommonEventProtocolGuid to support all TPL
> > event for PI drivers that use HW interrput. It has two interface, one
> > is to create event and the other one is to wait for event.
> >
> > Add 'volatile' specifier for gEfiCurrentTpl and gEventPending because
> > they may be changed out of the context (interrupt context).
> >
> > Cc: Jian J Wang <jian.j.wang@intel.com>
> > Cc: Hao A Wu <hao.a.wu@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Star Zeng <star.zeng@intel.com>
> > Cc: Liming gao <liming.gao@intel.com>
> > Cc: Sean Brogan <sean.brogan@microsoft.com>
> > Cc: Michael Turner <Michael.Turner@microsoft.com>
> > Cc: Bret Barkelew <Bret.Barkelew@microsoft.com>
> > Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> > ---
> >  MdeModulePkg/Core/Dxe/DxeMain.h               |  64 ++++++++++-
> >  MdeModulePkg/Core/Dxe/DxeMain.inf             |   2 +
> >  MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c       |  22 ++++
> >  .../Core/Dxe/DxeMain/DxeProtocolNotify.c      |   1 +
> >  MdeModulePkg/Core/Dxe/Event/Event.c           | 102 ++++++++++++++++-
> > -
> >  MdeModulePkg/Core/Dxe/Event/Event.h           |   2 +-
> >  MdeModulePkg/Include/Protocol/Cpu2.h          |   2 +-
> >  7 files changed, 181 insertions(+), 14 deletions(-)
> >
> > diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h
> > b/MdeModulePkg/Core/Dxe/DxeMain.h index 6a64852730..38907bfe8d
> 100644
> > --- a/MdeModulePkg/Core/Dxe/DxeMain.h
> > +++ b/MdeModulePkg/Core/Dxe/DxeMain.h
> > @@ -38,6 +38,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> > #include <Protocol/Security2.h>  #include <Protocol/Reset.h>  #include
> > <Protocol/Cpu.h>
> > +#include <Protocol/Cpu2.h>
> >  #include <Protocol/Metronome.h>
> >  #include <Protocol/FirmwareVolumeBlock.h>  #include
> > <Protocol/Capsule.h> @@ -47,6 +48,7 @@ SPDX-License-Identifier:
> > BSD-2-Clause-Patent  #include <Protocol/HiiPackageList.h>  #include
> > <Protocol/SmmBase2.h>  #include <Protocol/PeCoffImageEmulator.h>
> > +#include <Protocol/CommonEvent.h>
> >  #include <Guid/MemoryTypeInformation.h>  #include
> > <Guid/FirmwareFileSystem2.h>  #include <Guid/FirmwareFileSystem3.h>
> @@
> > -274,10 +276,11 @@ extern EFI_METRONOME_ARCH_PROTOCOL
> *gMetronome;
> >  extern EFI_TIMER_ARCH_PROTOCOL                  *gTimer;
> >  extern EFI_SECURITY_ARCH_PROTOCOL               *gSecurity;
> >  extern EFI_SECURITY2_ARCH_PROTOCOL              *gSecurity2;
> > +extern EDKII_CPU2_PROTOCOL                      *gCpu2;
> >  extern EFI_BDS_ARCH_PROTOCOL                    *gBds;
> >  extern EFI_SMM_BASE2_PROTOCOL                   *gSmmBase2;
> >
> > -extern EFI_TPL                                  gEfiCurrentTpl;
> > +extern volatile EFI_TPL                         gEfiCurrentTpl;
> >
> >  extern EFI_GUID                                 *gDxeCoreFileName;
> >  extern EFI_LOADED_IMAGE_PROTOCOL                *gDxeCoreLoadedImage;
> > @@ -1714,6 +1717,65 @@ CoreCheckEvent (
> >    );
> >
> >
> > +/**
> > +  Stops execution until an event is signaled.
> > +
> > +  Support all TPL.
> > +
> > +  @param  NumberOfEvents         The number of events in the UserEvents
> > array
> > +  @param  UserEvents             An array of EFI_EVENT
> > +  @param  UserIndex              Pointer to the index of the event which
> > +                                 satisfied the wait condition
> > +
> > +  @retval EFI_SUCCESS            The event indicated by Index was signaled.
> > +  @retval EFI_INVALID_PARAMETER  The event indicated by Index has a
> > notification
> > +                                 function or Event was not a valid type
> > +  @retval EFI_UNSUPPORTED        The current TPL is not TPL_APPLICATION
> > +
> > +**/
> > +EFI_STATUS
> > +EFIAPI
> > +CommonWaitForEvent (
> > +  IN UINTN        NumberOfEvents,
> > +  IN EFI_EVENT    *UserEvents,
> > +  OUT UINTN       *UserIndex
> > +  );
> > +
> > +
> > +/**
> > +  Creates an event in a group.
> > +
> > +  Support all TPL.
> > +
> > +  @param  Type                   The type of event to create and its mode and
> > +                                 attributes
> > +  @param  NotifyTpl              The task priority level of event notifications
> > +  @param  NotifyFunction         Pointer to the events notification function
> > +  @param  NotifyContext          Pointer to the notification functions context;
> > +                                 corresponds to parameter "Context" in the
> > +                                 notification function
> > +  @param  EventGroup             GUID for EventGroup if NULL act the same
> > as
> > +                                 gBS->CreateEvent().
> > +  @param  Event                  Pointer to the newly created event if the call
> > +                                 succeeds; undefined otherwise
> > +
> > +  @retval EFI_SUCCESS            The event structure was created
> > +  @retval EFI_INVALID_PARAMETER  One of the parameters has an invalid
> > value
> > +  @retval EFI_OUT_OF_RESOURCES   The event could not be allocated
> > +
> > +**/
> > +EFI_STATUS
> > +EFIAPI
> > +CommonCreateEventEx (
> > +  IN UINT32                   Type,
> > +  IN EFI_TPL                  NotifyTpl,
> > +  IN EFI_EVENT_NOTIFY         NotifyFunction, OPTIONAL
> > +  IN CONST VOID               *NotifyContext, OPTIONAL
> > +  IN CONST EFI_GUID           *EventGroup,    OPTIONAL
> > +  OUT EFI_EVENT               *Event
> > +  );
> > +
> > +
> >  /**
> >    Adds reserved memory, system memory, or memory-mapped I/O
> resources
> > to the
> >    global coherency domain of the processor.
> > diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf
> > b/MdeModulePkg/Core/Dxe/DxeMain.inf
> > index 61161bee28..44f90bcf4b 100644
> > --- a/MdeModulePkg/Core/Dxe/DxeMain.inf
> > +++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
> > @@ -171,6 +171,8 @@
> >    gEfiVariableArchProtocolGuid                  ## CONSUMES
> >    gEfiCapsuleArchProtocolGuid                   ## CONSUMES
> >    gEfiWatchdogTimerArchProtocolGuid             ## CONSUMES
> > +  gEdkiiCommonEventProtocolGuid                 ## SOMETIMES_CONSUMES
> > +  gEdkiiCpu2ProtocolGuid                        ## CONSUMES
> >
> >  [Pcd]
> >
> >
> gEfiMdeModulePkgTokenSpaceGuid.PcdLoadFixAddressBootTimeCodePage
> > Number    ## SOMETIMES_CONSUMES
> > diff --git a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
> > b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
> > index 514d1aa75a..8196d96daa 100644
> > --- a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
> > +++ b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
> > @@ -13,12 +13,18 @@ SPDX-License-Identifier: BSD-2-Clause-Patent  //
> >  EFI_HANDLE                                mDecompressHandle = NULL;
> >
> > +//
> > +//  Common event protocol
> > +//
> > +EFI_HANDLE                                mCommonEventHalde = NULL;
> > +
> >  //
> >  // DXE Core globals for Architecture Protocols  //
> >  EFI_SECURITY_ARCH_PROTOCOL        *gSecurity      = NULL;
> >  EFI_SECURITY2_ARCH_PROTOCOL       *gSecurity2     = NULL;
> >  EFI_CPU_ARCH_PROTOCOL             *gCpu           = NULL;
> > +EDKII_CPU2_PROTOCOL               *gCpu2          = NULL;
> >  EFI_METRONOME_ARCH_PROTOCOL       *gMetronome     = NULL;
> >  EFI_TIMER_ARCH_PROTOCOL           *gTimer         = NULL;
> >  EFI_BDS_ARCH_PROTOCOL             *gBds           = NULL;
> > @@ -211,6 +217,14 @@ EFI_DECOMPRESS_PROTOCOL  gEfiDecompress = {
> >    DxeMainUefiDecompress
> >  };
> >
> > +//
> > +// Common event protocol
> > +//
> > +EDKII_COMMON_EVENT_PROTOCOL gEdkiiCommonEvent = {
> > +  CommonCreateEventEx,
> > +  CommonWaitForEvent
> > +};
> > +
> >  //
> >  // For Loading modules at fixed address feature, the configuration
> > table is to cache the top address below which to load  // Runtime
> > code&boot time code @@ -475,6 +489,14 @@ DxeMain (
> >    //
> >    CoreNotifyOnProtocolInstallation ();
> >
> > +  Status = CoreInstallProtocolInterface (
> > +              &mCommonEventHalde,
> > +              &gEdkiiCommonEventProtocolGuid,
> > +              EFI_NATIVE_INTERFACE,
> > +              &gEdkiiCommonEvent
> > +              );
> > +  ASSERT_EFI_ERROR (Status);
> > +
> >    //
> >    // Produce Firmware Volume Protocols, one for each FV in the HOB list.
> >    //
> > diff --git a/MdeModulePkg/Core/Dxe/DxeMain/DxeProtocolNotify.c
> > b/MdeModulePkg/Core/Dxe/DxeMain/DxeProtocolNotify.c
> > index 29a55d02e6..57d52c250d 100644
> > --- a/MdeModulePkg/Core/Dxe/DxeMain/DxeProtocolNotify.c
> > +++ b/MdeModulePkg/Core/Dxe/DxeMain/DxeProtocolNotify.c
> > @@ -41,6 +41,7 @@ EFI_CORE_PROTOCOL_NOTIFY_ENTRY
> mArchProtocols[] = {
> > EFI_CORE_PROTOCOL_NOTIFY_ENTRY  mOptionalProtocols[] = {
> >    { &gEfiSecurity2ArchProtocolGuid,        (VOID **)&gSecurity2,     NULL,
> > NULL, FALSE },
> >    { &gEfiSmmBase2ProtocolGuid,             (VOID **)&gSmmBase2,      NULL,
> > NULL, FALSE },
> > +  { &gEdkiiCpu2ProtocolGuid,               (VOID **)&gCpu2,          NULL, NULL,
> > FALSE },
> >    { NULL,                                  (VOID **)NULL,            NULL, NULL, FALSE }
> >  };
> >
> > diff --git a/MdeModulePkg/Core/Dxe/Event/Event.c
> > b/MdeModulePkg/Core/Dxe/Event/Event.c
> > index 21db38aaf0..d271d2faf3 100644
> > --- a/MdeModulePkg/Core/Dxe/Event/Event.c
> > +++ b/MdeModulePkg/Core/Dxe/Event/Event.c
> > @@ -14,7 +14,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent  ///
> > /// gEfiCurrentTpl - Current Task priority level  /// -EFI_TPL
> > gEfiCurrentTpl = TPL_APPLICATION;
> > +volatile EFI_TPL  gEfiCurrentTpl = TPL_APPLICATION;
> >
> >  ///
> >  /// gEventQueueLock - Protects the event queues
> > @@ -29,7 +29,7 @@ LIST_ENTRY      gEventQueue[TPL_HIGH_LEVEL + 1];
> >  ///
> >  /// gEventPending - A bitmask of the EventQueues that are pending
> > ///
> > -UINTN           gEventPending = 0;
> > +volatile UINTN  gEventPending = 0;
> >
> >  ///
> >  /// gEventSignalQueue - A list of events to signal based on
> > EventGroup type @@ -658,16 +658,44 @@ CoreWaitForEvent (
> >    OUT UINTN       *UserIndex
> >    )
> >  {
> > -  EFI_STATUS      Status;
> > -  UINTN           Index;
> > -
> > -  //
> > +   //
> >    // Can only WaitForEvent at TPL_APPLICATION
> >    //
> >    if (gEfiCurrentTpl != TPL_APPLICATION) {
> >      return EFI_UNSUPPORTED;
> >    }
> >
> > +  return CommonWaitForEvent (NumberOfEvents, UserEvents,
> UserIndex);
> > +}
> > +
> > +
> > +/**
> > +  Stops execution until an event is signaled.
> > +
> > +  Support all TPL.
> > +
> > +  @param  NumberOfEvents         The number of events in the UserEvents
> > array
> > +  @param  UserEvents             An array of EFI_EVENT
> > +  @param  UserIndex              Pointer to the index of the event which
> > +                                 satisfied the wait condition
> > +
> > +  @retval EFI_SUCCESS            The event indicated by Index was signaled.
> > +  @retval EFI_INVALID_PARAMETER  The event indicated by Index has a
> > notification
> > +                                 function or Event was not a valid type
> > +  @retval EFI_UNSUPPORTED        The current TPL is not TPL_APPLICATION
> > +
> > +**/
> > +EFI_STATUS
> > +EFIAPI
> > +CommonWaitForEvent (
> > +  IN UINTN        NumberOfEvents,
> > +  IN EFI_EVENT    *UserEvents,
> > +  OUT UINTN       *UserIndex
> > +  )
> > +{
> > +  EFI_STATUS      Status;
> > +  UINTN           Index;
> > +
> >    if (NumberOfEvents == 0) {
> >      return EFI_INVALID_PARAMETER;
> >    }
> > @@ -678,7 +706,7 @@ CoreWaitForEvent (
> >
> >    for(;;) {
> >
> > -    for(Index = 0; Index < NumberOfEvents; Index++) {
> > +    for (Index = 0; Index < NumberOfEvents; Index++) {
> >
> >        Status = CoreCheckEvent (UserEvents[Index]);
> >
> > @@ -693,14 +721,66 @@ CoreWaitForEvent (
> >        }
> >      }
> >
> > -    //
> > -    // Signal the Idle event
> > -    //
> > -    CoreSignalEvent (gIdleLoopEvent);
> > +    if (gCpu != NULL && gCpu2 != NULL) {
> > +      //
> > +      // None of the events checked above are ready/signaled yet,
> > +      // disable interrupts before checking for all pending events
> > +      //
> > +      gCpu->DisableInterrupt (gCpu);
> > +
> > +      if (((UINTN) HighBitSet64 (gEventPending)) > gEfiCurrentTpl) {
> > +        //
> > +        // There are pending events, enable interrupts for these
> > + events to be
> > processed
> > +        //
> > +        gCpu->EnableInterrupt (gCpu);
> > +      } else {
> > +        //
> > +        // No events are pending, enable interrupts, sleep the CPU
> > + and wait
> > for an interrupt
> > +        //
> > +        gCpu2->EnableAndWaitForInterrupt (gCpu2);
> > +      }
> > +    }
> >    }
> >  }
> >
> >
> > +/**
> > +  Creates an event in a group.
> > +
> > +  Support all TPL.
> > +
> > +  @param  Type                   The type of event to create and its mode and
> > +                                 attributes
> > +  @param  NotifyTpl              The task priority level of event notifications
> > +  @param  NotifyFunction         Pointer to the events notification function
> > +  @param  NotifyContext          Pointer to the notification functions context;
> > +                                 corresponds to parameter "Context" in the
> > +                                 notification function
> > +  @param  EventGroup             GUID for EventGroup if NULL act the same
> > as
> > +                                 gBS->CreateEvent().
> > +  @param  Event                  Pointer to the newly created event if the call
> > +                                 succeeds; undefined otherwise
> > +
> > +  @retval EFI_SUCCESS            The event structure was created
> > +  @retval EFI_INVALID_PARAMETER  One of the parameters has an invalid
> > value
> > +  @retval EFI_OUT_OF_RESOURCES   The event could not be allocated
> > +
> > +**/
> > +EFI_STATUS
> > +EFIAPI
> > +CommonCreateEventEx (
> > +  IN UINT32                   Type,
> > +  IN EFI_TPL                  NotifyTpl,
> > +  IN EFI_EVENT_NOTIFY         NotifyFunction, OPTIONAL
> > +  IN CONST VOID               *NotifyContext, OPTIONAL
> > +  IN CONST EFI_GUID           *EventGroup,    OPTIONAL
> > +  OUT EFI_EVENT               *Event
> > +  )
> > +{
> > +  return CoreCreateEventInternal (Type, NotifyTpl, NotifyFunction,
> > NotifyContext, EventGroup, Event);
> > +}
> > +
> > +
> >  /**
> >    Closes an event and frees the event structure.
> >
> > diff --git a/MdeModulePkg/Core/Dxe/Event/Event.h
> > b/MdeModulePkg/Core/Dxe/Event/Event.h
> > index 8141c5003e..050df26ec9 100644
> > --- a/MdeModulePkg/Core/Dxe/Event/Event.h
> > +++ b/MdeModulePkg/Core/Dxe/Event/Event.h
> > @@ -12,7 +12,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> >
> >  #define VALID_TPL(a)            ((a) <= TPL_HIGH_LEVEL)
> > -extern  UINTN                   gEventPending;
> > +extern  volatile UINTN          gEventPending;
> >
> >  ///
> >  /// Set if Event is part of an event group diff --git
> > a/MdeModulePkg/Include/Protocol/Cpu2.h
> > b/MdeModulePkg/Include/Protocol/Cpu2.h
> > index 14464a38c8..81c5c8e424 100644
> > --- a/MdeModulePkg/Include/Protocol/Cpu2.h
> > +++ b/MdeModulePkg/Include/Protocol/Cpu2.h
> > @@ -37,7 +37,7 @@ EFI_STATUS
> >  /// Foundation.
> >  ///
> >  struct _EDKII_CPU2_PROTOCOL {
> > -  EDKII_CPU_ENABLE_AND_WAIT_FOR_INTERRUPT
> > EnableAndForWaitInterrupt;
> > +  EDKII_CPU_ENABLE_AND_WAIT_FOR_INTERRUPT
> > EnableAndWaitForInterrupt;
> >  };
> >
> 
> I think this change should belong to patch 1.

Yes. You are right. I would move the change to 1/4.

Thanks,
Zhichao

> 
> Regards,
> Jian
> 
> >  #endif
> > --
> > 2.21.0.windows.1