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

["Gao, Zhichao" <zhichao.gao@intel.com>]

Sorry I didn't add the BZ link.
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."

Here is the wait for event flow:

Result = CheckEvent
(interrupt happens here, and set the gEventPending)
Result == FALSE
Ilde event (put cpu to sleep)(there are events in pending list and they should be handle, but the cpu is hlt and cannot handle it)

Solution is here:
Result = CheckEvent
(interrupt happens)
Disable interrupt
PendingResult = Check gEventPending
If gEventPending == TRUE {
  Enable interrupt and goto CheckEvent to handle
} else {
  Enable interrupt and hlt (this should be 'atomic' so that no race condition would happen)
}

Thanks,
Zhichao

> -----Original Message-----
> From: Ni, Ray
> Sent: Friday, May 24, 2019 1:18 PM
> To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
> Cc: Wang, Jian J <jian.j.wang@intel.com>; Wu, Hao A <hao.a.wu@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>
> Subject: RE: [PATCH 0/6] Fix race condition and add event protocol
> 
> Zhichao,
> Did your detailed patch commit message describe the consequence of the
> race condition? (I haven't checked.) If no, could you please describe in detail
> about the consequence?
> 
> Thanks,
> Ray
> 
> > -----Original Message-----
> > From: Gao, Zhichao
> > Sent: Friday, May 24, 2019 1:05 PM
> > To: devel@edk2.groups.io
> > 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>
> > Subject: [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