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.100, mailfrom: jian.j.wang@intel.com) Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by groups.io with SMTP; Tue, 09 Jul 2019 02:23:40 -0700 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga003.jf.intel.com ([10.7.209.27]) by orsmga105.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 09 Jul 2019 02:23:39 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.63,470,1557212400"; d="scan'208";a="167931366" Received: from fmsmsx103.amr.corp.intel.com ([10.18.124.201]) by orsmga003.jf.intel.com with ESMTP; 09 Jul 2019 02:23:39 -0700 Received: from fmsmsx119.amr.corp.intel.com (10.18.124.207) by FMSMSX103.amr.corp.intel.com (10.18.124.201) with Microsoft SMTP Server (TLS) id 14.3.439.0; Tue, 9 Jul 2019 02:23:38 -0700 Received: from shsmsx103.ccr.corp.intel.com (10.239.4.69) by FMSMSX119.amr.corp.intel.com (10.18.124.207) with Microsoft SMTP Server (TLS) id 14.3.439.0; Tue, 9 Jul 2019 02:23:38 -0700 Received: from shsmsx107.ccr.corp.intel.com ([169.254.9.162]) by SHSMSX103.ccr.corp.intel.com ([169.254.4.109]) with mapi id 14.03.0439.000; Tue, 9 Jul 2019 17:23:35 +0800 From: "Wang, Jian J" 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 Thread-Topic: [PATCH V2 4/4] MdeModulePkg/DxeMain: Implement common event protocol Thread-Index: AQHVNjH4GNKrPmGfpk6/dVExl5ZN5KbCAaEw Date: Tue, 9 Jul 2019 09:23:35 +0000 Message-ID: References: <20190709083956.13024-1-zhichao.gao@intel.com> <20190709083956.13024-5-zhichao.gao@intel.com> In-Reply-To: <20190709083956.13024-5-zhichao.gao@intel.com> Accept-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-titus-metadata-40: eyJDYXRlZ29yeUxhYmVscyI6IiIsIk1ldGFkYXRhIjp7Im5zIjoiaHR0cDpcL1wvd3d3LnRpdHVzLmNvbVwvbnNcL0ludGVsMyIsImlkIjoiODFmYWEwZWItODdmZS00NTBhLWFhMDMtMjdmMzllNTBhYTdiIiwicHJvcHMiOlt7Im4iOiJDVFBDbGFzc2lmaWNhdGlvbiIsInZhbHMiOlt7InZhbHVlIjoiQ1RQX05UIn1dfV19LCJTdWJqZWN0TGFiZWxzIjpbXSwiVE1DVmVyc2lvbiI6IjE3LjEwLjE4MDQuNDkiLCJUcnVzdGVkTGFiZWxIYXNoIjoiOVFOOWFDckVYaXlXWk1XYmZ0djZyWm5kek1rSjhwams2MFwva0NMSGRRXC9pdnZBZ0xZWk8ySWFZcDAxZml0cEQ4In0= x-ctpclassification: CTP_NT dlp-product: dlpe-windows dlp-version: 11.0.600.7 dlp-reaction: no-action x-originating-ip: [10.239.127.40] MIME-Version: 1.0 Return-Path: jian.j.wang@intel.com Content-Language: en-US Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Hi Zhichao, One common comment: please update copy right year. 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 ; 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 >=20 > From: Sean Brogan >=20 > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=3D1400 >=20 > 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. >=20 > 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. >=20 > Add 'volatile' specifier for gEfiCurrentTpl and gEventPending > because they may be changed out of the context (interrupt context). >=20 > 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(-) >=20 > 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; >=20 > -extern EFI_TPL gEfiCurrentTpl; > +extern volatile EFI_TPL gEfiCurrentTpl; >=20 > extern EFI_GUID *gDxeCoreFileName; > extern EFI_LOADED_IMAGE_PROTOCOL *gDxeCoreLoadedImage; > @@ -1714,6 +1717,65 @@ CoreCheckEvent ( > ); >=20 >=20 > +/** > + 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 signal= ed. > + @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 mod= e and > + attributes > + @param NotifyTpl The task priority level of event notifi= cations > + @param NotifyFunction Pointer to the events notification func= tion > + @param NotifyContext Pointer to the notification functions c= ontext; > + corresponds to parameter "Context" in t= he > + notification function > + @param EventGroup GUID for EventGroup if NULL act the sam= e > as > + gBS->CreateEvent(). > + @param Event Pointer to the newly created event if t= he 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 >=20 > [Pcd] >=20 > 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; >=20 > +// > +// 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 > }; >=20 > +// > +// 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 (); >=20 > + Status =3D 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[] =3D { > EFI_CORE_PROTOCOL_NOTIFY_ENTRY mOptionalProtocols[] =3D { > { &gEfiSecurity2ArchProtocolGuid, (VOID **)&gSecurity2, NUL= L, > NULL, FALSE }, > { &gEfiSmmBase2ProtocolGuid, (VOID **)&gSmmBase2, NUL= L, > NULL, FALSE }, > + { &gEdkiiCpu2ProtocolGuid, (VOID **)&gCpu2, NUL= L, NULL, > FALSE }, > { NULL, (VOID **)NULL, NUL= L, NULL, FALSE } > }; >=20 > 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; >=20 > /// > /// 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; >=20 > /// > /// 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; > } >=20 > + 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 signal= ed. > + @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 =3D=3D 0) { > return EFI_INVALID_PARAMETER; > } > @@ -678,7 +706,7 @@ CoreWaitForEvent ( >=20 > for(;;) { >=20 > - for(Index =3D 0; Index < NumberOfEvents; Index++) { > + for (Index =3D 0; Index < NumberOfEvents; Index++) { >=20 > Status =3D CoreCheckEvent (UserEvents[Index]); >=20 > @@ -693,14 +721,66 @@ CoreWaitForEvent ( > } > } >=20 > - // > - // 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 w= ait > for an interrupt > + // > + gCpu2->EnableAndWaitForInterrupt (gCpu2); > + } > + } > } > } >=20 >=20 > +/** > + Creates an event in a group. > + > + Support all TPL. > + > + @param Type The type of event to create and its mod= e and > + attributes > + @param NotifyTpl The task priority level of event notifi= cations > + @param NotifyFunction Pointer to the events notification func= tion > + @param NotifyContext Pointer to the notification functions c= ontext; > + corresponds to parameter "Context" in t= he > + notification function > + @param EventGroup GUID for EventGroup if NULL act the sam= e > as > + gBS->CreateEvent(). > + @param Event Pointer to the newly created event if t= he 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. >=20 > 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 >=20 >=20 > #define VALID_TPL(a) ((a) <=3D TPL_HIGH_LEVEL) > -extern UINTN gEventPending; > +extern volatile UINTN gEventPending; >=20 > /// > /// 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. Regards, Jian > #endif > -- > 2.21.0.windows.1