Re: [edk2-devel] [PATCH 0/6] Fix race condition and add event protocol

["Felix Polyudov" <felixp@ami.com>]

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.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>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Cc: Eric Dong <eric.dong@intel.com>
Cc: Laszlo Ersek <lersek@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+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [felixp@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.