[edk2-devel] [PATCH V2 4/4] MdeModulePkg/DxeMain: Implement common event protocol
Wang, Jian J
jian.j.wang at intel.com
Tue Jul 9 09:23:35 UTC 2019
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 at edk2.groups.io
> Cc: Sean Brogan <sean.brogan at microsoft.com>; Wang, Jian J
> <jian.j.wang at intel.com>; Wu, Hao A <hao.a.wu at intel.com>; Ni, Ray
> <ray.ni at intel.com>; Zeng, Star <star.zeng at intel.com>; Gao, Liming
> <liming.gao at intel.com>; Michael Turner <Michael.Turner at microsoft.com>;
> Bret Barkelew <Bret.Barkelew at microsoft.com>
> Subject: [PATCH V2 4/4] MdeModulePkg/DxeMain: Implement common
> event protocol
>
> From: Sean Brogan <sean.brogan at 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 at intel.com>
> Cc: Hao A Wu <hao.a.wu at intel.com>
> Cc: Ray Ni <ray.ni at intel.com>
> Cc: Star Zeng <star.zeng at intel.com>
> Cc: Liming gao <liming.gao at intel.com>
> Cc: Sean Brogan <sean.brogan at microsoft.com>
> Cc: Michael Turner <Michael.Turner at microsoft.com>
> Cc: Bret Barkelew <Bret.Barkelew at microsoft.com>
> Signed-off-by: Zhichao Gao <zhichao.gao at 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.
Regards,
Jian
> #endif
> --
> 2.21.0.windows.1
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#43431): https://edk2.groups.io/g/devel/message/43431
Mute This Topic: https://groups.io/mt/32403452/1813853
Group Owner: devel+owner at edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [edk2-devel-archive at redhat.com]
-=-=-=-=-=-=-=-=-=-=-=-
More information about the edk2-devel-archive
mailing list