[v1] Fix race condition and add event protocol

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

Posted by Gao, Zhichao 6 years, 8 months ago

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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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

Posted by Laszlo Ersek 6 years, 8 months ago

On 05/24/19 07:04, Gao, Zhichao wrote:
> 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>

Thanks for the CC; I'll let others review this series.

Thanks
Laszlo

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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#41326): https://edk2.groups.io/g/devel/message/41326
Mute This Topic: https://groups.io/mt/31741727/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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

Posted by Felix Polyudov 6 years, 8 months ago

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.

-=-=-=-=-=-=-=-=-=-=-=-
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/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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

Posted by Liming Gao 6 years, 7 months ago

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.

-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#42392): https://edk2.groups.io/g/devel/message/42392
Mute This Topic: https://groups.io/mt/31741727/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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

Posted by Ni, Ray 6 years, 8 months ago

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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#41309): https://edk2.groups.io/g/devel/message/41309
Mute This Topic: https://groups.io/mt/31741727/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-

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

Posted by Gao, Zhichao 6 years, 8 months ago

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


-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.

View/Reply Online (#41317): https://edk2.groups.io/g/devel/message/41317
Mute This Topic: https://groups.io/mt/31741727/1787277
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub  [importer@patchew.org]
-=-=-=-=-=-=-=-=-=-=-=-