I think there is a bigger problem with the idle event signaling by CoreWaitForEvent. On every iteration CoreWaitForEvent checks user events, and, if none is signaled, the function signals special idle event. This itself is not a problem. However, on many (most?) platforms CPU driver installs idle event handler that calls CpuSleep (f.i. refer to IdleLoopEventCallback in UefiCpuPkg/CpuDxe/CpuDxe.c and ArmPkg/Drivers/CpuDxe/CpuDxe.c). CpuSleep "places the CPU in a sleep state until an interrupt is received", which changes the nature of the WaitForEvent function arguably violating the UEFI specification.
According to the UEFI specification: "The list of events in the Event array are evaluated in order from first to last, and this evaluation is repeated until an event is signaled or an error is detected." The sentence above implies continues evaluation (polling model). An idle event callback that sends CPU to a sleep state, converts WaitForEvent from a continues polling function into a poll-once-per-timer-period function (timer is typically the only enabled HW interrupt), which reduces quality of service provided by WaitForEvent and can lead to missed events. For example, UEFI application that reads key strokes using gBS->WaitForEvent(..)/ConIn->ReadKey(...) sequence will be losing key strokes if they are coming faster than 18.2 keys per second on a system based on legacy timer. So, should idle event support be removed from CoreWaitForEvent? -----Original Message----- From: devel@edk2.groups.io [mailto:devel@edk2.groups.io] On Behalf Of Gao, Zhichao Sent: Friday, May 24, 2019 1:05 AM To: devel@edk2.groups.io Cc: Jian J Wang; Hao A Wu; Ray Ni; Star Zeng; Liming gao; Sean Brogan; Michael Turner; Bret Barkelew; Michael D Kinney; Eric Dong; Laszlo Ersek Subject: [edk2-devel] [PATCH 0/6] Fix race condition and add event protocol There is a race condition in CoreWaitForEvent function: 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 gEfiCpu2ProtocolGuid to fix that. The protocol include one interface to enable interrupt and put the cpu to sleep. Add a event protocol gEdkiiCommonEventProtocolGuid to support all TPL event. It is require for PI drivers that use HW interrput. Cc: Jian J Wang <jian.j.w...@intel.com> Cc: Hao A Wu <hao.a...@intel.com> cc: Ray Ni <ray...@intel.com> Cc: Star Zeng <star.z...@intel.com> Cc: Liming gao <liming....@intel.com> Cc: Sean Brogan <sean.bro...@microsoft.com> Cc: Michael Turner <michael.tur...@microsoft.com> Cc: Bret Barkelew <bret.barke...@microsoft.com> Cc: Michael D Kinney <michael.d.kin...@intel.com> Cc: Eric Dong <eric.d...@intel.com> Cc: Laszlo Ersek <ler...@redhat.com> Sean Brogan (5): MdeModulePkg: Add gEdkiiCommonEventProtocolGuid for event MdePkg/BaseLib.h: Add EnableInterruptsAndSleep function declare MdePkg/BaseLib: Implement EnableInterruptsAndSleep MdePkg: Add gEfiCpu2ProtocolGuid and header file MdeModulePkg/DxeMain: Implement common event protocol Zhichao Gao (1): UefiCpuPkg/CpuDxe: Implement Cpu2 protocol MdeModulePkg/Core/Dxe/DxeMain.h | 64 ++++++++++- MdeModulePkg/Core/Dxe/DxeMain.inf | 1 + 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/CommonEvent.h | 18 ++++ MdeModulePkg/MdeModulePkg.dec | 3 + MdePkg/Include/Library/BaseLib.h | 11 ++ MdePkg/Include/Protocol/Cpu2.h | 43 ++++++++ .../Library/BaseLib/Ia32/EnableInterrupts.c | 18 +++- .../BaseLib/Ia32/EnableInterrupts.nasm | 15 ++- .../Library/BaseLib/X64/EnableInterrupts.nasm | 15 ++- MdePkg/MdePkg.dec | 3 + UefiCpuPkg/CpuDxe/CpuDxe.c | 40 ++++++- UefiCpuPkg/CpuDxe/CpuDxe.h | 15 +++ UefiCpuPkg/CpuDxe/CpuDxe.inf | 3 +- 17 files changed, 358 insertions(+), 18 deletions(-) create mode 100644 MdeModulePkg/Include/Protocol/CommonEvent.h create mode 100644 MdePkg/Include/Protocol/Cpu2.h -- 2.21.0.windows.1 -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#41302): https://edk2.groups.io/g/devel/message/41302 Mute This Topic: https://groups.io/mt/31741727/1498468 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [fel...@ami.com] -=-=-=-=-=-=-=-=-=-=-=- Please consider the environment before printing this email. The information contained in this message may be confidential and proprietary to American Megatrends, Inc. This communication is intended to be read only by the individual or entity to whom it is addressed or by their designee. If the reader of this message is not the intended recipient, you are on notice that any distribution of this message, in any form, is strictly prohibited. Please promptly notify the sender by reply e-mail or by telephone at 770-246-8600, and then delete or destroy all copies of the transmission. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#41329): https://edk2.groups.io/g/devel/message/41329 Mute This Topic: https://groups.io/mt/31741727/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-
