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

["Liming Gao" <liming.gao@intel.com>]

Felix:
  Sorry for the late response. 

> -----Original Message-----
> From: Felix Polyudov [mailto:Felixp@ami.com]
> Sent: Friday, May 24, 2019 8:53 PM
> To: devel@edk2.groups.io; Gao, Zhichao <zhichao.gao@intel.com>
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@intel.com>; Ni, Ray <ray.ni@intel.com>; Zeng, Star
> <star.zeng@intel.com>; Gao, Liming <liming.gao@intel.com>; Sean Brogan <sean.brogan@microsoft.com>; Michael Turner
> <Michael.Turner@microsoft.com>; Bret Barkelew <Bret.Barkelew@microsoft.com>; Kinney, Michael D <michael.d.kinney@intel.com>;
> Dong, Eric <eric.dong@intel.com>; Laszlo Ersek <lersek@redhat.com>; Andrew Fish (afish@apple.com) <afish@apple.com>
> Subject: RE: [edk2-devel] [PATCH 0/6] Fix race condition and add event protocol
> 
> 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).

Here is repeat. It is not identical to polling model. A check per timer period is also a repeat implementation. So, it follows UEFI spec.

> 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.

This case may not be real case. Idle event is added since 2011 year @54cd17e9842d82dae3cd78686e05c4dc37a3540d. 
I don't get the real issue report. Have you meet with some real issue in the production?

> 
> So, should idle event support be removed from CoreWaitForEvent?
> 
I think we can still keep it. It is edk2 implementation.

Thanks
Liming
> -----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
> 
> 
> 
> 
> 
> 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.