From mboxrd@z Thu Jan 1 00:00:00 1970 Authentication-Results: mx.groups.io; dkim=missing; spf=pass (domain: intel.com, ip: 134.134.136.31, mailfrom: zhichao.gao@intel.com) Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by groups.io with SMTP; Tue, 09 Jul 2019 17:24:20 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga002.fm.intel.com ([10.253.24.26]) by orsmga104.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 09 Jul 2019 17:24:18 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.63,472,1557212400"; d="scan'208";a="192871922" Received: from fmsmsx105.amr.corp.intel.com ([10.18.124.203]) by fmsmga002.fm.intel.com with ESMTP; 09 Jul 2019 17:24:18 -0700 Received: from shsmsx108.ccr.corp.intel.com (10.239.4.97) by FMSMSX105.amr.corp.intel.com (10.18.124.203) with Microsoft SMTP Server (TLS) id 14.3.439.0; Tue, 9 Jul 2019 17:24:18 -0700 Received: from shsmsx101.ccr.corp.intel.com ([169.254.1.134]) by SHSMSX108.ccr.corp.intel.com ([169.254.8.174]) with mapi id 14.03.0439.000; Wed, 10 Jul 2019 08:24:16 +0800 From: "Gao, Zhichao" To: "Wang, Jian J" , "devel@edk2.groups.io" CC: Sean Brogan , "Wu, Hao A" , "Ni, Ray" , "Zeng, Star" , "Gao, Liming" , Michael Turner , Bret Barkelew Subject: Re: [PATCH V2 4/4] MdeModulePkg/DxeMain: Implement common event protocol Thread-Topic: [PATCH V2 4/4] MdeModulePkg/DxeMain: Implement common event protocol Thread-Index: AQHVNjH47TFmwglIcEaR/yWwqhRfZqbCAaEwgAD6YvA= Date: Wed, 10 Jul 2019 00:24:15 +0000 Message-ID: <3CE959C139B4C44DBEA1810E3AA6F9000B806D6F@SHSMSX101.ccr.corp.intel.com> References: <20190709083956.13024-1-zhichao.gao@intel.com> <20190709083956.13024-5-zhichao.gao@intel.com> In-Reply-To: Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: zhichao.gao@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable > -----Original Message----- > From: Wang, Jian J > Sent: Tuesday, July 9, 2019 5:24 PM > To: Gao, Zhichao ; devel@edk2.groups.io > Cc: Sean Brogan ; Wu, Hao A > ; Ni, Ray ; Zeng, Star > ; Gao, Liming ; Michael Turner > ; Bret Barkelew > > Subject: RE: [PATCH V2 4/4] MdeModulePkg/DxeMain: Implement common > event protocol >=20 > Hi Zhichao, >=20 > 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. >=20 > See another comment at the almost the end of the email. >=20 > > -----Original Message----- > > From: Gao, Zhichao > > Sent: Tuesday, July 09, 2019 4:40 PM > > To: devel@edk2.groups.io > > Cc: Sean Brogan ; Wang, Jian J > > ; Wu, Hao A ; Ni, Ray > > ; Zeng, Star ; Gao, Liming > > ; Michael Turner > ; > > Bret Barkelew > > Subject: [PATCH V2 4/4] MdeModulePkg/DxeMain: Implement common > event > > protocol > > > > From: Sean Brogan > > > > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1400 > > > > 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 > > Cc: Hao A Wu > > Cc: Ray Ni > > Cc: Star Zeng > > Cc: Liming gao > > Cc: Sean Brogan > > Cc: Michael Turner > > Cc: Bret Barkelew > > Signed-off-by: Zhichao Gao > > --- > > 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 #include #include > > > > +#include > > #include > > #include #include > > @@ -47,6 +48,7 @@ SPDX-License-Identifier: > > BSD-2-Clause-Patent #include #include > > #include > > +#include > > #include #include > > #include > @@ > > -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 UserEvent= s > > array > > + @param UserEvents An array of EFI_EVENT > > + @param UserIndex Pointer to the index of the event whi= ch > > + satisfied the wait condition > > + > > + @retval EFI_SUCCESS The event indicated by Index was sign= aled. > > + @retval EFI_INVALID_PARAMETER The event indicated by Index has a > > notification > > + function or Event was not a valid typ= e > > + @retval EFI_UNSUPPORTED The current TPL is not TPL_APPLICATIO= N > > + > > +**/ > > +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 m= ode and > > + attributes > > + @param NotifyTpl The task priority level of event noti= fications > > + @param NotifyFunction Pointer to the events notification fu= nction > > + @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 s= ame > > 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 =3D NULL; > > > > +// > > +// Common event protocol > > +// > > +EFI_HANDLE mCommonEventHalde =3D NULL; > > + > > // > > // DXE Core globals for Architecture Protocols // > > EFI_SECURITY_ARCH_PROTOCOL *gSecurity =3D NULL; > > EFI_SECURITY2_ARCH_PROTOCOL *gSecurity2 =3D NULL; > > EFI_CPU_ARCH_PROTOCOL *gCpu =3D NULL; > > +EDKII_CPU2_PROTOCOL *gCpu2 =3D NULL; > > EFI_METRONOME_ARCH_PROTOCOL *gMetronome =3D NULL; > > EFI_TIMER_ARCH_PROTOCOL *gTimer =3D NULL; > > EFI_BDS_ARCH_PROTOCOL *gBds =3D NULL; > > @@ -211,6 +217,14 @@ EFI_DECOMPRESS_PROTOCOL gEfiDecompress =3D { > > DxeMainUefiDecompress > > }; > > > > +// > > +// Common event protocol > > +// > > +EDKII_COMMON_EVENT_PROTOCOL gEdkiiCommonEvent =3D { > > + 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 =3D CoreInstallProtocolInterface ( > > + &mCommonEventHalde, > > + &gEdkiiCommonEventProtocolGuid, > > + EFI_NATIVE_INTERFACE, > > + &gEdkiiCommonEvent > > + ); > > + ASSERT_EFI_ERROR (Status); > > + > > // > > // Produce Firmware Volume Protocols, one for each FV in the HOB lis= t. > > // > > 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[] =3D { > > EFI_CORE_PROTOCOL_NOTIFY_ENTRY mOptionalProtocols[] =3D { > > { &gEfiSecurity2ArchProtocolGuid, (VOID **)&gSecurity2, N= ULL, > > NULL, FALSE }, > > { &gEfiSmmBase2ProtocolGuid, (VOID **)&gSmmBase2, N= ULL, > > NULL, FALSE }, > > + { &gEdkiiCpu2ProtocolGuid, (VOID **)&gCpu2, N= ULL, NULL, > > FALSE }, > > { NULL, (VOID **)NULL, N= ULL, 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 =3D TPL_APPLICATION; > > +volatile EFI_TPL gEfiCurrentTpl =3D 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 =3D 0; > > +volatile UINTN gEventPending =3D 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 !=3D 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 UserEvent= s > > array > > + @param UserEvents An array of EFI_EVENT > > + @param UserIndex Pointer to the index of the event whi= ch > > + satisfied the wait condition > > + > > + @retval EFI_SUCCESS The event indicated by Index was sign= aled. > > + @retval EFI_INVALID_PARAMETER The event indicated by Index has a > > notification > > + function or Event was not a valid typ= e > > + @retval EFI_UNSUPPORTED The current TPL is not TPL_APPLICATIO= N > > + > > +**/ > > +EFI_STATUS > > +EFIAPI > > +CommonWaitForEvent ( > > + IN UINTN NumberOfEvents, > > + IN EFI_EVENT *UserEvents, > > + OUT UINTN *UserIndex > > + ) > > +{ > > + EFI_STATUS Status; > > + UINTN Index; > > + > > if (NumberOfEvents =3D=3D 0) { > > return EFI_INVALID_PARAMETER; > > } > > @@ -678,7 +706,7 @@ CoreWaitForEvent ( > > > > for(;;) { > > > > - for(Index =3D 0; Index < NumberOfEvents; Index++) { > > + for (Index =3D 0; Index < NumberOfEvents; Index++) { > > > > Status =3D CoreCheckEvent (UserEvents[Index]); > > > > @@ -693,14 +721,66 @@ CoreWaitForEvent ( > > } > > } > > > > - // > > - // Signal the Idle event > > - // > > - CoreSignalEvent (gIdleLoopEvent); > > + if (gCpu !=3D NULL && gCpu2 !=3D 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 m= ode and > > + attributes > > + @param NotifyTpl The task priority level of event noti= fications > > + @param NotifyFunction Pointer to the events notification fu= nction > > + @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 s= ame > > 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) <=3D 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; > > }; > > >=20 > I think this change should belong to patch 1. Yes. You are right. I would move the change to 1/4. Thanks, Zhichao >=20 > Regards, > Jian >=20 > > #endif > > -- > > 2.21.0.windows.1