* [PATCH V2 0/4] Fix race condition and add event protocol
@ 2019-07-09 8:39 Gao, Zhichao
2019-07-09 8:39 ` [PATCH V2 1/4] MdeModulePkg: Add gEdkiiCpu2ProtocolGuid and header file Gao, Zhichao
` (4 more replies)
0 siblings, 5 replies; 15+ messages in thread
From: Gao, Zhichao @ 2019-07-09 8:39 UTC (permalink / raw)
To: devel
Cc: Jian J Wang, Hao A Wu, Ray Ni, Star Zeng, Liming gao, Sean Brogan,
Michael Turner, Bret Barkelew, Michael D Kinney, Eric Dong
V1:
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.
V2:
Move EnableInterruptsAndSleep to UefiCpuPkg/CpuDxe. It is inappropiate
to add it to MdePkg which should only contain code base on the UEFI spec.
Change the name gEfiCpu2ProtocolGuid to gEdkiiCpu2ProtocolGuid.
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>
Sean Brogan (3):
MdeModulePkg: Add gEdkiiCpu2ProtocolGuid and header file
UefiCpuPkg/CpuDxe: Implement Cpu2 protocol
MdeModulePkg/DxeMain: Implement common event protocol
Zhichao Gao (1):
MdeModulePkg: Add gEdkiiCommonEventProtocolGuid for creating event
MdeModulePkg/Core/Dxe/DxeMain.h | 64 ++++++++++-
MdeModulePkg/Core/Dxe/DxeMain.inf | 2 +
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/Include/Protocol/Cpu2.h | 43 ++++++++
MdeModulePkg/MdeModulePkg.dec | 6 ++
UefiCpuPkg/CpuDxe/CpuDxe.c | 38 +++++++
UefiCpuPkg/CpuDxe/CpuDxe.h | 25 +++++
UefiCpuPkg/CpuDxe/CpuDxe.inf | 3 +
.../CpuDxe/Ia32/EnableInterruptsAndSleep.c | 24 +++++
.../CpuDxe/X64/EnableInterruptsAndSleep.nasm | 31 ++++++
14 files changed, 368 insertions(+), 13 deletions(-)
create mode 100644 MdeModulePkg/Include/Protocol/CommonEvent.h
create mode 100644 MdeModulePkg/Include/Protocol/Cpu2.h
create mode 100644 UefiCpuPkg/CpuDxe/Ia32/EnableInterruptsAndSleep.c
create mode 100644 UefiCpuPkg/CpuDxe/X64/EnableInterruptsAndSleep.nasm
--
2.21.0.windows.1
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH V2 1/4] MdeModulePkg: Add gEdkiiCpu2ProtocolGuid and header file
2019-07-09 8:39 [PATCH V2 0/4] Fix race condition and add event protocol Gao, Zhichao
@ 2019-07-09 8:39 ` Gao, Zhichao
2019-07-09 8:39 ` [PATCH V2 2/4] UefiCpuPkg/CpuDxe: Implement Cpu2 protocol Gao, Zhichao
` (3 subsequent siblings)
4 siblings, 0 replies; 15+ messages in thread
From: Gao, Zhichao @ 2019-07-09 8:39 UTC (permalink / raw)
To: devel
Cc: Sean Brogan, Jian J Wang, Hao A Wu, Ray Ni, Star Zeng, Liming gao,
Michael Turner, Bret Barkelew
From: Sean Brogan <sean.brogan@microsoft.com>
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1400
Add gEdkiiCpu2ProtocolGuid to MdeMdeModulePkg.dec.
Add the header file of Cpu2 protocol: it has one interface
to enable interrupt and put cpu to sleep to wait for an
interrupt.
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>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
MdeModulePkg/Include/Protocol/Cpu2.h | 43 ++++++++++++++++++++++++++++
MdeModulePkg/MdeModulePkg.dec | 3 ++
2 files changed, 46 insertions(+)
create mode 100644 MdeModulePkg/Include/Protocol/Cpu2.h
diff --git a/MdeModulePkg/Include/Protocol/Cpu2.h b/MdeModulePkg/Include/Protocol/Cpu2.h
new file mode 100644
index 0000000000..14464a38c8
--- /dev/null
+++ b/MdeModulePkg/Include/Protocol/Cpu2.h
@@ -0,0 +1,43 @@
+/** @file
+ CPU2 Protocol
+
+ This code abstracts the DXE core from processor implementation details.
+
+ Copyright (c) 2006 - 2018, Microsoft Corporation. All rights reserved.<BR>
+
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+#ifndef __PROTOCOL_CPU2_H__
+#define __PROTOCOL_CPU2_H__
+
+#include <Uefi.h>
+
+typedef struct _EDKII_CPU2_PROTOCOL EDKII_CPU2_PROTOCOL;
+
+
+/**
+ This function enables CPU interrupts and then waits for an interrupt to arrive.
+
+ @param This The EDKII_CPU2_PROTOCOL instance.
+
+ @retval EFI_SUCCESS Interrupts are enabled on the processor.
+ @retval EFI_DEVICE_ERROR Interrupts could not be enabled on the processor.
+
+**/
+typedef
+EFI_STATUS
+(EFIAPI *EDKII_CPU_ENABLE_AND_WAIT_FOR_INTERRUPT)(
+ IN EDKII_CPU2_PROTOCOL *This
+ );
+
+///
+/// The EDKII_CPU2_PROTOCOL is used to abstract processor-specific functions from the DXE
+/// Foundation.
+///
+struct _EDKII_CPU2_PROTOCOL {
+ EDKII_CPU_ENABLE_AND_WAIT_FOR_INTERRUPT EnableAndForWaitInterrupt;
+};
+
+#endif
diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index 12e0bbf579..354241c2ab 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -620,6 +620,9 @@
## Include/Protocol/PeCoffImageEmulator.h
gEdkiiPeCoffImageEmulatorProtocolGuid = { 0x96f46153, 0x97a7, 0x4793, { 0xac, 0xc1, 0xfa, 0x19, 0xbf, 0x78, 0xea, 0x97 } }
+ ## Include/Protocol/Cpu2.h
+ gEdkiiCpu2ProtocolGuid = { 0x55198405, 0x26C0, 0x4765, {0x8B, 0x7D, 0xBE, 0x1D, 0xF5, 0xF9, 0x97, 0x12 }}
+
#
# [Error.gEfiMdeModulePkgTokenSpaceGuid]
# 0x80000001 | Invalid value provided.
--
2.21.0.windows.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH V2 2/4] UefiCpuPkg/CpuDxe: Implement Cpu2 protocol
2019-07-09 8:39 [PATCH V2 0/4] Fix race condition and add event protocol Gao, Zhichao
2019-07-09 8:39 ` [PATCH V2 1/4] MdeModulePkg: Add gEdkiiCpu2ProtocolGuid and header file Gao, Zhichao
@ 2019-07-09 8:39 ` Gao, Zhichao
2019-07-11 2:22 ` Dong, Eric
2019-07-09 8:39 ` [PATCH V2 3/4] MdeModulePkg: Add gEdkiiCommonEventProtocolGuid for creating event Gao, Zhichao
` (2 subsequent siblings)
4 siblings, 1 reply; 15+ messages in thread
From: Gao, Zhichao @ 2019-07-09 8:39 UTC (permalink / raw)
To: devel
Cc: Sean Brogan, Eric Dong, Ray Ni, Laszlo Ersek, Liming Gao,
Michael Turner, Bret Barkelew
From: Sean Brogan <sean.brogan@microsoft.com>
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1400
Implement Cp2 protocol: it has one interface to enable the
interrupt and put cpu to sleep and wait for an interrupt.
Cc: Eric Dong <eric.dong@intel.com>
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.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>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
UefiCpuPkg/CpuDxe/CpuDxe.c | 38 +++++++++++++++++++
UefiCpuPkg/CpuDxe/CpuDxe.h | 25 ++++++++++++
UefiCpuPkg/CpuDxe/CpuDxe.inf | 3 ++
.../CpuDxe/Ia32/EnableInterruptsAndSleep.c | 24 ++++++++++++
.../CpuDxe/X64/EnableInterruptsAndSleep.nasm | 31 +++++++++++++++
5 files changed, 121 insertions(+)
create mode 100644 UefiCpuPkg/CpuDxe/Ia32/EnableInterruptsAndSleep.c
create mode 100644 UefiCpuPkg/CpuDxe/X64/EnableInterruptsAndSleep.nasm
diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.c b/UefiCpuPkg/CpuDxe/CpuDxe.c
index 7d7270e10b..2eeffcf426 100644
--- a/UefiCpuPkg/CpuDxe/CpuDxe.c
+++ b/UefiCpuPkg/CpuDxe/CpuDxe.c
@@ -18,6 +18,7 @@
//
BOOLEAN InterruptState = FALSE;
EFI_HANDLE mCpuHandle = NULL;
+EFI_HANDLE mCpu2Handle = NULL;
BOOLEAN mIsFlushingGCD;
BOOLEAN mIsAllocatingPageTable = FALSE;
UINT64 mValidMtrrAddressMask;
@@ -96,6 +97,10 @@ EFI_CPU_ARCH_PROTOCOL gCpu = {
4 // DmaBufferAlignment
};
+EDKII_CPU2_PROTOCOL gCpu2 = {
+ CpuEnableAndWaitForInterrupt
+};
+
//
// CPU Arch Protocol Functions
//
@@ -499,6 +504,28 @@ CpuSetMemoryAttributes (
return AssignMemoryPageAttributes (NULL, BaseAddress, Length, MemoryAttributes, NULL);
}
+//
+// CPU2 Protocol Functions
+//
+/**
+ This function enables CPU interrupts and then waits for an interrupt to arrive.
+
+ @param This The EFI_CPU2_PROTOCOL instance.
+
+ @retval EFI_SUCCESS Interrupts are enabled on the processor.
+ @retval EFI_DEVICE_ERROR Interrupts could not be enabled on the processor.
+
+**/
+EFI_STATUS
+CpuEnableAndWaitForInterrupt (
+ IN EDKII_CPU2_PROTOCOL *This
+ )
+{
+ EnableInterruptsAndSleep ();
+
+ return EFI_SUCCESS;
+}
+
/**
Initializes the valid bits mask and valid address mask for MTRRs.
@@ -1211,6 +1238,17 @@ InitializeCpu (
);
ASSERT_EFI_ERROR (Status);
+ //
+ // Install CPU2 Protocol
+ //
+ Status = gBS->InstallMultipleProtocolInterfaces (
+ &mCpu2Handle,
+ &gEdkiiCpu2ProtocolGuid,
+ &gCpu2,
+ NULL
+ );
+ ASSERT_EFI_ERROR (Status);
+
InitializeMpSupport ();
return Status;
diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.h b/UefiCpuPkg/CpuDxe/CpuDxe.h
index b029be430b..c1398830ba 100644
--- a/UefiCpuPkg/CpuDxe/CpuDxe.h
+++ b/UefiCpuPkg/CpuDxe/CpuDxe.h
@@ -12,6 +12,7 @@
#include <PiDxe.h>
#include <Protocol/Cpu.h>
+#include <Protocol/Cpu2.h>
#include <Protocol/MpService.h>
#include <Register/Msr.h>
@@ -305,6 +306,30 @@ PageFaultExceptionHandler (
IN EFI_SYSTEM_CONTEXT SystemContext
);
+/**
+ This function enables CPU interrupts and then waits for an interrupt to arrive.
+
+ @param This The EFI_CPU2_PROTOCOL instance.
+
+ @retval EFI_SUCCESS Interrupts are enabled on the processor.
+ @retval EFI_DEVICE_ERROR Interrupts could not be enabled on the processor.
+
+**/
+EFI_STATUS
+CpuEnableAndWaitForInterrupt (
+ IN EDKII_CPU2_PROTOCOL *This
+ );
+
+/**
+ Enables CPU interrupts and then waits for an interrupt to arrive.
+
+**/
+VOID
+EFIAPI
+EnableInterruptsAndSleep (
+ VOID
+ );
+
extern BOOLEAN mIsAllocatingPageTable;
extern UINTN mNumberOfProcessors;
diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.inf b/UefiCpuPkg/CpuDxe/CpuDxe.inf
index 57381dbc85..334ddb142f 100644
--- a/UefiCpuPkg/CpuDxe/CpuDxe.inf
+++ b/UefiCpuPkg/CpuDxe/CpuDxe.inf
@@ -54,14 +54,17 @@
[Sources.IA32]
Ia32/CpuAsm.nasm
+ Ia32/EnableInterruptsAndSleep.c
[Sources.X64]
X64/CpuAsm.nasm
+ X64/EnableInterruptsAndSleep.nasm
[Protocols]
gEfiCpuArchProtocolGuid ## PRODUCES
gEfiMpServiceProtocolGuid ## PRODUCES
gEfiSmmBase2ProtocolGuid ## SOMETIMES_CONSUMES
+ gEdkiiCpu2ProtocolGuid ## PRODUCES
[Guids]
gIdleLoopEventGuid ## CONSUMES ## Event
diff --git a/UefiCpuPkg/CpuDxe/Ia32/EnableInterruptsAndSleep.c b/UefiCpuPkg/CpuDxe/Ia32/EnableInterruptsAndSleep.c
new file mode 100644
index 0000000000..dda76139ab
--- /dev/null
+++ b/UefiCpuPkg/CpuDxe/Ia32/EnableInterruptsAndSleep.c
@@ -0,0 +1,24 @@
+/** @file
+ EnableInterruptsAndSleep function
+
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+
+
+
+/**
+ Enables CPU interrupts and then sleep.
+
+**/
+VOID
+EFIAPI
+EnableInterruptsAndSleep (
+ VOID
+ )
+{
+ _asm {
+ sti
+ hlt
+ }
+}
diff --git a/UefiCpuPkg/CpuDxe/X64/EnableInterruptsAndSleep.nasm b/UefiCpuPkg/CpuDxe/X64/EnableInterruptsAndSleep.nasm
new file mode 100644
index 0000000000..2d93ecb4bb
--- /dev/null
+++ b/UefiCpuPkg/CpuDxe/X64/EnableInterruptsAndSleep.nasm
@@ -0,0 +1,31 @@
+;------------------------------------------------------------------------------
+;
+; SPDX-License-Identifier: BSD-2-Clause-Patent
+;
+; Module Name:
+;
+; EnableInterruptsAndSleep.nasm
+;
+; Abstract:
+;
+; EnableInterruptsAndSleep function
+;
+; Notes:
+;
+;------------------------------------------------------------------------------
+
+ DEFAULT REL
+ SECTION .text
+
+;------------------------------------------------------------------------------
+; VOID
+; EFIAPI
+; EnableInterruptsAndSleep (
+; VOID
+; );
+;------------------------------------------------------------------------------
+global ASM_PFX(EnableInterruptsAndSleep)
+ASM_PFX(EnableInterruptsAndSleep):
+ sti
+ hlt
+ ret
--
2.21.0.windows.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH V2 3/4] MdeModulePkg: Add gEdkiiCommonEventProtocolGuid for creating event
2019-07-09 8:39 [PATCH V2 0/4] Fix race condition and add event protocol Gao, Zhichao
2019-07-09 8:39 ` [PATCH V2 1/4] MdeModulePkg: Add gEdkiiCpu2ProtocolGuid and header file Gao, Zhichao
2019-07-09 8:39 ` [PATCH V2 2/4] UefiCpuPkg/CpuDxe: Implement Cpu2 protocol Gao, Zhichao
@ 2019-07-09 8:39 ` Gao, Zhichao
2019-07-09 8:39 ` [PATCH V2 4/4] MdeModulePkg/DxeMain: Implement common event protocol Gao, Zhichao
2019-07-10 12:20 ` [edk2-devel] [PATCH V2 0/4] Fix race condition and add " Laszlo Ersek
4 siblings, 0 replies; 15+ messages in thread
From: Gao, Zhichao @ 2019-07-09 8:39 UTC (permalink / raw)
To: devel
Cc: Jian J Wang, Hao A Wu, Ray Ni, Star Zeng, Liming gao, Sean Brogan,
Michael Turner, Bret Barkelew
REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1400
Add common event protocol to support all TPL event:
Add the guid and header file of it.
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>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
MdeModulePkg/Include/Protocol/CommonEvent.h | 18 ++++++++++++++++++
MdeModulePkg/MdeModulePkg.dec | 3 +++
2 files changed, 21 insertions(+)
create mode 100644 MdeModulePkg/Include/Protocol/CommonEvent.h
diff --git a/MdeModulePkg/Include/Protocol/CommonEvent.h b/MdeModulePkg/Include/Protocol/CommonEvent.h
new file mode 100644
index 0000000000..176be14101
--- /dev/null
+++ b/MdeModulePkg/Include/Protocol/CommonEvent.h
@@ -0,0 +1,18 @@
+/** @file
+ Common event protocol would provide a service of event.
+
+ Copyright (c) 2019, Intel Corporation. All rights reserved.<BR>
+ SPDX-License-Identifier: BSD-2-Clause-Patent
+
+**/
+#ifndef __COMMON_EVENT_H__
+#define __COMMON_EVENT_H__
+
+#include <Uefi.h>
+
+typedef struct {
+ EFI_CREATE_EVENT_EX CreateEventEx;
+ EFI_WAIT_FOR_EVENT WaitForEvent;
+} EDKII_COMMON_EVENT_PROTOCOL;
+
+#endif
diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index 354241c2ab..8c746927e5 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -623,6 +623,9 @@
## Include/Protocol/Cpu2.h
gEdkiiCpu2ProtocolGuid = { 0x55198405, 0x26C0, 0x4765, {0x8B, 0x7D, 0xBE, 0x1D, 0xF5, 0xF9, 0x97, 0x12 }}
+ ## Include/Protocole/CommonEvent.h
+ gEdkiiCommonEventProtocolGuid = { 0x8bfb3718, 0xdb10, 0x4cc1, { 0xa9, 0xf5, 0x84, 0xae, 0xce, 0xce, 0x99, 0x55 } }
+
#
# [Error.gEfiMdeModulePkgTokenSpaceGuid]
# 0x80000001 | Invalid value provided.
--
2.21.0.windows.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH V2 4/4] MdeModulePkg/DxeMain: Implement common event protocol
2019-07-09 8:39 [PATCH V2 0/4] Fix race condition and add event protocol Gao, Zhichao
` (2 preceding siblings ...)
2019-07-09 8:39 ` [PATCH V2 3/4] MdeModulePkg: Add gEdkiiCommonEventProtocolGuid for creating event Gao, Zhichao
@ 2019-07-09 8:39 ` Gao, Zhichao
2019-07-09 9:23 ` Wang, Jian J
2019-07-10 12:20 ` [edk2-devel] [PATCH V2 0/4] Fix race condition and add " Laszlo Ersek
4 siblings, 1 reply; 15+ messages in thread
From: Gao, Zhichao @ 2019-07-09 8:39 UTC (permalink / raw)
To: devel
Cc: Sean Brogan, Jian J Wang, Hao A Wu, Ray Ni, Star Zeng, Liming gao,
Michael Turner, Bret Barkelew
From: Sean Brogan <sean.brogan@microsoft.com>
REF: 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. So it is
required to check the gEventPending with the interrupt disabled.
Implement a protocol gEdkiiCommonEventProtocolGuid to support
all TPL event for PI drivers that use HW interrput. It has two
interface, one is to create event and the other one is to wait for
event.
Add 'volatile' specifier for gEfiCurrentTpl and gEventPending
because they may be changed out of the context (interrupt context).
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>
Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
---
MdeModulePkg/Core/Dxe/DxeMain.h | 64 ++++++++++-
MdeModulePkg/Core/Dxe/DxeMain.inf | 2 +
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/Cpu2.h | 2 +-
7 files changed, 181 insertions(+), 14 deletions(-)
diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h b/MdeModulePkg/Core/Dxe/DxeMain.h
index 6a64852730..38907bfe8d 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain.h
+++ b/MdeModulePkg/Core/Dxe/DxeMain.h
@@ -38,6 +38,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include <Protocol/Security2.h>
#include <Protocol/Reset.h>
#include <Protocol/Cpu.h>
+#include <Protocol/Cpu2.h>
#include <Protocol/Metronome.h>
#include <Protocol/FirmwareVolumeBlock.h>
#include <Protocol/Capsule.h>
@@ -47,6 +48,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#include <Protocol/HiiPackageList.h>
#include <Protocol/SmmBase2.h>
#include <Protocol/PeCoffImageEmulator.h>
+#include <Protocol/CommonEvent.h>
#include <Guid/MemoryTypeInformation.h>
#include <Guid/FirmwareFileSystem2.h>
#include <Guid/FirmwareFileSystem3.h>
@@ -274,10 +276,11 @@ extern EFI_METRONOME_ARCH_PROTOCOL *gMetronome;
extern EFI_TIMER_ARCH_PROTOCOL *gTimer;
extern EFI_SECURITY_ARCH_PROTOCOL *gSecurity;
extern EFI_SECURITY2_ARCH_PROTOCOL *gSecurity2;
+extern EDKII_CPU2_PROTOCOL *gCpu2;
extern EFI_BDS_ARCH_PROTOCOL *gBds;
extern EFI_SMM_BASE2_PROTOCOL *gSmmBase2;
-extern EFI_TPL gEfiCurrentTpl;
+extern volatile EFI_TPL gEfiCurrentTpl;
extern EFI_GUID *gDxeCoreFileName;
extern EFI_LOADED_IMAGE_PROTOCOL *gDxeCoreLoadedImage;
@@ -1714,6 +1717,65 @@ CoreCheckEvent (
);
+/**
+ Stops execution until an event is signaled.
+
+ Support all TPL.
+
+ @param NumberOfEvents The number of events in the UserEvents array
+ @param UserEvents An array of EFI_EVENT
+ @param UserIndex Pointer to the index of the event which
+ satisfied the wait condition
+
+ @retval EFI_SUCCESS The event indicated by Index was signaled.
+ @retval EFI_INVALID_PARAMETER The event indicated by Index has a notification
+ function or Event was not a valid type
+ @retval EFI_UNSUPPORTED The current TPL is not TPL_APPLICATION
+
+**/
+EFI_STATUS
+EFIAPI
+CommonWaitForEvent (
+ IN UINTN NumberOfEvents,
+ IN EFI_EVENT *UserEvents,
+ OUT UINTN *UserIndex
+ );
+
+
+/**
+ Creates an event in a group.
+
+ Support all TPL.
+
+ @param Type The type of event to create and its mode and
+ attributes
+ @param NotifyTpl The task priority level of event notifications
+ @param NotifyFunction Pointer to the events notification function
+ @param NotifyContext Pointer to the notification functions context;
+ corresponds to parameter "Context" in the
+ notification function
+ @param EventGroup GUID for EventGroup if NULL act the same as
+ gBS->CreateEvent().
+ @param Event Pointer to the newly created event if the call
+ succeeds; undefined otherwise
+
+ @retval EFI_SUCCESS The event structure was created
+ @retval EFI_INVALID_PARAMETER One of the parameters has an invalid value
+ @retval EFI_OUT_OF_RESOURCES The event could not be allocated
+
+**/
+EFI_STATUS
+EFIAPI
+CommonCreateEventEx (
+ IN UINT32 Type,
+ IN EFI_TPL NotifyTpl,
+ IN EFI_EVENT_NOTIFY NotifyFunction, OPTIONAL
+ IN CONST VOID *NotifyContext, OPTIONAL
+ IN CONST EFI_GUID *EventGroup, OPTIONAL
+ OUT EFI_EVENT *Event
+ );
+
+
/**
Adds reserved memory, system memory, or memory-mapped I/O resources to the
global coherency domain of the processor.
diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf b/MdeModulePkg/Core/Dxe/DxeMain.inf
index 61161bee28..44f90bcf4b 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain.inf
+++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
@@ -171,6 +171,8 @@
gEfiVariableArchProtocolGuid ## CONSUMES
gEfiCapsuleArchProtocolGuid ## CONSUMES
gEfiWatchdogTimerArchProtocolGuid ## CONSUMES
+ gEdkiiCommonEventProtocolGuid ## SOMETIMES_CONSUMES
+ gEdkiiCpu2ProtocolGuid ## CONSUMES
[Pcd]
gEfiMdeModulePkgTokenSpaceGuid.PcdLoadFixAddressBootTimeCodePageNumber ## SOMETIMES_CONSUMES
diff --git a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
index 514d1aa75a..8196d96daa 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
+++ b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
@@ -13,12 +13,18 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
//
EFI_HANDLE mDecompressHandle = NULL;
+//
+// Common event protocol
+//
+EFI_HANDLE mCommonEventHalde = NULL;
+
//
// DXE Core globals for Architecture Protocols
//
EFI_SECURITY_ARCH_PROTOCOL *gSecurity = NULL;
EFI_SECURITY2_ARCH_PROTOCOL *gSecurity2 = NULL;
EFI_CPU_ARCH_PROTOCOL *gCpu = NULL;
+EDKII_CPU2_PROTOCOL *gCpu2 = NULL;
EFI_METRONOME_ARCH_PROTOCOL *gMetronome = NULL;
EFI_TIMER_ARCH_PROTOCOL *gTimer = NULL;
EFI_BDS_ARCH_PROTOCOL *gBds = NULL;
@@ -211,6 +217,14 @@ EFI_DECOMPRESS_PROTOCOL gEfiDecompress = {
DxeMainUefiDecompress
};
+//
+// Common event protocol
+//
+EDKII_COMMON_EVENT_PROTOCOL gEdkiiCommonEvent = {
+ CommonCreateEventEx,
+ CommonWaitForEvent
+};
+
//
// For Loading modules at fixed address feature, the configuration table is to cache the top address below which to load
// Runtime code&boot time code
@@ -475,6 +489,14 @@ DxeMain (
//
CoreNotifyOnProtocolInstallation ();
+ Status = CoreInstallProtocolInterface (
+ &mCommonEventHalde,
+ &gEdkiiCommonEventProtocolGuid,
+ EFI_NATIVE_INTERFACE,
+ &gEdkiiCommonEvent
+ );
+ ASSERT_EFI_ERROR (Status);
+
//
// Produce Firmware Volume Protocols, one for each FV in the HOB list.
//
diff --git a/MdeModulePkg/Core/Dxe/DxeMain/DxeProtocolNotify.c b/MdeModulePkg/Core/Dxe/DxeMain/DxeProtocolNotify.c
index 29a55d02e6..57d52c250d 100644
--- a/MdeModulePkg/Core/Dxe/DxeMain/DxeProtocolNotify.c
+++ b/MdeModulePkg/Core/Dxe/DxeMain/DxeProtocolNotify.c
@@ -41,6 +41,7 @@ EFI_CORE_PROTOCOL_NOTIFY_ENTRY mArchProtocols[] = {
EFI_CORE_PROTOCOL_NOTIFY_ENTRY mOptionalProtocols[] = {
{ &gEfiSecurity2ArchProtocolGuid, (VOID **)&gSecurity2, NULL, NULL, FALSE },
{ &gEfiSmmBase2ProtocolGuid, (VOID **)&gSmmBase2, NULL, NULL, FALSE },
+ { &gEdkiiCpu2ProtocolGuid, (VOID **)&gCpu2, NULL, NULL, FALSE },
{ NULL, (VOID **)NULL, NULL, NULL, FALSE }
};
diff --git a/MdeModulePkg/Core/Dxe/Event/Event.c b/MdeModulePkg/Core/Dxe/Event/Event.c
index 21db38aaf0..d271d2faf3 100644
--- a/MdeModulePkg/Core/Dxe/Event/Event.c
+++ b/MdeModulePkg/Core/Dxe/Event/Event.c
@@ -14,7 +14,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
///
/// gEfiCurrentTpl - Current Task priority level
///
-EFI_TPL gEfiCurrentTpl = TPL_APPLICATION;
+volatile EFI_TPL gEfiCurrentTpl = TPL_APPLICATION;
///
/// gEventQueueLock - Protects the event queues
@@ -29,7 +29,7 @@ LIST_ENTRY gEventQueue[TPL_HIGH_LEVEL + 1];
///
/// gEventPending - A bitmask of the EventQueues that are pending
///
-UINTN gEventPending = 0;
+volatile UINTN gEventPending = 0;
///
/// gEventSignalQueue - A list of events to signal based on EventGroup type
@@ -658,16 +658,44 @@ CoreWaitForEvent (
OUT UINTN *UserIndex
)
{
- EFI_STATUS Status;
- UINTN Index;
-
- //
+ //
// Can only WaitForEvent at TPL_APPLICATION
//
if (gEfiCurrentTpl != TPL_APPLICATION) {
return EFI_UNSUPPORTED;
}
+ return CommonWaitForEvent (NumberOfEvents, UserEvents, UserIndex);
+}
+
+
+/**
+ Stops execution until an event is signaled.
+
+ Support all TPL.
+
+ @param NumberOfEvents The number of events in the UserEvents array
+ @param UserEvents An array of EFI_EVENT
+ @param UserIndex Pointer to the index of the event which
+ satisfied the wait condition
+
+ @retval EFI_SUCCESS The event indicated by Index was signaled.
+ @retval EFI_INVALID_PARAMETER The event indicated by Index has a notification
+ function or Event was not a valid type
+ @retval EFI_UNSUPPORTED The current TPL is not TPL_APPLICATION
+
+**/
+EFI_STATUS
+EFIAPI
+CommonWaitForEvent (
+ IN UINTN NumberOfEvents,
+ IN EFI_EVENT *UserEvents,
+ OUT UINTN *UserIndex
+ )
+{
+ EFI_STATUS Status;
+ UINTN Index;
+
if (NumberOfEvents == 0) {
return EFI_INVALID_PARAMETER;
}
@@ -678,7 +706,7 @@ CoreWaitForEvent (
for(;;) {
- for(Index = 0; Index < NumberOfEvents; Index++) {
+ for (Index = 0; Index < NumberOfEvents; Index++) {
Status = CoreCheckEvent (UserEvents[Index]);
@@ -693,14 +721,66 @@ CoreWaitForEvent (
}
}
- //
- // Signal the Idle event
- //
- CoreSignalEvent (gIdleLoopEvent);
+ if (gCpu != NULL && gCpu2 != NULL) {
+ //
+ // None of the events checked above are ready/signaled yet,
+ // disable interrupts before checking for all pending events
+ //
+ gCpu->DisableInterrupt (gCpu);
+
+ if (((UINTN) HighBitSet64 (gEventPending)) > gEfiCurrentTpl) {
+ //
+ // There are pending events, enable interrupts for these events to be processed
+ //
+ gCpu->EnableInterrupt (gCpu);
+ } else {
+ //
+ // No events are pending, enable interrupts, sleep the CPU and wait for an interrupt
+ //
+ gCpu2->EnableAndWaitForInterrupt (gCpu2);
+ }
+ }
}
}
+/**
+ Creates an event in a group.
+
+ Support all TPL.
+
+ @param Type The type of event to create and its mode and
+ attributes
+ @param NotifyTpl The task priority level of event notifications
+ @param NotifyFunction Pointer to the events notification function
+ @param NotifyContext Pointer to the notification functions context;
+ corresponds to parameter "Context" in the
+ notification function
+ @param EventGroup GUID for EventGroup if NULL act the same as
+ gBS->CreateEvent().
+ @param Event Pointer to the newly created event if the call
+ succeeds; undefined otherwise
+
+ @retval EFI_SUCCESS The event structure was created
+ @retval EFI_INVALID_PARAMETER One of the parameters has an invalid value
+ @retval EFI_OUT_OF_RESOURCES The event could not be allocated
+
+**/
+EFI_STATUS
+EFIAPI
+CommonCreateEventEx (
+ IN UINT32 Type,
+ IN EFI_TPL NotifyTpl,
+ IN EFI_EVENT_NOTIFY NotifyFunction, OPTIONAL
+ IN CONST VOID *NotifyContext, OPTIONAL
+ IN CONST EFI_GUID *EventGroup, OPTIONAL
+ OUT EFI_EVENT *Event
+ )
+{
+ return CoreCreateEventInternal (Type, NotifyTpl, NotifyFunction, NotifyContext, EventGroup, Event);
+}
+
+
/**
Closes an event and frees the event structure.
diff --git a/MdeModulePkg/Core/Dxe/Event/Event.h b/MdeModulePkg/Core/Dxe/Event/Event.h
index 8141c5003e..050df26ec9 100644
--- a/MdeModulePkg/Core/Dxe/Event/Event.h
+++ b/MdeModulePkg/Core/Dxe/Event/Event.h
@@ -12,7 +12,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
#define VALID_TPL(a) ((a) <= TPL_HIGH_LEVEL)
-extern UINTN gEventPending;
+extern volatile UINTN gEventPending;
///
/// Set if Event is part of an event group
diff --git a/MdeModulePkg/Include/Protocol/Cpu2.h b/MdeModulePkg/Include/Protocol/Cpu2.h
index 14464a38c8..81c5c8e424 100644
--- a/MdeModulePkg/Include/Protocol/Cpu2.h
+++ b/MdeModulePkg/Include/Protocol/Cpu2.h
@@ -37,7 +37,7 @@ EFI_STATUS
/// Foundation.
///
struct _EDKII_CPU2_PROTOCOL {
- EDKII_CPU_ENABLE_AND_WAIT_FOR_INTERRUPT EnableAndForWaitInterrupt;
+ EDKII_CPU_ENABLE_AND_WAIT_FOR_INTERRUPT EnableAndWaitForInterrupt;
};
#endif
--
2.21.0.windows.1
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH V2 4/4] MdeModulePkg/DxeMain: Implement common event protocol
2019-07-09 8:39 ` [PATCH V2 4/4] MdeModulePkg/DxeMain: Implement common event protocol Gao, Zhichao
@ 2019-07-09 9:23 ` Wang, Jian J
2019-07-10 0:24 ` Gao, Zhichao
0 siblings, 1 reply; 15+ messages in thread
From: Wang, Jian J @ 2019-07-09 9:23 UTC (permalink / raw)
To: Gao, Zhichao, devel@edk2.groups.io
Cc: Sean Brogan, Wu, Hao A, Ni, Ray, Zeng, Star, Gao, Liming,
Michael Turner, Bret Barkelew
Hi Zhichao,
One common comment: please update copy right year.
See another comment at the almost the end of the email.
> -----Original Message-----
> From: Gao, Zhichao
> Sent: Tuesday, July 09, 2019 4:40 PM
> To: devel@edk2.groups.io
> Cc: Sean Brogan <sean.brogan@microsoft.com>; 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>; Michael Turner <Michael.Turner@microsoft.com>;
> Bret Barkelew <Bret.Barkelew@microsoft.com>
> Subject: [PATCH V2 4/4] MdeModulePkg/DxeMain: Implement common
> event protocol
>
> From: Sean Brogan <sean.brogan@microsoft.com>
>
> REF: 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. So it is
> required to check the gEventPending with the interrupt disabled.
>
> Implement a protocol gEdkiiCommonEventProtocolGuid to support
> all TPL event for PI drivers that use HW interrput. It has two
> interface, one is to create event and the other one is to wait for
> event.
>
> Add 'volatile' specifier for gEfiCurrentTpl and gEventPending
> because they may be changed out of the context (interrupt context).
>
> 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>
> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> ---
> MdeModulePkg/Core/Dxe/DxeMain.h | 64 ++++++++++-
> MdeModulePkg/Core/Dxe/DxeMain.inf | 2 +
> 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/Cpu2.h | 2 +-
> 7 files changed, 181 insertions(+), 14 deletions(-)
>
> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h
> b/MdeModulePkg/Core/Dxe/DxeMain.h
> index 6a64852730..38907bfe8d 100644
> --- a/MdeModulePkg/Core/Dxe/DxeMain.h
> +++ b/MdeModulePkg/Core/Dxe/DxeMain.h
> @@ -38,6 +38,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> #include <Protocol/Security2.h>
> #include <Protocol/Reset.h>
> #include <Protocol/Cpu.h>
> +#include <Protocol/Cpu2.h>
> #include <Protocol/Metronome.h>
> #include <Protocol/FirmwareVolumeBlock.h>
> #include <Protocol/Capsule.h>
> @@ -47,6 +48,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> #include <Protocol/HiiPackageList.h>
> #include <Protocol/SmmBase2.h>
> #include <Protocol/PeCoffImageEmulator.h>
> +#include <Protocol/CommonEvent.h>
> #include <Guid/MemoryTypeInformation.h>
> #include <Guid/FirmwareFileSystem2.h>
> #include <Guid/FirmwareFileSystem3.h>
> @@ -274,10 +276,11 @@ extern EFI_METRONOME_ARCH_PROTOCOL
> *gMetronome;
> extern EFI_TIMER_ARCH_PROTOCOL *gTimer;
> extern EFI_SECURITY_ARCH_PROTOCOL *gSecurity;
> extern EFI_SECURITY2_ARCH_PROTOCOL *gSecurity2;
> +extern EDKII_CPU2_PROTOCOL *gCpu2;
> extern EFI_BDS_ARCH_PROTOCOL *gBds;
> extern EFI_SMM_BASE2_PROTOCOL *gSmmBase2;
>
> -extern EFI_TPL gEfiCurrentTpl;
> +extern volatile EFI_TPL gEfiCurrentTpl;
>
> extern EFI_GUID *gDxeCoreFileName;
> extern EFI_LOADED_IMAGE_PROTOCOL *gDxeCoreLoadedImage;
> @@ -1714,6 +1717,65 @@ CoreCheckEvent (
> );
>
>
> +/**
> + Stops execution until an event is signaled.
> +
> + Support all TPL.
> +
> + @param NumberOfEvents The number of events in the UserEvents
> array
> + @param UserEvents An array of EFI_EVENT
> + @param UserIndex Pointer to the index of the event which
> + satisfied the wait condition
> +
> + @retval EFI_SUCCESS The event indicated by Index was signaled.
> + @retval EFI_INVALID_PARAMETER The event indicated by Index has a
> notification
> + function or Event was not a valid type
> + @retval EFI_UNSUPPORTED The current TPL is not TPL_APPLICATION
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +CommonWaitForEvent (
> + IN UINTN NumberOfEvents,
> + IN EFI_EVENT *UserEvents,
> + OUT UINTN *UserIndex
> + );
> +
> +
> +/**
> + Creates an event in a group.
> +
> + Support all TPL.
> +
> + @param Type The type of event to create and its mode and
> + attributes
> + @param NotifyTpl The task priority level of event notifications
> + @param NotifyFunction Pointer to the events notification function
> + @param NotifyContext Pointer to the notification functions context;
> + corresponds to parameter "Context" in the
> + notification function
> + @param EventGroup GUID for EventGroup if NULL act the same
> as
> + gBS->CreateEvent().
> + @param Event Pointer to the newly created event if the call
> + succeeds; undefined otherwise
> +
> + @retval EFI_SUCCESS The event structure was created
> + @retval EFI_INVALID_PARAMETER One of the parameters has an invalid
> value
> + @retval EFI_OUT_OF_RESOURCES The event could not be allocated
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +CommonCreateEventEx (
> + IN UINT32 Type,
> + IN EFI_TPL NotifyTpl,
> + IN EFI_EVENT_NOTIFY NotifyFunction, OPTIONAL
> + IN CONST VOID *NotifyContext, OPTIONAL
> + IN CONST EFI_GUID *EventGroup, OPTIONAL
> + OUT EFI_EVENT *Event
> + );
> +
> +
> /**
> Adds reserved memory, system memory, or memory-mapped I/O
> resources to the
> global coherency domain of the processor.
> diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf
> b/MdeModulePkg/Core/Dxe/DxeMain.inf
> index 61161bee28..44f90bcf4b 100644
> --- a/MdeModulePkg/Core/Dxe/DxeMain.inf
> +++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
> @@ -171,6 +171,8 @@
> gEfiVariableArchProtocolGuid ## CONSUMES
> gEfiCapsuleArchProtocolGuid ## CONSUMES
> gEfiWatchdogTimerArchProtocolGuid ## CONSUMES
> + gEdkiiCommonEventProtocolGuid ## SOMETIMES_CONSUMES
> + gEdkiiCpu2ProtocolGuid ## CONSUMES
>
> [Pcd]
>
> gEfiMdeModulePkgTokenSpaceGuid.PcdLoadFixAddressBootTimeCodePage
> Number ## SOMETIMES_CONSUMES
> diff --git a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
> b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
> index 514d1aa75a..8196d96daa 100644
> --- a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
> +++ b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
> @@ -13,12 +13,18 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> //
> EFI_HANDLE mDecompressHandle = NULL;
>
> +//
> +// Common event protocol
> +//
> +EFI_HANDLE mCommonEventHalde = NULL;
> +
> //
> // DXE Core globals for Architecture Protocols
> //
> EFI_SECURITY_ARCH_PROTOCOL *gSecurity = NULL;
> EFI_SECURITY2_ARCH_PROTOCOL *gSecurity2 = NULL;
> EFI_CPU_ARCH_PROTOCOL *gCpu = NULL;
> +EDKII_CPU2_PROTOCOL *gCpu2 = NULL;
> EFI_METRONOME_ARCH_PROTOCOL *gMetronome = NULL;
> EFI_TIMER_ARCH_PROTOCOL *gTimer = NULL;
> EFI_BDS_ARCH_PROTOCOL *gBds = NULL;
> @@ -211,6 +217,14 @@ EFI_DECOMPRESS_PROTOCOL gEfiDecompress = {
> DxeMainUefiDecompress
> };
>
> +//
> +// Common event protocol
> +//
> +EDKII_COMMON_EVENT_PROTOCOL gEdkiiCommonEvent = {
> + CommonCreateEventEx,
> + CommonWaitForEvent
> +};
> +
> //
> // For Loading modules at fixed address feature, the configuration table is
> to cache the top address below which to load
> // Runtime code&boot time code
> @@ -475,6 +489,14 @@ DxeMain (
> //
> CoreNotifyOnProtocolInstallation ();
>
> + Status = CoreInstallProtocolInterface (
> + &mCommonEventHalde,
> + &gEdkiiCommonEventProtocolGuid,
> + EFI_NATIVE_INTERFACE,
> + &gEdkiiCommonEvent
> + );
> + ASSERT_EFI_ERROR (Status);
> +
> //
> // Produce Firmware Volume Protocols, one for each FV in the HOB list.
> //
> diff --git a/MdeModulePkg/Core/Dxe/DxeMain/DxeProtocolNotify.c
> b/MdeModulePkg/Core/Dxe/DxeMain/DxeProtocolNotify.c
> index 29a55d02e6..57d52c250d 100644
> --- a/MdeModulePkg/Core/Dxe/DxeMain/DxeProtocolNotify.c
> +++ b/MdeModulePkg/Core/Dxe/DxeMain/DxeProtocolNotify.c
> @@ -41,6 +41,7 @@ EFI_CORE_PROTOCOL_NOTIFY_ENTRY
> mArchProtocols[] = {
> EFI_CORE_PROTOCOL_NOTIFY_ENTRY mOptionalProtocols[] = {
> { &gEfiSecurity2ArchProtocolGuid, (VOID **)&gSecurity2, NULL,
> NULL, FALSE },
> { &gEfiSmmBase2ProtocolGuid, (VOID **)&gSmmBase2, NULL,
> NULL, FALSE },
> + { &gEdkiiCpu2ProtocolGuid, (VOID **)&gCpu2, NULL, NULL,
> FALSE },
> { NULL, (VOID **)NULL, NULL, NULL, FALSE }
> };
>
> diff --git a/MdeModulePkg/Core/Dxe/Event/Event.c
> b/MdeModulePkg/Core/Dxe/Event/Event.c
> index 21db38aaf0..d271d2faf3 100644
> --- a/MdeModulePkg/Core/Dxe/Event/Event.c
> +++ b/MdeModulePkg/Core/Dxe/Event/Event.c
> @@ -14,7 +14,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> ///
> /// gEfiCurrentTpl - Current Task priority level
> ///
> -EFI_TPL gEfiCurrentTpl = TPL_APPLICATION;
> +volatile EFI_TPL gEfiCurrentTpl = TPL_APPLICATION;
>
> ///
> /// gEventQueueLock - Protects the event queues
> @@ -29,7 +29,7 @@ LIST_ENTRY gEventQueue[TPL_HIGH_LEVEL + 1];
> ///
> /// gEventPending - A bitmask of the EventQueues that are pending
> ///
> -UINTN gEventPending = 0;
> +volatile UINTN gEventPending = 0;
>
> ///
> /// gEventSignalQueue - A list of events to signal based on EventGroup
> type
> @@ -658,16 +658,44 @@ CoreWaitForEvent (
> OUT UINTN *UserIndex
> )
> {
> - EFI_STATUS Status;
> - UINTN Index;
> -
> - //
> + //
> // Can only WaitForEvent at TPL_APPLICATION
> //
> if (gEfiCurrentTpl != TPL_APPLICATION) {
> return EFI_UNSUPPORTED;
> }
>
> + return CommonWaitForEvent (NumberOfEvents, UserEvents, UserIndex);
> +}
> +
> +
> +/**
> + Stops execution until an event is signaled.
> +
> + Support all TPL.
> +
> + @param NumberOfEvents The number of events in the UserEvents
> array
> + @param UserEvents An array of EFI_EVENT
> + @param UserIndex Pointer to the index of the event which
> + satisfied the wait condition
> +
> + @retval EFI_SUCCESS The event indicated by Index was signaled.
> + @retval EFI_INVALID_PARAMETER The event indicated by Index has a
> notification
> + function or Event was not a valid type
> + @retval EFI_UNSUPPORTED The current TPL is not TPL_APPLICATION
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +CommonWaitForEvent (
> + IN UINTN NumberOfEvents,
> + IN EFI_EVENT *UserEvents,
> + OUT UINTN *UserIndex
> + )
> +{
> + EFI_STATUS Status;
> + UINTN Index;
> +
> if (NumberOfEvents == 0) {
> return EFI_INVALID_PARAMETER;
> }
> @@ -678,7 +706,7 @@ CoreWaitForEvent (
>
> for(;;) {
>
> - for(Index = 0; Index < NumberOfEvents; Index++) {
> + for (Index = 0; Index < NumberOfEvents; Index++) {
>
> Status = CoreCheckEvent (UserEvents[Index]);
>
> @@ -693,14 +721,66 @@ CoreWaitForEvent (
> }
> }
>
> - //
> - // Signal the Idle event
> - //
> - CoreSignalEvent (gIdleLoopEvent);
> + if (gCpu != NULL && gCpu2 != NULL) {
> + //
> + // None of the events checked above are ready/signaled yet,
> + // disable interrupts before checking for all pending events
> + //
> + gCpu->DisableInterrupt (gCpu);
> +
> + if (((UINTN) HighBitSet64 (gEventPending)) > gEfiCurrentTpl) {
> + //
> + // There are pending events, enable interrupts for these events to be
> processed
> + //
> + gCpu->EnableInterrupt (gCpu);
> + } else {
> + //
> + // No events are pending, enable interrupts, sleep the CPU and wait
> for an interrupt
> + //
> + gCpu2->EnableAndWaitForInterrupt (gCpu2);
> + }
> + }
> }
> }
>
>
> +/**
> + Creates an event in a group.
> +
> + Support all TPL.
> +
> + @param Type The type of event to create and its mode and
> + attributes
> + @param NotifyTpl The task priority level of event notifications
> + @param NotifyFunction Pointer to the events notification function
> + @param NotifyContext Pointer to the notification functions context;
> + corresponds to parameter "Context" in the
> + notification function
> + @param EventGroup GUID for EventGroup if NULL act the same
> as
> + gBS->CreateEvent().
> + @param Event Pointer to the newly created event if the call
> + succeeds; undefined otherwise
> +
> + @retval EFI_SUCCESS The event structure was created
> + @retval EFI_INVALID_PARAMETER One of the parameters has an invalid
> value
> + @retval EFI_OUT_OF_RESOURCES The event could not be allocated
> +
> +**/
> +EFI_STATUS
> +EFIAPI
> +CommonCreateEventEx (
> + IN UINT32 Type,
> + IN EFI_TPL NotifyTpl,
> + IN EFI_EVENT_NOTIFY NotifyFunction, OPTIONAL
> + IN CONST VOID *NotifyContext, OPTIONAL
> + IN CONST EFI_GUID *EventGroup, OPTIONAL
> + OUT EFI_EVENT *Event
> + )
> +{
> + return CoreCreateEventInternal (Type, NotifyTpl, NotifyFunction,
> NotifyContext, EventGroup, Event);
> +}
> +
> +
> /**
> Closes an event and frees the event structure.
>
> diff --git a/MdeModulePkg/Core/Dxe/Event/Event.h
> b/MdeModulePkg/Core/Dxe/Event/Event.h
> index 8141c5003e..050df26ec9 100644
> --- a/MdeModulePkg/Core/Dxe/Event/Event.h
> +++ b/MdeModulePkg/Core/Dxe/Event/Event.h
> @@ -12,7 +12,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
>
>
> #define VALID_TPL(a) ((a) <= TPL_HIGH_LEVEL)
> -extern UINTN gEventPending;
> +extern volatile UINTN gEventPending;
>
> ///
> /// Set if Event is part of an event group
> diff --git a/MdeModulePkg/Include/Protocol/Cpu2.h
> b/MdeModulePkg/Include/Protocol/Cpu2.h
> index 14464a38c8..81c5c8e424 100644
> --- a/MdeModulePkg/Include/Protocol/Cpu2.h
> +++ b/MdeModulePkg/Include/Protocol/Cpu2.h
> @@ -37,7 +37,7 @@ EFI_STATUS
> /// Foundation.
> ///
> struct _EDKII_CPU2_PROTOCOL {
> - EDKII_CPU_ENABLE_AND_WAIT_FOR_INTERRUPT
> EnableAndForWaitInterrupt;
> + EDKII_CPU_ENABLE_AND_WAIT_FOR_INTERRUPT
> EnableAndWaitForInterrupt;
> };
>
I think this change should belong to patch 1.
Regards,
Jian
> #endif
> --
> 2.21.0.windows.1
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V2 4/4] MdeModulePkg/DxeMain: Implement common event protocol
2019-07-09 9:23 ` Wang, Jian J
@ 2019-07-10 0:24 ` Gao, Zhichao
2019-07-10 8:46 ` Wang, Jian J
0 siblings, 1 reply; 15+ messages in thread
From: Gao, Zhichao @ 2019-07-10 0:24 UTC (permalink / raw)
To: Wang, Jian J, devel@edk2.groups.io
Cc: Sean Brogan, Wu, Hao A, Ni, Ray, Zeng, Star, Gao, Liming,
Michael Turner, Bret Barkelew
> -----Original Message-----
> From: Wang, Jian J
> Sent: Tuesday, July 9, 2019 5:24 PM
> To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
> Cc: Sean Brogan <sean.brogan@microsoft.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>; Michael Turner
> <Michael.Turner@microsoft.com>; Bret Barkelew
> <Bret.Barkelew@microsoft.com>
> Subject: RE: [PATCH V2 4/4] MdeModulePkg/DxeMain: Implement common
> event protocol
>
> Hi Zhichao,
>
> One common comment: please update copy right year.
I got the copyright remind before. Refer to https://edk2.groups.io/g/devel/topic/31828852#41576.
This patch is almost provided from MU project and contributed by Microsoft developers, 'From:...' is the author. And they didn't add their copyright. So I think it is fine to keep the copyright.
>
> See another comment at the almost the end of the email.
>
> > -----Original Message-----
> > From: Gao, Zhichao
> > Sent: Tuesday, July 09, 2019 4:40 PM
> > To: devel@edk2.groups.io
> > Cc: Sean Brogan <sean.brogan@microsoft.com>; 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>; Michael Turner
> <Michael.Turner@microsoft.com>;
> > Bret Barkelew <Bret.Barkelew@microsoft.com>
> > Subject: [PATCH V2 4/4] MdeModulePkg/DxeMain: Implement common
> event
> > protocol
> >
> > From: Sean Brogan <sean.brogan@microsoft.com>
> >
> > REF: 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. So it is required to check
> > the gEventPending with the interrupt disabled.
> >
> > Implement a protocol gEdkiiCommonEventProtocolGuid to support all TPL
> > event for PI drivers that use HW interrput. It has two interface, one
> > is to create event and the other one is to wait for event.
> >
> > Add 'volatile' specifier for gEfiCurrentTpl and gEventPending because
> > they may be changed out of the context (interrupt context).
> >
> > 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>
> > Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> > ---
> > MdeModulePkg/Core/Dxe/DxeMain.h | 64 ++++++++++-
> > MdeModulePkg/Core/Dxe/DxeMain.inf | 2 +
> > 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/Cpu2.h | 2 +-
> > 7 files changed, 181 insertions(+), 14 deletions(-)
> >
> > diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h
> > b/MdeModulePkg/Core/Dxe/DxeMain.h index 6a64852730..38907bfe8d
> 100644
> > --- a/MdeModulePkg/Core/Dxe/DxeMain.h
> > +++ b/MdeModulePkg/Core/Dxe/DxeMain.h
> > @@ -38,6 +38,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> > #include <Protocol/Security2.h> #include <Protocol/Reset.h> #include
> > <Protocol/Cpu.h>
> > +#include <Protocol/Cpu2.h>
> > #include <Protocol/Metronome.h>
> > #include <Protocol/FirmwareVolumeBlock.h> #include
> > <Protocol/Capsule.h> @@ -47,6 +48,7 @@ SPDX-License-Identifier:
> > BSD-2-Clause-Patent #include <Protocol/HiiPackageList.h> #include
> > <Protocol/SmmBase2.h> #include <Protocol/PeCoffImageEmulator.h>
> > +#include <Protocol/CommonEvent.h>
> > #include <Guid/MemoryTypeInformation.h> #include
> > <Guid/FirmwareFileSystem2.h> #include <Guid/FirmwareFileSystem3.h>
> @@
> > -274,10 +276,11 @@ extern EFI_METRONOME_ARCH_PROTOCOL
> *gMetronome;
> > extern EFI_TIMER_ARCH_PROTOCOL *gTimer;
> > extern EFI_SECURITY_ARCH_PROTOCOL *gSecurity;
> > extern EFI_SECURITY2_ARCH_PROTOCOL *gSecurity2;
> > +extern EDKII_CPU2_PROTOCOL *gCpu2;
> > extern EFI_BDS_ARCH_PROTOCOL *gBds;
> > extern EFI_SMM_BASE2_PROTOCOL *gSmmBase2;
> >
> > -extern EFI_TPL gEfiCurrentTpl;
> > +extern volatile EFI_TPL gEfiCurrentTpl;
> >
> > extern EFI_GUID *gDxeCoreFileName;
> > extern EFI_LOADED_IMAGE_PROTOCOL *gDxeCoreLoadedImage;
> > @@ -1714,6 +1717,65 @@ CoreCheckEvent (
> > );
> >
> >
> > +/**
> > + Stops execution until an event is signaled.
> > +
> > + Support all TPL.
> > +
> > + @param NumberOfEvents The number of events in the UserEvents
> > array
> > + @param UserEvents An array of EFI_EVENT
> > + @param UserIndex Pointer to the index of the event which
> > + satisfied the wait condition
> > +
> > + @retval EFI_SUCCESS The event indicated by Index was signaled.
> > + @retval EFI_INVALID_PARAMETER The event indicated by Index has a
> > notification
> > + function or Event was not a valid type
> > + @retval EFI_UNSUPPORTED The current TPL is not TPL_APPLICATION
> > +
> > +**/
> > +EFI_STATUS
> > +EFIAPI
> > +CommonWaitForEvent (
> > + IN UINTN NumberOfEvents,
> > + IN EFI_EVENT *UserEvents,
> > + OUT UINTN *UserIndex
> > + );
> > +
> > +
> > +/**
> > + Creates an event in a group.
> > +
> > + Support all TPL.
> > +
> > + @param Type The type of event to create and its mode and
> > + attributes
> > + @param NotifyTpl The task priority level of event notifications
> > + @param NotifyFunction Pointer to the events notification function
> > + @param NotifyContext Pointer to the notification functions context;
> > + corresponds to parameter "Context" in the
> > + notification function
> > + @param EventGroup GUID for EventGroup if NULL act the same
> > as
> > + gBS->CreateEvent().
> > + @param Event Pointer to the newly created event if the call
> > + succeeds; undefined otherwise
> > +
> > + @retval EFI_SUCCESS The event structure was created
> > + @retval EFI_INVALID_PARAMETER One of the parameters has an invalid
> > value
> > + @retval EFI_OUT_OF_RESOURCES The event could not be allocated
> > +
> > +**/
> > +EFI_STATUS
> > +EFIAPI
> > +CommonCreateEventEx (
> > + IN UINT32 Type,
> > + IN EFI_TPL NotifyTpl,
> > + IN EFI_EVENT_NOTIFY NotifyFunction, OPTIONAL
> > + IN CONST VOID *NotifyContext, OPTIONAL
> > + IN CONST EFI_GUID *EventGroup, OPTIONAL
> > + OUT EFI_EVENT *Event
> > + );
> > +
> > +
> > /**
> > Adds reserved memory, system memory, or memory-mapped I/O
> resources
> > to the
> > global coherency domain of the processor.
> > diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf
> > b/MdeModulePkg/Core/Dxe/DxeMain.inf
> > index 61161bee28..44f90bcf4b 100644
> > --- a/MdeModulePkg/Core/Dxe/DxeMain.inf
> > +++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
> > @@ -171,6 +171,8 @@
> > gEfiVariableArchProtocolGuid ## CONSUMES
> > gEfiCapsuleArchProtocolGuid ## CONSUMES
> > gEfiWatchdogTimerArchProtocolGuid ## CONSUMES
> > + gEdkiiCommonEventProtocolGuid ## SOMETIMES_CONSUMES
> > + gEdkiiCpu2ProtocolGuid ## CONSUMES
> >
> > [Pcd]
> >
> >
> gEfiMdeModulePkgTokenSpaceGuid.PcdLoadFixAddressBootTimeCodePage
> > Number ## SOMETIMES_CONSUMES
> > diff --git a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
> > b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
> > index 514d1aa75a..8196d96daa 100644
> > --- a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
> > +++ b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
> > @@ -13,12 +13,18 @@ SPDX-License-Identifier: BSD-2-Clause-Patent //
> > EFI_HANDLE mDecompressHandle = NULL;
> >
> > +//
> > +// Common event protocol
> > +//
> > +EFI_HANDLE mCommonEventHalde = NULL;
> > +
> > //
> > // DXE Core globals for Architecture Protocols //
> > EFI_SECURITY_ARCH_PROTOCOL *gSecurity = NULL;
> > EFI_SECURITY2_ARCH_PROTOCOL *gSecurity2 = NULL;
> > EFI_CPU_ARCH_PROTOCOL *gCpu = NULL;
> > +EDKII_CPU2_PROTOCOL *gCpu2 = NULL;
> > EFI_METRONOME_ARCH_PROTOCOL *gMetronome = NULL;
> > EFI_TIMER_ARCH_PROTOCOL *gTimer = NULL;
> > EFI_BDS_ARCH_PROTOCOL *gBds = NULL;
> > @@ -211,6 +217,14 @@ EFI_DECOMPRESS_PROTOCOL gEfiDecompress = {
> > DxeMainUefiDecompress
> > };
> >
> > +//
> > +// Common event protocol
> > +//
> > +EDKII_COMMON_EVENT_PROTOCOL gEdkiiCommonEvent = {
> > + CommonCreateEventEx,
> > + CommonWaitForEvent
> > +};
> > +
> > //
> > // For Loading modules at fixed address feature, the configuration
> > table is to cache the top address below which to load // Runtime
> > code&boot time code @@ -475,6 +489,14 @@ DxeMain (
> > //
> > CoreNotifyOnProtocolInstallation ();
> >
> > + Status = CoreInstallProtocolInterface (
> > + &mCommonEventHalde,
> > + &gEdkiiCommonEventProtocolGuid,
> > + EFI_NATIVE_INTERFACE,
> > + &gEdkiiCommonEvent
> > + );
> > + ASSERT_EFI_ERROR (Status);
> > +
> > //
> > // Produce Firmware Volume Protocols, one for each FV in the HOB list.
> > //
> > diff --git a/MdeModulePkg/Core/Dxe/DxeMain/DxeProtocolNotify.c
> > b/MdeModulePkg/Core/Dxe/DxeMain/DxeProtocolNotify.c
> > index 29a55d02e6..57d52c250d 100644
> > --- a/MdeModulePkg/Core/Dxe/DxeMain/DxeProtocolNotify.c
> > +++ b/MdeModulePkg/Core/Dxe/DxeMain/DxeProtocolNotify.c
> > @@ -41,6 +41,7 @@ EFI_CORE_PROTOCOL_NOTIFY_ENTRY
> mArchProtocols[] = {
> > EFI_CORE_PROTOCOL_NOTIFY_ENTRY mOptionalProtocols[] = {
> > { &gEfiSecurity2ArchProtocolGuid, (VOID **)&gSecurity2, NULL,
> > NULL, FALSE },
> > { &gEfiSmmBase2ProtocolGuid, (VOID **)&gSmmBase2, NULL,
> > NULL, FALSE },
> > + { &gEdkiiCpu2ProtocolGuid, (VOID **)&gCpu2, NULL, NULL,
> > FALSE },
> > { NULL, (VOID **)NULL, NULL, NULL, FALSE }
> > };
> >
> > diff --git a/MdeModulePkg/Core/Dxe/Event/Event.c
> > b/MdeModulePkg/Core/Dxe/Event/Event.c
> > index 21db38aaf0..d271d2faf3 100644
> > --- a/MdeModulePkg/Core/Dxe/Event/Event.c
> > +++ b/MdeModulePkg/Core/Dxe/Event/Event.c
> > @@ -14,7 +14,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent ///
> > /// gEfiCurrentTpl - Current Task priority level /// -EFI_TPL
> > gEfiCurrentTpl = TPL_APPLICATION;
> > +volatile EFI_TPL gEfiCurrentTpl = TPL_APPLICATION;
> >
> > ///
> > /// gEventQueueLock - Protects the event queues
> > @@ -29,7 +29,7 @@ LIST_ENTRY gEventQueue[TPL_HIGH_LEVEL + 1];
> > ///
> > /// gEventPending - A bitmask of the EventQueues that are pending
> > ///
> > -UINTN gEventPending = 0;
> > +volatile UINTN gEventPending = 0;
> >
> > ///
> > /// gEventSignalQueue - A list of events to signal based on
> > EventGroup type @@ -658,16 +658,44 @@ CoreWaitForEvent (
> > OUT UINTN *UserIndex
> > )
> > {
> > - EFI_STATUS Status;
> > - UINTN Index;
> > -
> > - //
> > + //
> > // Can only WaitForEvent at TPL_APPLICATION
> > //
> > if (gEfiCurrentTpl != TPL_APPLICATION) {
> > return EFI_UNSUPPORTED;
> > }
> >
> > + return CommonWaitForEvent (NumberOfEvents, UserEvents,
> UserIndex);
> > +}
> > +
> > +
> > +/**
> > + Stops execution until an event is signaled.
> > +
> > + Support all TPL.
> > +
> > + @param NumberOfEvents The number of events in the UserEvents
> > array
> > + @param UserEvents An array of EFI_EVENT
> > + @param UserIndex Pointer to the index of the event which
> > + satisfied the wait condition
> > +
> > + @retval EFI_SUCCESS The event indicated by Index was signaled.
> > + @retval EFI_INVALID_PARAMETER The event indicated by Index has a
> > notification
> > + function or Event was not a valid type
> > + @retval EFI_UNSUPPORTED The current TPL is not TPL_APPLICATION
> > +
> > +**/
> > +EFI_STATUS
> > +EFIAPI
> > +CommonWaitForEvent (
> > + IN UINTN NumberOfEvents,
> > + IN EFI_EVENT *UserEvents,
> > + OUT UINTN *UserIndex
> > + )
> > +{
> > + EFI_STATUS Status;
> > + UINTN Index;
> > +
> > if (NumberOfEvents == 0) {
> > return EFI_INVALID_PARAMETER;
> > }
> > @@ -678,7 +706,7 @@ CoreWaitForEvent (
> >
> > for(;;) {
> >
> > - for(Index = 0; Index < NumberOfEvents; Index++) {
> > + for (Index = 0; Index < NumberOfEvents; Index++) {
> >
> > Status = CoreCheckEvent (UserEvents[Index]);
> >
> > @@ -693,14 +721,66 @@ CoreWaitForEvent (
> > }
> > }
> >
> > - //
> > - // Signal the Idle event
> > - //
> > - CoreSignalEvent (gIdleLoopEvent);
> > + if (gCpu != NULL && gCpu2 != NULL) {
> > + //
> > + // None of the events checked above are ready/signaled yet,
> > + // disable interrupts before checking for all pending events
> > + //
> > + gCpu->DisableInterrupt (gCpu);
> > +
> > + if (((UINTN) HighBitSet64 (gEventPending)) > gEfiCurrentTpl) {
> > + //
> > + // There are pending events, enable interrupts for these
> > + events to be
> > processed
> > + //
> > + gCpu->EnableInterrupt (gCpu);
> > + } else {
> > + //
> > + // No events are pending, enable interrupts, sleep the CPU
> > + and wait
> > for an interrupt
> > + //
> > + gCpu2->EnableAndWaitForInterrupt (gCpu2);
> > + }
> > + }
> > }
> > }
> >
> >
> > +/**
> > + Creates an event in a group.
> > +
> > + Support all TPL.
> > +
> > + @param Type The type of event to create and its mode and
> > + attributes
> > + @param NotifyTpl The task priority level of event notifications
> > + @param NotifyFunction Pointer to the events notification function
> > + @param NotifyContext Pointer to the notification functions context;
> > + corresponds to parameter "Context" in the
> > + notification function
> > + @param EventGroup GUID for EventGroup if NULL act the same
> > as
> > + gBS->CreateEvent().
> > + @param Event Pointer to the newly created event if the call
> > + succeeds; undefined otherwise
> > +
> > + @retval EFI_SUCCESS The event structure was created
> > + @retval EFI_INVALID_PARAMETER One of the parameters has an invalid
> > value
> > + @retval EFI_OUT_OF_RESOURCES The event could not be allocated
> > +
> > +**/
> > +EFI_STATUS
> > +EFIAPI
> > +CommonCreateEventEx (
> > + IN UINT32 Type,
> > + IN EFI_TPL NotifyTpl,
> > + IN EFI_EVENT_NOTIFY NotifyFunction, OPTIONAL
> > + IN CONST VOID *NotifyContext, OPTIONAL
> > + IN CONST EFI_GUID *EventGroup, OPTIONAL
> > + OUT EFI_EVENT *Event
> > + )
> > +{
> > + return CoreCreateEventInternal (Type, NotifyTpl, NotifyFunction,
> > NotifyContext, EventGroup, Event);
> > +}
> > +
> > +
> > /**
> > Closes an event and frees the event structure.
> >
> > diff --git a/MdeModulePkg/Core/Dxe/Event/Event.h
> > b/MdeModulePkg/Core/Dxe/Event/Event.h
> > index 8141c5003e..050df26ec9 100644
> > --- a/MdeModulePkg/Core/Dxe/Event/Event.h
> > +++ b/MdeModulePkg/Core/Dxe/Event/Event.h
> > @@ -12,7 +12,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> >
> >
> > #define VALID_TPL(a) ((a) <= TPL_HIGH_LEVEL)
> > -extern UINTN gEventPending;
> > +extern volatile UINTN gEventPending;
> >
> > ///
> > /// Set if Event is part of an event group diff --git
> > a/MdeModulePkg/Include/Protocol/Cpu2.h
> > b/MdeModulePkg/Include/Protocol/Cpu2.h
> > index 14464a38c8..81c5c8e424 100644
> > --- a/MdeModulePkg/Include/Protocol/Cpu2.h
> > +++ b/MdeModulePkg/Include/Protocol/Cpu2.h
> > @@ -37,7 +37,7 @@ EFI_STATUS
> > /// Foundation.
> > ///
> > struct _EDKII_CPU2_PROTOCOL {
> > - EDKII_CPU_ENABLE_AND_WAIT_FOR_INTERRUPT
> > EnableAndForWaitInterrupt;
> > + EDKII_CPU_ENABLE_AND_WAIT_FOR_INTERRUPT
> > EnableAndWaitForInterrupt;
> > };
> >
>
> I think this change should belong to patch 1.
Yes. You are right. I would move the change to 1/4.
Thanks,
Zhichao
>
> Regards,
> Jian
>
> > #endif
> > --
> > 2.21.0.windows.1
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V2 4/4] MdeModulePkg/DxeMain: Implement common event protocol
2019-07-10 0:24 ` Gao, Zhichao
@ 2019-07-10 8:46 ` Wang, Jian J
2019-07-11 0:20 ` Gao, Zhichao
0 siblings, 1 reply; 15+ messages in thread
From: Wang, Jian J @ 2019-07-10 8:46 UTC (permalink / raw)
To: Gao, Zhichao, devel@edk2.groups.io
Cc: Sean Brogan, Wu, Hao A, Ni, Ray, Zeng, Star, Gao, Liming,
Michael Turner, Bret Barkelew
Zhichao,
> -----Original Message-----
> From: Gao, Zhichao
> Sent: Wednesday, July 10, 2019 8:24 AM
> To: Wang, Jian J <jian.j.wang@intel.com>; devel@edk2.groups.io
> Cc: Sean Brogan <sean.brogan@microsoft.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>; Michael
> Turner <Michael.Turner@microsoft.com>; Bret Barkelew
> <Bret.Barkelew@microsoft.com>
> Subject: RE: [PATCH V2 4/4] MdeModulePkg/DxeMain: Implement common
> event protocol
>
>
>
> > -----Original Message-----
> > From: Wang, Jian J
> > Sent: Tuesday, July 9, 2019 5:24 PM
> > To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
> > Cc: Sean Brogan <sean.brogan@microsoft.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>; Michael
> Turner
> > <Michael.Turner@microsoft.com>; Bret Barkelew
> > <Bret.Barkelew@microsoft.com>
> > Subject: RE: [PATCH V2 4/4] MdeModulePkg/DxeMain: Implement
> common
> > event protocol
> >
> > Hi Zhichao,
> >
> > One common comment: please update copy right year.
>
> I got the copyright remind before. Refer to
> https://edk2.groups.io/g/devel/topic/31828852#41576.
> This patch is almost provided from MU project and contributed by
> Microsoft developers, 'From:...' is the author. And they didn't add their
> copyright. So I think it is fine to keep the copyright.
>
I mean the *year* in Intel copyright not the whole copyright.
Regards,
Jian
> >
> > See another comment at the almost the end of the email.
> >
> > > -----Original Message-----
> > > From: Gao, Zhichao
> > > Sent: Tuesday, July 09, 2019 4:40 PM
> > > To: devel@edk2.groups.io
> > > Cc: Sean Brogan <sean.brogan@microsoft.com>; 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>; Michael Turner
> > <Michael.Turner@microsoft.com>;
> > > Bret Barkelew <Bret.Barkelew@microsoft.com>
> > > Subject: [PATCH V2 4/4] MdeModulePkg/DxeMain: Implement common
> > event
> > > protocol
> > >
> > > From: Sean Brogan <sean.brogan@microsoft.com>
> > >
> > > REF: 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. So it is required to check
> > > the gEventPending with the interrupt disabled.
> > >
> > > Implement a protocol gEdkiiCommonEventProtocolGuid to support all
> TPL
> > > event for PI drivers that use HW interrput. It has two interface, one
> > > is to create event and the other one is to wait for event.
> > >
> > > Add 'volatile' specifier for gEfiCurrentTpl and gEventPending because
> > > they may be changed out of the context (interrupt context).
> > >
> > > 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>
> > > Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> > > ---
> > > MdeModulePkg/Core/Dxe/DxeMain.h | 64 ++++++++++-
> > > MdeModulePkg/Core/Dxe/DxeMain.inf | 2 +
> > > 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/Cpu2.h | 2 +-
> > > 7 files changed, 181 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h
> > > b/MdeModulePkg/Core/Dxe/DxeMain.h index
> 6a64852730..38907bfe8d
> > 100644
> > > --- a/MdeModulePkg/Core/Dxe/DxeMain.h
> > > +++ b/MdeModulePkg/Core/Dxe/DxeMain.h
> > > @@ -38,6 +38,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> > > #include <Protocol/Security2.h> #include <Protocol/Reset.h> #include
> > > <Protocol/Cpu.h>
> > > +#include <Protocol/Cpu2.h>
> > > #include <Protocol/Metronome.h>
> > > #include <Protocol/FirmwareVolumeBlock.h> #include
> > > <Protocol/Capsule.h> @@ -47,6 +48,7 @@ SPDX-License-Identifier:
> > > BSD-2-Clause-Patent #include <Protocol/HiiPackageList.h> #include
> > > <Protocol/SmmBase2.h> #include <Protocol/PeCoffImageEmulator.h>
> > > +#include <Protocol/CommonEvent.h>
> > > #include <Guid/MemoryTypeInformation.h> #include
> > > <Guid/FirmwareFileSystem2.h> #include
> <Guid/FirmwareFileSystem3.h>
> > @@
> > > -274,10 +276,11 @@ extern EFI_METRONOME_ARCH_PROTOCOL
> > *gMetronome;
> > > extern EFI_TIMER_ARCH_PROTOCOL *gTimer;
> > > extern EFI_SECURITY_ARCH_PROTOCOL *gSecurity;
> > > extern EFI_SECURITY2_ARCH_PROTOCOL *gSecurity2;
> > > +extern EDKII_CPU2_PROTOCOL *gCpu2;
> > > extern EFI_BDS_ARCH_PROTOCOL *gBds;
> > > extern EFI_SMM_BASE2_PROTOCOL *gSmmBase2;
> > >
> > > -extern EFI_TPL gEfiCurrentTpl;
> > > +extern volatile EFI_TPL gEfiCurrentTpl;
> > >
> > > extern EFI_GUID *gDxeCoreFileName;
> > > extern EFI_LOADED_IMAGE_PROTOCOL
> *gDxeCoreLoadedImage;
> > > @@ -1714,6 +1717,65 @@ CoreCheckEvent (
> > > );
> > >
> > >
> > > +/**
> > > + Stops execution until an event is signaled.
> > > +
> > > + Support all TPL.
> > > +
> > > + @param NumberOfEvents The number of events in the
> UserEvents
> > > array
> > > + @param UserEvents An array of EFI_EVENT
> > > + @param UserIndex Pointer to the index of the event which
> > > + satisfied the wait condition
> > > +
> > > + @retval EFI_SUCCESS The event indicated by Index was
> signaled.
> > > + @retval EFI_INVALID_PARAMETER The event indicated by Index has a
> > > notification
> > > + function or Event was not a valid type
> > > + @retval EFI_UNSUPPORTED The current TPL is not
> TPL_APPLICATION
> > > +
> > > +**/
> > > +EFI_STATUS
> > > +EFIAPI
> > > +CommonWaitForEvent (
> > > + IN UINTN NumberOfEvents,
> > > + IN EFI_EVENT *UserEvents,
> > > + OUT UINTN *UserIndex
> > > + );
> > > +
> > > +
> > > +/**
> > > + Creates an event in a group.
> > > +
> > > + Support all TPL.
> > > +
> > > + @param Type The type of event to create and its mode
> and
> > > + attributes
> > > + @param NotifyTpl The task priority level of event
> notifications
> > > + @param NotifyFunction Pointer to the events notification
> function
> > > + @param NotifyContext Pointer to the notification functions
> context;
> > > + corresponds to parameter "Context" in the
> > > + notification function
> > > + @param EventGroup GUID for EventGroup if NULL act the
> same
> > > as
> > > + gBS->CreateEvent().
> > > + @param Event Pointer to the newly created event if the
> call
> > > + succeeds; undefined otherwise
> > > +
> > > + @retval EFI_SUCCESS The event structure was created
> > > + @retval EFI_INVALID_PARAMETER One of the parameters has an
> invalid
> > > value
> > > + @retval EFI_OUT_OF_RESOURCES The event could not be allocated
> > > +
> > > +**/
> > > +EFI_STATUS
> > > +EFIAPI
> > > +CommonCreateEventEx (
> > > + IN UINT32 Type,
> > > + IN EFI_TPL NotifyTpl,
> > > + IN EFI_EVENT_NOTIFY NotifyFunction, OPTIONAL
> > > + IN CONST VOID *NotifyContext, OPTIONAL
> > > + IN CONST EFI_GUID *EventGroup, OPTIONAL
> > > + OUT EFI_EVENT *Event
> > > + );
> > > +
> > > +
> > > /**
> > > Adds reserved memory, system memory, or memory-mapped I/O
> > resources
> > > to the
> > > global coherency domain of the processor.
> > > diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf
> > > b/MdeModulePkg/Core/Dxe/DxeMain.inf
> > > index 61161bee28..44f90bcf4b 100644
> > > --- a/MdeModulePkg/Core/Dxe/DxeMain.inf
> > > +++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
> > > @@ -171,6 +171,8 @@
> > > gEfiVariableArchProtocolGuid ## CONSUMES
> > > gEfiCapsuleArchProtocolGuid ## CONSUMES
> > > gEfiWatchdogTimerArchProtocolGuid ## CONSUMES
> > > + gEdkiiCommonEventProtocolGuid ##
> SOMETIMES_CONSUMES
> > > + gEdkiiCpu2ProtocolGuid ## CONSUMES
> > >
> > > [Pcd]
> > >
> > >
> >
> gEfiMdeModulePkgTokenSpaceGuid.PcdLoadFixAddressBootTimeCodePage
> > > Number ## SOMETIMES_CONSUMES
> > > diff --git a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
> > > b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
> > > index 514d1aa75a..8196d96daa 100644
> > > --- a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
> > > +++ b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
> > > @@ -13,12 +13,18 @@ SPDX-License-Identifier: BSD-2-Clause-Patent //
> > > EFI_HANDLE mDecompressHandle = NULL;
> > >
> > > +//
> > > +// Common event protocol
> > > +//
> > > +EFI_HANDLE mCommonEventHalde = NULL;
> > > +
> > > //
> > > // DXE Core globals for Architecture Protocols //
> > > EFI_SECURITY_ARCH_PROTOCOL *gSecurity = NULL;
> > > EFI_SECURITY2_ARCH_PROTOCOL *gSecurity2 = NULL;
> > > EFI_CPU_ARCH_PROTOCOL *gCpu = NULL;
> > > +EDKII_CPU2_PROTOCOL *gCpu2 = NULL;
> > > EFI_METRONOME_ARCH_PROTOCOL *gMetronome = NULL;
> > > EFI_TIMER_ARCH_PROTOCOL *gTimer = NULL;
> > > EFI_BDS_ARCH_PROTOCOL *gBds = NULL;
> > > @@ -211,6 +217,14 @@ EFI_DECOMPRESS_PROTOCOL
> gEfiDecompress = {
> > > DxeMainUefiDecompress
> > > };
> > >
> > > +//
> > > +// Common event protocol
> > > +//
> > > +EDKII_COMMON_EVENT_PROTOCOL gEdkiiCommonEvent = {
> > > + CommonCreateEventEx,
> > > + CommonWaitForEvent
> > > +};
> > > +
> > > //
> > > // For Loading modules at fixed address feature, the configuration
> > > table is to cache the top address below which to load // Runtime
> > > code&boot time code @@ -475,6 +489,14 @@ DxeMain (
> > > //
> > > CoreNotifyOnProtocolInstallation ();
> > >
> > > + Status = CoreInstallProtocolInterface (
> > > + &mCommonEventHalde,
> > > + &gEdkiiCommonEventProtocolGuid,
> > > + EFI_NATIVE_INTERFACE,
> > > + &gEdkiiCommonEvent
> > > + );
> > > + ASSERT_EFI_ERROR (Status);
> > > +
> > > //
> > > // Produce Firmware Volume Protocols, one for each FV in the HOB
> list.
> > > //
> > > diff --git a/MdeModulePkg/Core/Dxe/DxeMain/DxeProtocolNotify.c
> > > b/MdeModulePkg/Core/Dxe/DxeMain/DxeProtocolNotify.c
> > > index 29a55d02e6..57d52c250d 100644
> > > --- a/MdeModulePkg/Core/Dxe/DxeMain/DxeProtocolNotify.c
> > > +++ b/MdeModulePkg/Core/Dxe/DxeMain/DxeProtocolNotify.c
> > > @@ -41,6 +41,7 @@ EFI_CORE_PROTOCOL_NOTIFY_ENTRY
> > mArchProtocols[] = {
> > > EFI_CORE_PROTOCOL_NOTIFY_ENTRY mOptionalProtocols[] = {
> > > { &gEfiSecurity2ArchProtocolGuid, (VOID **)&gSecurity2, NULL,
> > > NULL, FALSE },
> > > { &gEfiSmmBase2ProtocolGuid, (VOID **)&gSmmBase2,
> NULL,
> > > NULL, FALSE },
> > > + { &gEdkiiCpu2ProtocolGuid, (VOID **)&gCpu2, NULL,
> NULL,
> > > FALSE },
> > > { NULL, (VOID **)NULL, NULL, NULL, FALSE }
> > > };
> > >
> > > diff --git a/MdeModulePkg/Core/Dxe/Event/Event.c
> > > b/MdeModulePkg/Core/Dxe/Event/Event.c
> > > index 21db38aaf0..d271d2faf3 100644
> > > --- a/MdeModulePkg/Core/Dxe/Event/Event.c
> > > +++ b/MdeModulePkg/Core/Dxe/Event/Event.c
> > > @@ -14,7 +14,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent ///
> > > /// gEfiCurrentTpl - Current Task priority level /// -EFI_TPL
> > > gEfiCurrentTpl = TPL_APPLICATION;
> > > +volatile EFI_TPL gEfiCurrentTpl = TPL_APPLICATION;
> > >
> > > ///
> > > /// gEventQueueLock - Protects the event queues
> > > @@ -29,7 +29,7 @@ LIST_ENTRY gEventQueue[TPL_HIGH_LEVEL + 1];
> > > ///
> > > /// gEventPending - A bitmask of the EventQueues that are pending
> > > ///
> > > -UINTN gEventPending = 0;
> > > +volatile UINTN gEventPending = 0;
> > >
> > > ///
> > > /// gEventSignalQueue - A list of events to signal based on
> > > EventGroup type @@ -658,16 +658,44 @@ CoreWaitForEvent (
> > > OUT UINTN *UserIndex
> > > )
> > > {
> > > - EFI_STATUS Status;
> > > - UINTN Index;
> > > -
> > > - //
> > > + //
> > > // Can only WaitForEvent at TPL_APPLICATION
> > > //
> > > if (gEfiCurrentTpl != TPL_APPLICATION) {
> > > return EFI_UNSUPPORTED;
> > > }
> > >
> > > + return CommonWaitForEvent (NumberOfEvents, UserEvents,
> > UserIndex);
> > > +}
> > > +
> > > +
> > > +/**
> > > + Stops execution until an event is signaled.
> > > +
> > > + Support all TPL.
> > > +
> > > + @param NumberOfEvents The number of events in the
> UserEvents
> > > array
> > > + @param UserEvents An array of EFI_EVENT
> > > + @param UserIndex Pointer to the index of the event which
> > > + satisfied the wait condition
> > > +
> > > + @retval EFI_SUCCESS The event indicated by Index was
> signaled.
> > > + @retval EFI_INVALID_PARAMETER The event indicated by Index has a
> > > notification
> > > + function or Event was not a valid type
> > > + @retval EFI_UNSUPPORTED The current TPL is not
> TPL_APPLICATION
> > > +
> > > +**/
> > > +EFI_STATUS
> > > +EFIAPI
> > > +CommonWaitForEvent (
> > > + IN UINTN NumberOfEvents,
> > > + IN EFI_EVENT *UserEvents,
> > > + OUT UINTN *UserIndex
> > > + )
> > > +{
> > > + EFI_STATUS Status;
> > > + UINTN Index;
> > > +
> > > if (NumberOfEvents == 0) {
> > > return EFI_INVALID_PARAMETER;
> > > }
> > > @@ -678,7 +706,7 @@ CoreWaitForEvent (
> > >
> > > for(;;) {
> > >
> > > - for(Index = 0; Index < NumberOfEvents; Index++) {
> > > + for (Index = 0; Index < NumberOfEvents; Index++) {
> > >
> > > Status = CoreCheckEvent (UserEvents[Index]);
> > >
> > > @@ -693,14 +721,66 @@ CoreWaitForEvent (
> > > }
> > > }
> > >
> > > - //
> > > - // Signal the Idle event
> > > - //
> > > - CoreSignalEvent (gIdleLoopEvent);
> > > + if (gCpu != NULL && gCpu2 != NULL) {
> > > + //
> > > + // None of the events checked above are ready/signaled yet,
> > > + // disable interrupts before checking for all pending events
> > > + //
> > > + gCpu->DisableInterrupt (gCpu);
> > > +
> > > + if (((UINTN) HighBitSet64 (gEventPending)) > gEfiCurrentTpl) {
> > > + //
> > > + // There are pending events, enable interrupts for these
> > > + events to be
> > > processed
> > > + //
> > > + gCpu->EnableInterrupt (gCpu);
> > > + } else {
> > > + //
> > > + // No events are pending, enable interrupts, sleep the CPU
> > > + and wait
> > > for an interrupt
> > > + //
> > > + gCpu2->EnableAndWaitForInterrupt (gCpu2);
> > > + }
> > > + }
> > > }
> > > }
> > >
> > >
> > > +/**
> > > + Creates an event in a group.
> > > +
> > > + Support all TPL.
> > > +
> > > + @param Type The type of event to create and its mode
> and
> > > + attributes
> > > + @param NotifyTpl The task priority level of event
> notifications
> > > + @param NotifyFunction Pointer to the events notification
> function
> > > + @param NotifyContext Pointer to the notification functions
> context;
> > > + corresponds to parameter "Context" in the
> > > + notification function
> > > + @param EventGroup GUID for EventGroup if NULL act the
> same
> > > as
> > > + gBS->CreateEvent().
> > > + @param Event Pointer to the newly created event if the
> call
> > > + succeeds; undefined otherwise
> > > +
> > > + @retval EFI_SUCCESS The event structure was created
> > > + @retval EFI_INVALID_PARAMETER One of the parameters has an
> invalid
> > > value
> > > + @retval EFI_OUT_OF_RESOURCES The event could not be allocated
> > > +
> > > +**/
> > > +EFI_STATUS
> > > +EFIAPI
> > > +CommonCreateEventEx (
> > > + IN UINT32 Type,
> > > + IN EFI_TPL NotifyTpl,
> > > + IN EFI_EVENT_NOTIFY NotifyFunction, OPTIONAL
> > > + IN CONST VOID *NotifyContext, OPTIONAL
> > > + IN CONST EFI_GUID *EventGroup, OPTIONAL
> > > + OUT EFI_EVENT *Event
> > > + )
> > > +{
> > > + return CoreCreateEventInternal (Type, NotifyTpl, NotifyFunction,
> > > NotifyContext, EventGroup, Event);
> > > +}
> > > +
> > > +
> > > /**
> > > Closes an event and frees the event structure.
> > >
> > > diff --git a/MdeModulePkg/Core/Dxe/Event/Event.h
> > > b/MdeModulePkg/Core/Dxe/Event/Event.h
> > > index 8141c5003e..050df26ec9 100644
> > > --- a/MdeModulePkg/Core/Dxe/Event/Event.h
> > > +++ b/MdeModulePkg/Core/Dxe/Event/Event.h
> > > @@ -12,7 +12,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> > >
> > >
> > > #define VALID_TPL(a) ((a) <= TPL_HIGH_LEVEL)
> > > -extern UINTN gEventPending;
> > > +extern volatile UINTN gEventPending;
> > >
> > > ///
> > > /// Set if Event is part of an event group diff --git
> > > a/MdeModulePkg/Include/Protocol/Cpu2.h
> > > b/MdeModulePkg/Include/Protocol/Cpu2.h
> > > index 14464a38c8..81c5c8e424 100644
> > > --- a/MdeModulePkg/Include/Protocol/Cpu2.h
> > > +++ b/MdeModulePkg/Include/Protocol/Cpu2.h
> > > @@ -37,7 +37,7 @@ EFI_STATUS
> > > /// Foundation.
> > > ///
> > > struct _EDKII_CPU2_PROTOCOL {
> > > - EDKII_CPU_ENABLE_AND_WAIT_FOR_INTERRUPT
> > > EnableAndForWaitInterrupt;
> > > + EDKII_CPU_ENABLE_AND_WAIT_FOR_INTERRUPT
> > > EnableAndWaitForInterrupt;
> > > };
> > >
> >
> > I think this change should belong to patch 1.
>
> Yes. You are right. I would move the change to 1/4.
>
> Thanks,
> Zhichao
>
> >
> > Regards,
> > Jian
> >
> > > #endif
> > > --
> > > 2.21.0.windows.1
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [edk2-devel] [PATCH V2 0/4] Fix race condition and add event protocol
2019-07-09 8:39 [PATCH V2 0/4] Fix race condition and add event protocol Gao, Zhichao
` (3 preceding siblings ...)
2019-07-09 8:39 ` [PATCH V2 4/4] MdeModulePkg/DxeMain: Implement common event protocol Gao, Zhichao
@ 2019-07-10 12:20 ` Laszlo Ersek
4 siblings, 0 replies; 15+ messages in thread
From: Laszlo Ersek @ 2019-07-10 12:20 UTC (permalink / raw)
To: devel, zhichao.gao
Cc: Jian J Wang, Hao A Wu, Ray Ni, Star Zeng, Liming gao, Sean Brogan,
Michael Turner, Bret Barkelew, Michael D Kinney, Eric Dong,
Leif Lindholm, Ard Biesheuvel
On 07/09/19 10:39, Gao, Zhichao wrote:
> V1:
> 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.
>
> V2:
> Move EnableInterruptsAndSleep to UefiCpuPkg/CpuDxe. It is inappropiate
> to add it to MdePkg which should only contain code base on the UEFI spec.
> Change the name gEfiCpu2ProtocolGuid to gEdkiiCpu2ProtocolGuid.
Thanks for the CC.
* On patch #2, I'll defer to Eric and Ray.
* From the problem description, and from the final fix in patch #4, it
seems like the issue (the race condition) applies to all edk2
architectures. Therefore I would suggest filing a TianoCore Feature
Request BZ for "ArmPkg/Drivers/CpuDxe" as well, for producing
gEdkiiCpu2ProtocolGuid. (CC'ing Leif and Ard.)
(I do see that the patch series does not *require* the new protocol, so
it's fine to implement the ARM/AARCH64 support separately, in my opinion.)
Thanks
Laszlo
> 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>
>
> Sean Brogan (3):
> MdeModulePkg: Add gEdkiiCpu2ProtocolGuid and header file
> UefiCpuPkg/CpuDxe: Implement Cpu2 protocol
> MdeModulePkg/DxeMain: Implement common event protocol
>
> Zhichao Gao (1):
> MdeModulePkg: Add gEdkiiCommonEventProtocolGuid for creating event
>
> MdeModulePkg/Core/Dxe/DxeMain.h | 64 ++++++++++-
> MdeModulePkg/Core/Dxe/DxeMain.inf | 2 +
> 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/Include/Protocol/Cpu2.h | 43 ++++++++
> MdeModulePkg/MdeModulePkg.dec | 6 ++
> UefiCpuPkg/CpuDxe/CpuDxe.c | 38 +++++++
> UefiCpuPkg/CpuDxe/CpuDxe.h | 25 +++++
> UefiCpuPkg/CpuDxe/CpuDxe.inf | 3 +
> .../CpuDxe/Ia32/EnableInterruptsAndSleep.c | 24 +++++
> .../CpuDxe/X64/EnableInterruptsAndSleep.nasm | 31 ++++++
> 14 files changed, 368 insertions(+), 13 deletions(-)
> create mode 100644 MdeModulePkg/Include/Protocol/CommonEvent.h
> create mode 100644 MdeModulePkg/Include/Protocol/Cpu2.h
> create mode 100644 UefiCpuPkg/CpuDxe/Ia32/EnableInterruptsAndSleep.c
> create mode 100644 UefiCpuPkg/CpuDxe/X64/EnableInterruptsAndSleep.nasm
>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V2 4/4] MdeModulePkg/DxeMain: Implement common event protocol
2019-07-10 8:46 ` Wang, Jian J
@ 2019-07-11 0:20 ` Gao, Zhichao
2019-07-11 1:26 ` Wang, Jian J
0 siblings, 1 reply; 15+ messages in thread
From: Gao, Zhichao @ 2019-07-11 0:20 UTC (permalink / raw)
To: Wang, Jian J, devel@edk2.groups.io
Cc: Sean Brogan, Wu, Hao A, Ni, Ray, Zeng, Star, Gao, Liming,
Michael Turner, Bret Barkelew
> -----Original Message-----
> From: Wang, Jian J
> Sent: Wednesday, July 10, 2019 4:47 PM
> To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
> Cc: Sean Brogan <sean.brogan@microsoft.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>; Michael Turner
> <Michael.Turner@microsoft.com>; Bret Barkelew
> <Bret.Barkelew@microsoft.com>
> Subject: RE: [PATCH V2 4/4] MdeModulePkg/DxeMain: Implement common
> event protocol
>
> Zhichao,
>
> > -----Original Message-----
> > From: Gao, Zhichao
> > Sent: Wednesday, July 10, 2019 8:24 AM
> > To: Wang, Jian J <jian.j.wang@intel.com>; devel@edk2.groups.io
> > Cc: Sean Brogan <sean.brogan@microsoft.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>; Michael
> > Turner <Michael.Turner@microsoft.com>; Bret Barkelew
> > <Bret.Barkelew@microsoft.com>
> > Subject: RE: [PATCH V2 4/4] MdeModulePkg/DxeMain: Implement
> common
> > event protocol
> >
> >
> >
> > > -----Original Message-----
> > > From: Wang, Jian J
> > > Sent: Tuesday, July 9, 2019 5:24 PM
> > > To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
> > > Cc: Sean Brogan <sean.brogan@microsoft.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>; Michael
> > Turner
> > > <Michael.Turner@microsoft.com>; Bret Barkelew
> > > <Bret.Barkelew@microsoft.com>
> > > Subject: RE: [PATCH V2 4/4] MdeModulePkg/DxeMain: Implement
> > common
> > > event protocol
> > >
> > > Hi Zhichao,
> > >
> > > One common comment: please update copy right year.
> >
> > I got the copyright remind before. Refer to
> > https://edk2.groups.io/g/devel/topic/31828852#41576.
> > This patch is almost provided from MU project and contributed by
> > Microsoft developers, 'From:...' is the author. And they didn't add
> > their copyright. So I think it is fine to keep the copyright.
> >
>
> I mean the *year* in Intel copyright not the whole copyright.
https://edk2.groups.io/g/devel/message/41599
https://edk2.groups.io/g/devel/message/41753
See above message. Maybe updating the year means you are making a legal claim to the intellectual
property provided by the patch on behalf of the company.
Thanks,
Zhichao
>
> Regards,
> Jian
>
> > >
> > > See another comment at the almost the end of the email.
> > >
> > > > -----Original Message-----
> > > > From: Gao, Zhichao
> > > > Sent: Tuesday, July 09, 2019 4:40 PM
> > > > To: devel@edk2.groups.io
> > > > Cc: Sean Brogan <sean.brogan@microsoft.com>; 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>; Michael Turner
> > > <Michael.Turner@microsoft.com>;
> > > > Bret Barkelew <Bret.Barkelew@microsoft.com>
> > > > Subject: [PATCH V2 4/4] MdeModulePkg/DxeMain: Implement
> common
> > > event
> > > > protocol
> > > >
> > > > From: Sean Brogan <sean.brogan@microsoft.com>
> > > >
> > > > REF: 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. So it is required
> > > > to check the gEventPending with the interrupt disabled.
> > > >
> > > > Implement a protocol gEdkiiCommonEventProtocolGuid to support all
> > TPL
> > > > event for PI drivers that use HW interrput. It has two interface,
> > > > one is to create event and the other one is to wait for event.
> > > >
> > > > Add 'volatile' specifier for gEfiCurrentTpl and gEventPending
> > > > because they may be changed out of the context (interrupt context).
> > > >
> > > > 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>
> > > > Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> > > > ---
> > > > MdeModulePkg/Core/Dxe/DxeMain.h | 64 ++++++++++-
> > > > MdeModulePkg/Core/Dxe/DxeMain.inf | 2 +
> > > > 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/Cpu2.h | 2 +-
> > > > 7 files changed, 181 insertions(+), 14 deletions(-)
> > > >
> > > > diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h
> > > > b/MdeModulePkg/Core/Dxe/DxeMain.h index
> > 6a64852730..38907bfe8d
> > > 100644
> > > > --- a/MdeModulePkg/Core/Dxe/DxeMain.h
> > > > +++ b/MdeModulePkg/Core/Dxe/DxeMain.h
> > > > @@ -38,6 +38,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> > > > #include <Protocol/Security2.h> #include <Protocol/Reset.h>
> > > > #include <Protocol/Cpu.h>
> > > > +#include <Protocol/Cpu2.h>
> > > > #include <Protocol/Metronome.h>
> > > > #include <Protocol/FirmwareVolumeBlock.h> #include
> > > > <Protocol/Capsule.h> @@ -47,6 +48,7 @@ SPDX-License-Identifier:
> > > > BSD-2-Clause-Patent #include <Protocol/HiiPackageList.h>
> > > > #include <Protocol/SmmBase2.h> #include
> > > > <Protocol/PeCoffImageEmulator.h>
> > > > +#include <Protocol/CommonEvent.h>
> > > > #include <Guid/MemoryTypeInformation.h> #include
> > > > <Guid/FirmwareFileSystem2.h> #include
> > <Guid/FirmwareFileSystem3.h>
> > > @@
> > > > -274,10 +276,11 @@ extern EFI_METRONOME_ARCH_PROTOCOL
> > > *gMetronome;
> > > > extern EFI_TIMER_ARCH_PROTOCOL *gTimer;
> > > > extern EFI_SECURITY_ARCH_PROTOCOL *gSecurity;
> > > > extern EFI_SECURITY2_ARCH_PROTOCOL *gSecurity2;
> > > > +extern EDKII_CPU2_PROTOCOL *gCpu2;
> > > > extern EFI_BDS_ARCH_PROTOCOL *gBds;
> > > > extern EFI_SMM_BASE2_PROTOCOL *gSmmBase2;
> > > >
> > > > -extern EFI_TPL gEfiCurrentTpl;
> > > > +extern volatile EFI_TPL gEfiCurrentTpl;
> > > >
> > > > extern EFI_GUID *gDxeCoreFileName;
> > > > extern EFI_LOADED_IMAGE_PROTOCOL
> > *gDxeCoreLoadedImage;
> > > > @@ -1714,6 +1717,65 @@ CoreCheckEvent (
> > > > );
> > > >
> > > >
> > > > +/**
> > > > + Stops execution until an event is signaled.
> > > > +
> > > > + Support all TPL.
> > > > +
> > > > + @param NumberOfEvents The number of events in the
> > UserEvents
> > > > array
> > > > + @param UserEvents An array of EFI_EVENT
> > > > + @param UserIndex Pointer to the index of the event which
> > > > + satisfied the wait condition
> > > > +
> > > > + @retval EFI_SUCCESS The event indicated by Index was
> > signaled.
> > > > + @retval EFI_INVALID_PARAMETER The event indicated by Index has
> > > > + a
> > > > notification
> > > > + function or Event was not a valid type
> > > > + @retval EFI_UNSUPPORTED The current TPL is not
> > TPL_APPLICATION
> > > > +
> > > > +**/
> > > > +EFI_STATUS
> > > > +EFIAPI
> > > > +CommonWaitForEvent (
> > > > + IN UINTN NumberOfEvents,
> > > > + IN EFI_EVENT *UserEvents,
> > > > + OUT UINTN *UserIndex
> > > > + );
> > > > +
> > > > +
> > > > +/**
> > > > + Creates an event in a group.
> > > > +
> > > > + Support all TPL.
> > > > +
> > > > + @param Type The type of event to create and its mode
> > and
> > > > + attributes
> > > > + @param NotifyTpl The task priority level of event
> > notifications
> > > > + @param NotifyFunction Pointer to the events notification
> > function
> > > > + @param NotifyContext Pointer to the notification functions
> > context;
> > > > + corresponds to parameter "Context" in the
> > > > + notification function
> > > > + @param EventGroup GUID for EventGroup if NULL act the
> > same
> > > > as
> > > > + gBS->CreateEvent().
> > > > + @param Event Pointer to the newly created event if the
> > call
> > > > + succeeds; undefined otherwise
> > > > +
> > > > + @retval EFI_SUCCESS The event structure was created
> > > > + @retval EFI_INVALID_PARAMETER One of the parameters has an
> > invalid
> > > > value
> > > > + @retval EFI_OUT_OF_RESOURCES The event could not be allocated
> > > > +
> > > > +**/
> > > > +EFI_STATUS
> > > > +EFIAPI
> > > > +CommonCreateEventEx (
> > > > + IN UINT32 Type,
> > > > + IN EFI_TPL NotifyTpl,
> > > > + IN EFI_EVENT_NOTIFY NotifyFunction, OPTIONAL
> > > > + IN CONST VOID *NotifyContext, OPTIONAL
> > > > + IN CONST EFI_GUID *EventGroup, OPTIONAL
> > > > + OUT EFI_EVENT *Event
> > > > + );
> > > > +
> > > > +
> > > > /**
> > > > Adds reserved memory, system memory, or memory-mapped I/O
> > > resources
> > > > to the
> > > > global coherency domain of the processor.
> > > > diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf
> > > > b/MdeModulePkg/Core/Dxe/DxeMain.inf
> > > > index 61161bee28..44f90bcf4b 100644
> > > > --- a/MdeModulePkg/Core/Dxe/DxeMain.inf
> > > > +++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
> > > > @@ -171,6 +171,8 @@
> > > > gEfiVariableArchProtocolGuid ## CONSUMES
> > > > gEfiCapsuleArchProtocolGuid ## CONSUMES
> > > > gEfiWatchdogTimerArchProtocolGuid ## CONSUMES
> > > > + gEdkiiCommonEventProtocolGuid ##
> > SOMETIMES_CONSUMES
> > > > + gEdkiiCpu2ProtocolGuid ## CONSUMES
> > > >
> > > > [Pcd]
> > > >
> > > >
> > >
> >
> gEfiMdeModulePkgTokenSpaceGuid.PcdLoadFixAddressBootTimeCodePage
> > > > Number ## SOMETIMES_CONSUMES
> > > > diff --git a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
> > > > b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
> > > > index 514d1aa75a..8196d96daa 100644
> > > > --- a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
> > > > +++ b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
> > > > @@ -13,12 +13,18 @@ SPDX-License-Identifier: BSD-2-Clause-Patent //
> > > > EFI_HANDLE mDecompressHandle = NULL;
> > > >
> > > > +//
> > > > +// Common event protocol
> > > > +//
> > > > +EFI_HANDLE mCommonEventHalde = NULL;
> > > > +
> > > > //
> > > > // DXE Core globals for Architecture Protocols //
> > > > EFI_SECURITY_ARCH_PROTOCOL *gSecurity = NULL;
> > > > EFI_SECURITY2_ARCH_PROTOCOL *gSecurity2 = NULL;
> > > > EFI_CPU_ARCH_PROTOCOL *gCpu = NULL;
> > > > +EDKII_CPU2_PROTOCOL *gCpu2 = NULL;
> > > > EFI_METRONOME_ARCH_PROTOCOL *gMetronome = NULL;
> > > > EFI_TIMER_ARCH_PROTOCOL *gTimer = NULL;
> > > > EFI_BDS_ARCH_PROTOCOL *gBds = NULL;
> > > > @@ -211,6 +217,14 @@ EFI_DECOMPRESS_PROTOCOL
> > gEfiDecompress = {
> > > > DxeMainUefiDecompress
> > > > };
> > > >
> > > > +//
> > > > +// Common event protocol
> > > > +//
> > > > +EDKII_COMMON_EVENT_PROTOCOL gEdkiiCommonEvent = {
> > > > + CommonCreateEventEx,
> > > > + CommonWaitForEvent
> > > > +};
> > > > +
> > > > //
> > > > // For Loading modules at fixed address feature, the
> > > > configuration table is to cache the top address below which to
> > > > load // Runtime code&boot time code @@ -475,6 +489,14 @@
> DxeMain (
> > > > //
> > > > CoreNotifyOnProtocolInstallation ();
> > > >
> > > > + Status = CoreInstallProtocolInterface (
> > > > + &mCommonEventHalde,
> > > > + &gEdkiiCommonEventProtocolGuid,
> > > > + EFI_NATIVE_INTERFACE,
> > > > + &gEdkiiCommonEvent
> > > > + );
> > > > + ASSERT_EFI_ERROR (Status);
> > > > +
> > > > //
> > > > // Produce Firmware Volume Protocols, one for each FV in the
> > > > HOB
> > list.
> > > > //
> > > > diff --git a/MdeModulePkg/Core/Dxe/DxeMain/DxeProtocolNotify.c
> > > > b/MdeModulePkg/Core/Dxe/DxeMain/DxeProtocolNotify.c
> > > > index 29a55d02e6..57d52c250d 100644
> > > > --- a/MdeModulePkg/Core/Dxe/DxeMain/DxeProtocolNotify.c
> > > > +++ b/MdeModulePkg/Core/Dxe/DxeMain/DxeProtocolNotify.c
> > > > @@ -41,6 +41,7 @@ EFI_CORE_PROTOCOL_NOTIFY_ENTRY
> > > mArchProtocols[] = {
> > > > EFI_CORE_PROTOCOL_NOTIFY_ENTRY mOptionalProtocols[] = {
> > > > { &gEfiSecurity2ArchProtocolGuid, (VOID **)&gSecurity2, NULL,
> > > > NULL, FALSE },
> > > > { &gEfiSmmBase2ProtocolGuid, (VOID **)&gSmmBase2,
> > NULL,
> > > > NULL, FALSE },
> > > > + { &gEdkiiCpu2ProtocolGuid, (VOID **)&gCpu2, NULL,
> > NULL,
> > > > FALSE },
> > > > { NULL, (VOID **)NULL, NULL, NULL, FALSE }
> > > > };
> > > >
> > > > diff --git a/MdeModulePkg/Core/Dxe/Event/Event.c
> > > > b/MdeModulePkg/Core/Dxe/Event/Event.c
> > > > index 21db38aaf0..d271d2faf3 100644
> > > > --- a/MdeModulePkg/Core/Dxe/Event/Event.c
> > > > +++ b/MdeModulePkg/Core/Dxe/Event/Event.c
> > > > @@ -14,7 +14,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> > > > /// /// gEfiCurrentTpl - Current Task priority level /// -EFI_TPL
> > > > gEfiCurrentTpl = TPL_APPLICATION;
> > > > +volatile EFI_TPL gEfiCurrentTpl = TPL_APPLICATION;
> > > >
> > > > ///
> > > > /// gEventQueueLock - Protects the event queues
> > > > @@ -29,7 +29,7 @@ LIST_ENTRY gEventQueue[TPL_HIGH_LEVEL + 1];
> > > > ///
> > > > /// gEventPending - A bitmask of the EventQueues that are pending
> > > > ///
> > > > -UINTN gEventPending = 0;
> > > > +volatile UINTN gEventPending = 0;
> > > >
> > > > ///
> > > > /// gEventSignalQueue - A list of events to signal based on
> > > > EventGroup type @@ -658,16 +658,44 @@ CoreWaitForEvent (
> > > > OUT UINTN *UserIndex
> > > > )
> > > > {
> > > > - EFI_STATUS Status;
> > > > - UINTN Index;
> > > > -
> > > > - //
> > > > + //
> > > > // Can only WaitForEvent at TPL_APPLICATION
> > > > //
> > > > if (gEfiCurrentTpl != TPL_APPLICATION) {
> > > > return EFI_UNSUPPORTED;
> > > > }
> > > >
> > > > + return CommonWaitForEvent (NumberOfEvents, UserEvents,
> > > UserIndex);
> > > > +}
> > > > +
> > > > +
> > > > +/**
> > > > + Stops execution until an event is signaled.
> > > > +
> > > > + Support all TPL.
> > > > +
> > > > + @param NumberOfEvents The number of events in the
> > UserEvents
> > > > array
> > > > + @param UserEvents An array of EFI_EVENT
> > > > + @param UserIndex Pointer to the index of the event which
> > > > + satisfied the wait condition
> > > > +
> > > > + @retval EFI_SUCCESS The event indicated by Index was
> > signaled.
> > > > + @retval EFI_INVALID_PARAMETER The event indicated by Index has
> > > > + a
> > > > notification
> > > > + function or Event was not a valid type
> > > > + @retval EFI_UNSUPPORTED The current TPL is not
> > TPL_APPLICATION
> > > > +
> > > > +**/
> > > > +EFI_STATUS
> > > > +EFIAPI
> > > > +CommonWaitForEvent (
> > > > + IN UINTN NumberOfEvents,
> > > > + IN EFI_EVENT *UserEvents,
> > > > + OUT UINTN *UserIndex
> > > > + )
> > > > +{
> > > > + EFI_STATUS Status;
> > > > + UINTN Index;
> > > > +
> > > > if (NumberOfEvents == 0) {
> > > > return EFI_INVALID_PARAMETER;
> > > > }
> > > > @@ -678,7 +706,7 @@ CoreWaitForEvent (
> > > >
> > > > for(;;) {
> > > >
> > > > - for(Index = 0; Index < NumberOfEvents; Index++) {
> > > > + for (Index = 0; Index < NumberOfEvents; Index++) {
> > > >
> > > > Status = CoreCheckEvent (UserEvents[Index]);
> > > >
> > > > @@ -693,14 +721,66 @@ CoreWaitForEvent (
> > > > }
> > > > }
> > > >
> > > > - //
> > > > - // Signal the Idle event
> > > > - //
> > > > - CoreSignalEvent (gIdleLoopEvent);
> > > > + if (gCpu != NULL && gCpu2 != NULL) {
> > > > + //
> > > > + // None of the events checked above are ready/signaled yet,
> > > > + // disable interrupts before checking for all pending events
> > > > + //
> > > > + gCpu->DisableInterrupt (gCpu);
> > > > +
> > > > + if (((UINTN) HighBitSet64 (gEventPending)) > gEfiCurrentTpl) {
> > > > + //
> > > > + // There are pending events, enable interrupts for these
> > > > + events to be
> > > > processed
> > > > + //
> > > > + gCpu->EnableInterrupt (gCpu);
> > > > + } else {
> > > > + //
> > > > + // No events are pending, enable interrupts, sleep the
> > > > + CPU and wait
> > > > for an interrupt
> > > > + //
> > > > + gCpu2->EnableAndWaitForInterrupt (gCpu2);
> > > > + }
> > > > + }
> > > > }
> > > > }
> > > >
> > > >
> > > > +/**
> > > > + Creates an event in a group.
> > > > +
> > > > + Support all TPL.
> > > > +
> > > > + @param Type The type of event to create and its mode
> > and
> > > > + attributes
> > > > + @param NotifyTpl The task priority level of event
> > notifications
> > > > + @param NotifyFunction Pointer to the events notification
> > function
> > > > + @param NotifyContext Pointer to the notification functions
> > context;
> > > > + corresponds to parameter "Context" in the
> > > > + notification function
> > > > + @param EventGroup GUID for EventGroup if NULL act the
> > same
> > > > as
> > > > + gBS->CreateEvent().
> > > > + @param Event Pointer to the newly created event if the
> > call
> > > > + succeeds; undefined otherwise
> > > > +
> > > > + @retval EFI_SUCCESS The event structure was created
> > > > + @retval EFI_INVALID_PARAMETER One of the parameters has an
> > invalid
> > > > value
> > > > + @retval EFI_OUT_OF_RESOURCES The event could not be allocated
> > > > +
> > > > +**/
> > > > +EFI_STATUS
> > > > +EFIAPI
> > > > +CommonCreateEventEx (
> > > > + IN UINT32 Type,
> > > > + IN EFI_TPL NotifyTpl,
> > > > + IN EFI_EVENT_NOTIFY NotifyFunction, OPTIONAL
> > > > + IN CONST VOID *NotifyContext, OPTIONAL
> > > > + IN CONST EFI_GUID *EventGroup, OPTIONAL
> > > > + OUT EFI_EVENT *Event
> > > > + )
> > > > +{
> > > > + return CoreCreateEventInternal (Type, NotifyTpl,
> > > > +NotifyFunction,
> > > > NotifyContext, EventGroup, Event);
> > > > +}
> > > > +
> > > > +
> > > > /**
> > > > Closes an event and frees the event structure.
> > > >
> > > > diff --git a/MdeModulePkg/Core/Dxe/Event/Event.h
> > > > b/MdeModulePkg/Core/Dxe/Event/Event.h
> > > > index 8141c5003e..050df26ec9 100644
> > > > --- a/MdeModulePkg/Core/Dxe/Event/Event.h
> > > > +++ b/MdeModulePkg/Core/Dxe/Event/Event.h
> > > > @@ -12,7 +12,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> > > >
> > > >
> > > > #define VALID_TPL(a) ((a) <= TPL_HIGH_LEVEL)
> > > > -extern UINTN gEventPending;
> > > > +extern volatile UINTN gEventPending;
> > > >
> > > > ///
> > > > /// Set if Event is part of an event group diff --git
> > > > a/MdeModulePkg/Include/Protocol/Cpu2.h
> > > > b/MdeModulePkg/Include/Protocol/Cpu2.h
> > > > index 14464a38c8..81c5c8e424 100644
> > > > --- a/MdeModulePkg/Include/Protocol/Cpu2.h
> > > > +++ b/MdeModulePkg/Include/Protocol/Cpu2.h
> > > > @@ -37,7 +37,7 @@ EFI_STATUS
> > > > /// Foundation.
> > > > ///
> > > > struct _EDKII_CPU2_PROTOCOL {
> > > > - EDKII_CPU_ENABLE_AND_WAIT_FOR_INTERRUPT
> > > > EnableAndForWaitInterrupt;
> > > > + EDKII_CPU_ENABLE_AND_WAIT_FOR_INTERRUPT
> > > > EnableAndWaitForInterrupt;
> > > > };
> > > >
> > >
> > > I think this change should belong to patch 1.
> >
> > Yes. You are right. I would move the change to 1/4.
> >
> > Thanks,
> > Zhichao
> >
> > >
> > > Regards,
> > > Jian
> > >
> > > > #endif
> > > > --
> > > > 2.21.0.windows.1
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V2 4/4] MdeModulePkg/DxeMain: Implement common event protocol
2019-07-11 0:20 ` Gao, Zhichao
@ 2019-07-11 1:26 ` Wang, Jian J
0 siblings, 0 replies; 15+ messages in thread
From: Wang, Jian J @ 2019-07-11 1:26 UTC (permalink / raw)
To: Gao, Zhichao, devel@edk2.groups.io
Cc: Sean Brogan, Wu, Hao A, Ni, Ray, Zeng, Star, Gao, Liming,
Michael Turner, Bret Barkelew
Zhichao,
Understood now. Thanks for the information.
Regards,
Jian
> -----Original Message-----
> From: Gao, Zhichao
> Sent: Thursday, July 11, 2019 8:21 AM
> To: Wang, Jian J <jian.j.wang@intel.com>; devel@edk2.groups.io
> Cc: Sean Brogan <sean.brogan@microsoft.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>; Michael
> Turner <Michael.Turner@microsoft.com>; Bret Barkelew
> <Bret.Barkelew@microsoft.com>
> Subject: RE: [PATCH V2 4/4] MdeModulePkg/DxeMain: Implement common
> event protocol
>
>
>
> > -----Original Message-----
> > From: Wang, Jian J
> > Sent: Wednesday, July 10, 2019 4:47 PM
> > To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
> > Cc: Sean Brogan <sean.brogan@microsoft.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>; Michael
> Turner
> > <Michael.Turner@microsoft.com>; Bret Barkelew
> > <Bret.Barkelew@microsoft.com>
> > Subject: RE: [PATCH V2 4/4] MdeModulePkg/DxeMain: Implement
> common
> > event protocol
> >
> > Zhichao,
> >
> > > -----Original Message-----
> > > From: Gao, Zhichao
> > > Sent: Wednesday, July 10, 2019 8:24 AM
> > > To: Wang, Jian J <jian.j.wang@intel.com>; devel@edk2.groups.io
> > > Cc: Sean Brogan <sean.brogan@microsoft.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>; Michael
> > > Turner <Michael.Turner@microsoft.com>; Bret Barkelew
> > > <Bret.Barkelew@microsoft.com>
> > > Subject: RE: [PATCH V2 4/4] MdeModulePkg/DxeMain: Implement
> > common
> > > event protocol
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Wang, Jian J
> > > > Sent: Tuesday, July 9, 2019 5:24 PM
> > > > To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
> > > > Cc: Sean Brogan <sean.brogan@microsoft.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>; Michael
> > > Turner
> > > > <Michael.Turner@microsoft.com>; Bret Barkelew
> > > > <Bret.Barkelew@microsoft.com>
> > > > Subject: RE: [PATCH V2 4/4] MdeModulePkg/DxeMain: Implement
> > > common
> > > > event protocol
> > > >
> > > > Hi Zhichao,
> > > >
> > > > One common comment: please update copy right year.
> > >
> > > I got the copyright remind before. Refer to
> > > https://edk2.groups.io/g/devel/topic/31828852#41576.
> > > This patch is almost provided from MU project and contributed by
> > > Microsoft developers, 'From:...' is the author. And they didn't add
> > > their copyright. So I think it is fine to keep the copyright.
> > >
> >
> > I mean the *year* in Intel copyright not the whole copyright.
>
> https://edk2.groups.io/g/devel/message/41599
> https://edk2.groups.io/g/devel/message/41753
> See above message. Maybe updating the year means you are making a legal
> claim to the intellectual
> property provided by the patch on behalf of the company.
>
> Thanks,
> Zhichao
>
> >
> > Regards,
> > Jian
> >
> > > >
> > > > See another comment at the almost the end of the email.
> > > >
> > > > > -----Original Message-----
> > > > > From: Gao, Zhichao
> > > > > Sent: Tuesday, July 09, 2019 4:40 PM
> > > > > To: devel@edk2.groups.io
> > > > > Cc: Sean Brogan <sean.brogan@microsoft.com>; 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>; Michael Turner
> > > > <Michael.Turner@microsoft.com>;
> > > > > Bret Barkelew <Bret.Barkelew@microsoft.com>
> > > > > Subject: [PATCH V2 4/4] MdeModulePkg/DxeMain: Implement
> > common
> > > > event
> > > > > protocol
> > > > >
> > > > > From: Sean Brogan <sean.brogan@microsoft.com>
> > > > >
> > > > > REF: 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. So it is required
> > > > > to check the gEventPending with the interrupt disabled.
> > > > >
> > > > > Implement a protocol gEdkiiCommonEventProtocolGuid to support
> all
> > > TPL
> > > > > event for PI drivers that use HW interrput. It has two interface,
> > > > > one is to create event and the other one is to wait for event.
> > > > >
> > > > > Add 'volatile' specifier for gEfiCurrentTpl and gEventPending
> > > > > because they may be changed out of the context (interrupt context).
> > > > >
> > > > > 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>
> > > > > Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> > > > > ---
> > > > > MdeModulePkg/Core/Dxe/DxeMain.h | 64 ++++++++++-
> > > > > MdeModulePkg/Core/Dxe/DxeMain.inf | 2 +
> > > > > 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/Cpu2.h | 2 +-
> > > > > 7 files changed, 181 insertions(+), 14 deletions(-)
> > > > >
> > > > > diff --git a/MdeModulePkg/Core/Dxe/DxeMain.h
> > > > > b/MdeModulePkg/Core/Dxe/DxeMain.h index
> > > 6a64852730..38907bfe8d
> > > > 100644
> > > > > --- a/MdeModulePkg/Core/Dxe/DxeMain.h
> > > > > +++ b/MdeModulePkg/Core/Dxe/DxeMain.h
> > > > > @@ -38,6 +38,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> > > > > #include <Protocol/Security2.h> #include <Protocol/Reset.h>
> > > > > #include <Protocol/Cpu.h>
> > > > > +#include <Protocol/Cpu2.h>
> > > > > #include <Protocol/Metronome.h>
> > > > > #include <Protocol/FirmwareVolumeBlock.h> #include
> > > > > <Protocol/Capsule.h> @@ -47,6 +48,7 @@ SPDX-License-Identifier:
> > > > > BSD-2-Clause-Patent #include <Protocol/HiiPackageList.h>
> > > > > #include <Protocol/SmmBase2.h> #include
> > > > > <Protocol/PeCoffImageEmulator.h>
> > > > > +#include <Protocol/CommonEvent.h>
> > > > > #include <Guid/MemoryTypeInformation.h> #include
> > > > > <Guid/FirmwareFileSystem2.h> #include
> > > <Guid/FirmwareFileSystem3.h>
> > > > @@
> > > > > -274,10 +276,11 @@ extern EFI_METRONOME_ARCH_PROTOCOL
> > > > *gMetronome;
> > > > > extern EFI_TIMER_ARCH_PROTOCOL *gTimer;
> > > > > extern EFI_SECURITY_ARCH_PROTOCOL *gSecurity;
> > > > > extern EFI_SECURITY2_ARCH_PROTOCOL *gSecurity2;
> > > > > +extern EDKII_CPU2_PROTOCOL *gCpu2;
> > > > > extern EFI_BDS_ARCH_PROTOCOL *gBds;
> > > > > extern EFI_SMM_BASE2_PROTOCOL *gSmmBase2;
> > > > >
> > > > > -extern EFI_TPL gEfiCurrentTpl;
> > > > > +extern volatile EFI_TPL gEfiCurrentTpl;
> > > > >
> > > > > extern EFI_GUID *gDxeCoreFileName;
> > > > > extern EFI_LOADED_IMAGE_PROTOCOL
> > > *gDxeCoreLoadedImage;
> > > > > @@ -1714,6 +1717,65 @@ CoreCheckEvent (
> > > > > );
> > > > >
> > > > >
> > > > > +/**
> > > > > + Stops execution until an event is signaled.
> > > > > +
> > > > > + Support all TPL.
> > > > > +
> > > > > + @param NumberOfEvents The number of events in the
> > > UserEvents
> > > > > array
> > > > > + @param UserEvents An array of EFI_EVENT
> > > > > + @param UserIndex Pointer to the index of the event
> which
> > > > > + satisfied the wait condition
> > > > > +
> > > > > + @retval EFI_SUCCESS The event indicated by Index was
> > > signaled.
> > > > > + @retval EFI_INVALID_PARAMETER The event indicated by Index
> has
> > > > > + a
> > > > > notification
> > > > > + function or Event was not a valid type
> > > > > + @retval EFI_UNSUPPORTED The current TPL is not
> > > TPL_APPLICATION
> > > > > +
> > > > > +**/
> > > > > +EFI_STATUS
> > > > > +EFIAPI
> > > > > +CommonWaitForEvent (
> > > > > + IN UINTN NumberOfEvents,
> > > > > + IN EFI_EVENT *UserEvents,
> > > > > + OUT UINTN *UserIndex
> > > > > + );
> > > > > +
> > > > > +
> > > > > +/**
> > > > > + Creates an event in a group.
> > > > > +
> > > > > + Support all TPL.
> > > > > +
> > > > > + @param Type The type of event to create and its mode
> > > and
> > > > > + attributes
> > > > > + @param NotifyTpl The task priority level of event
> > > notifications
> > > > > + @param NotifyFunction Pointer to the events notification
> > > function
> > > > > + @param NotifyContext Pointer to the notification functions
> > > context;
> > > > > + corresponds to parameter "Context" in the
> > > > > + notification function
> > > > > + @param EventGroup GUID for EventGroup if NULL act the
> > > same
> > > > > as
> > > > > + gBS->CreateEvent().
> > > > > + @param Event Pointer to the newly created event if the
> > > call
> > > > > + succeeds; undefined otherwise
> > > > > +
> > > > > + @retval EFI_SUCCESS The event structure was created
> > > > > + @retval EFI_INVALID_PARAMETER One of the parameters has an
> > > invalid
> > > > > value
> > > > > + @retval EFI_OUT_OF_RESOURCES The event could not be
> allocated
> > > > > +
> > > > > +**/
> > > > > +EFI_STATUS
> > > > > +EFIAPI
> > > > > +CommonCreateEventEx (
> > > > > + IN UINT32 Type,
> > > > > + IN EFI_TPL NotifyTpl,
> > > > > + IN EFI_EVENT_NOTIFY NotifyFunction, OPTIONAL
> > > > > + IN CONST VOID *NotifyContext, OPTIONAL
> > > > > + IN CONST EFI_GUID *EventGroup, OPTIONAL
> > > > > + OUT EFI_EVENT *Event
> > > > > + );
> > > > > +
> > > > > +
> > > > > /**
> > > > > Adds reserved memory, system memory, or memory-mapped I/O
> > > > resources
> > > > > to the
> > > > > global coherency domain of the processor.
> > > > > diff --git a/MdeModulePkg/Core/Dxe/DxeMain.inf
> > > > > b/MdeModulePkg/Core/Dxe/DxeMain.inf
> > > > > index 61161bee28..44f90bcf4b 100644
> > > > > --- a/MdeModulePkg/Core/Dxe/DxeMain.inf
> > > > > +++ b/MdeModulePkg/Core/Dxe/DxeMain.inf
> > > > > @@ -171,6 +171,8 @@
> > > > > gEfiVariableArchProtocolGuid ## CONSUMES
> > > > > gEfiCapsuleArchProtocolGuid ## CONSUMES
> > > > > gEfiWatchdogTimerArchProtocolGuid ## CONSUMES
> > > > > + gEdkiiCommonEventProtocolGuid ##
> > > SOMETIMES_CONSUMES
> > > > > + gEdkiiCpu2ProtocolGuid ## CONSUMES
> > > > >
> > > > > [Pcd]
> > > > >
> > > > >
> > > >
> > >
> >
> gEfiMdeModulePkgTokenSpaceGuid.PcdLoadFixAddressBootTimeCodePage
> > > > > Number ## SOMETIMES_CONSUMES
> > > > > diff --git a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
> > > > > b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
> > > > > index 514d1aa75a..8196d96daa 100644
> > > > > --- a/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
> > > > > +++ b/MdeModulePkg/Core/Dxe/DxeMain/DxeMain.c
> > > > > @@ -13,12 +13,18 @@ SPDX-License-Identifier: BSD-2-Clause-
> Patent //
> > > > > EFI_HANDLE mDecompressHandle = NULL;
> > > > >
> > > > > +//
> > > > > +// Common event protocol
> > > > > +//
> > > > > +EFI_HANDLE mCommonEventHalde = NULL;
> > > > > +
> > > > > //
> > > > > // DXE Core globals for Architecture Protocols //
> > > > > EFI_SECURITY_ARCH_PROTOCOL *gSecurity = NULL;
> > > > > EFI_SECURITY2_ARCH_PROTOCOL *gSecurity2 = NULL;
> > > > > EFI_CPU_ARCH_PROTOCOL *gCpu = NULL;
> > > > > +EDKII_CPU2_PROTOCOL *gCpu2 = NULL;
> > > > > EFI_METRONOME_ARCH_PROTOCOL *gMetronome = NULL;
> > > > > EFI_TIMER_ARCH_PROTOCOL *gTimer = NULL;
> > > > > EFI_BDS_ARCH_PROTOCOL *gBds = NULL;
> > > > > @@ -211,6 +217,14 @@ EFI_DECOMPRESS_PROTOCOL
> > > gEfiDecompress = {
> > > > > DxeMainUefiDecompress
> > > > > };
> > > > >
> > > > > +//
> > > > > +// Common event protocol
> > > > > +//
> > > > > +EDKII_COMMON_EVENT_PROTOCOL gEdkiiCommonEvent = {
> > > > > + CommonCreateEventEx,
> > > > > + CommonWaitForEvent
> > > > > +};
> > > > > +
> > > > > //
> > > > > // For Loading modules at fixed address feature, the
> > > > > configuration table is to cache the top address below which to
> > > > > load // Runtime code&boot time code @@ -475,6 +489,14 @@
> > DxeMain (
> > > > > //
> > > > > CoreNotifyOnProtocolInstallation ();
> > > > >
> > > > > + Status = CoreInstallProtocolInterface (
> > > > > + &mCommonEventHalde,
> > > > > + &gEdkiiCommonEventProtocolGuid,
> > > > > + EFI_NATIVE_INTERFACE,
> > > > > + &gEdkiiCommonEvent
> > > > > + );
> > > > > + ASSERT_EFI_ERROR (Status);
> > > > > +
> > > > > //
> > > > > // Produce Firmware Volume Protocols, one for each FV in the
> > > > > HOB
> > > list.
> > > > > //
> > > > > diff --git a/MdeModulePkg/Core/Dxe/DxeMain/DxeProtocolNotify.c
> > > > > b/MdeModulePkg/Core/Dxe/DxeMain/DxeProtocolNotify.c
> > > > > index 29a55d02e6..57d52c250d 100644
> > > > > --- a/MdeModulePkg/Core/Dxe/DxeMain/DxeProtocolNotify.c
> > > > > +++ b/MdeModulePkg/Core/Dxe/DxeMain/DxeProtocolNotify.c
> > > > > @@ -41,6 +41,7 @@ EFI_CORE_PROTOCOL_NOTIFY_ENTRY
> > > > mArchProtocols[] = {
> > > > > EFI_CORE_PROTOCOL_NOTIFY_ENTRY mOptionalProtocols[] = {
> > > > > { &gEfiSecurity2ArchProtocolGuid, (VOID **)&gSecurity2,
> NULL,
> > > > > NULL, FALSE },
> > > > > { &gEfiSmmBase2ProtocolGuid, (VOID **)&gSmmBase2,
> > > NULL,
> > > > > NULL, FALSE },
> > > > > + { &gEdkiiCpu2ProtocolGuid, (VOID **)&gCpu2, NULL,
> > > NULL,
> > > > > FALSE },
> > > > > { NULL, (VOID **)NULL, NULL, NULL,
> FALSE }
> > > > > };
> > > > >
> > > > > diff --git a/MdeModulePkg/Core/Dxe/Event/Event.c
> > > > > b/MdeModulePkg/Core/Dxe/Event/Event.c
> > > > > index 21db38aaf0..d271d2faf3 100644
> > > > > --- a/MdeModulePkg/Core/Dxe/Event/Event.c
> > > > > +++ b/MdeModulePkg/Core/Dxe/Event/Event.c
> > > > > @@ -14,7 +14,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> > > > > /// /// gEfiCurrentTpl - Current Task priority level /// -EFI_TPL
> > > > > gEfiCurrentTpl = TPL_APPLICATION;
> > > > > +volatile EFI_TPL gEfiCurrentTpl = TPL_APPLICATION;
> > > > >
> > > > > ///
> > > > > /// gEventQueueLock - Protects the event queues
> > > > > @@ -29,7 +29,7 @@ LIST_ENTRY gEventQueue[TPL_HIGH_LEVEL
> + 1];
> > > > > ///
> > > > > /// gEventPending - A bitmask of the EventQueues that are pending
> > > > > ///
> > > > > -UINTN gEventPending = 0;
> > > > > +volatile UINTN gEventPending = 0;
> > > > >
> > > > > ///
> > > > > /// gEventSignalQueue - A list of events to signal based on
> > > > > EventGroup type @@ -658,16 +658,44 @@ CoreWaitForEvent (
> > > > > OUT UINTN *UserIndex
> > > > > )
> > > > > {
> > > > > - EFI_STATUS Status;
> > > > > - UINTN Index;
> > > > > -
> > > > > - //
> > > > > + //
> > > > > // Can only WaitForEvent at TPL_APPLICATION
> > > > > //
> > > > > if (gEfiCurrentTpl != TPL_APPLICATION) {
> > > > > return EFI_UNSUPPORTED;
> > > > > }
> > > > >
> > > > > + return CommonWaitForEvent (NumberOfEvents, UserEvents,
> > > > UserIndex);
> > > > > +}
> > > > > +
> > > > > +
> > > > > +/**
> > > > > + Stops execution until an event is signaled.
> > > > > +
> > > > > + Support all TPL.
> > > > > +
> > > > > + @param NumberOfEvents The number of events in the
> > > UserEvents
> > > > > array
> > > > > + @param UserEvents An array of EFI_EVENT
> > > > > + @param UserIndex Pointer to the index of the event
> which
> > > > > + satisfied the wait condition
> > > > > +
> > > > > + @retval EFI_SUCCESS The event indicated by Index was
> > > signaled.
> > > > > + @retval EFI_INVALID_PARAMETER The event indicated by Index
> has
> > > > > + a
> > > > > notification
> > > > > + function or Event was not a valid type
> > > > > + @retval EFI_UNSUPPORTED The current TPL is not
> > > TPL_APPLICATION
> > > > > +
> > > > > +**/
> > > > > +EFI_STATUS
> > > > > +EFIAPI
> > > > > +CommonWaitForEvent (
> > > > > + IN UINTN NumberOfEvents,
> > > > > + IN EFI_EVENT *UserEvents,
> > > > > + OUT UINTN *UserIndex
> > > > > + )
> > > > > +{
> > > > > + EFI_STATUS Status;
> > > > > + UINTN Index;
> > > > > +
> > > > > if (NumberOfEvents == 0) {
> > > > > return EFI_INVALID_PARAMETER;
> > > > > }
> > > > > @@ -678,7 +706,7 @@ CoreWaitForEvent (
> > > > >
> > > > > for(;;) {
> > > > >
> > > > > - for(Index = 0; Index < NumberOfEvents; Index++) {
> > > > > + for (Index = 0; Index < NumberOfEvents; Index++) {
> > > > >
> > > > > Status = CoreCheckEvent (UserEvents[Index]);
> > > > >
> > > > > @@ -693,14 +721,66 @@ CoreWaitForEvent (
> > > > > }
> > > > > }
> > > > >
> > > > > - //
> > > > > - // Signal the Idle event
> > > > > - //
> > > > > - CoreSignalEvent (gIdleLoopEvent);
> > > > > + if (gCpu != NULL && gCpu2 != NULL) {
> > > > > + //
> > > > > + // None of the events checked above are ready/signaled yet,
> > > > > + // disable interrupts before checking for all pending events
> > > > > + //
> > > > > + gCpu->DisableInterrupt (gCpu);
> > > > > +
> > > > > + if (((UINTN) HighBitSet64 (gEventPending)) > gEfiCurrentTpl) {
> > > > > + //
> > > > > + // There are pending events, enable interrupts for these
> > > > > + events to be
> > > > > processed
> > > > > + //
> > > > > + gCpu->EnableInterrupt (gCpu);
> > > > > + } else {
> > > > > + //
> > > > > + // No events are pending, enable interrupts, sleep the
> > > > > + CPU and wait
> > > > > for an interrupt
> > > > > + //
> > > > > + gCpu2->EnableAndWaitForInterrupt (gCpu2);
> > > > > + }
> > > > > + }
> > > > > }
> > > > > }
> > > > >
> > > > >
> > > > > +/**
> > > > > + Creates an event in a group.
> > > > > +
> > > > > + Support all TPL.
> > > > > +
> > > > > + @param Type The type of event to create and its mode
> > > and
> > > > > + attributes
> > > > > + @param NotifyTpl The task priority level of event
> > > notifications
> > > > > + @param NotifyFunction Pointer to the events notification
> > > function
> > > > > + @param NotifyContext Pointer to the notification functions
> > > context;
> > > > > + corresponds to parameter "Context" in the
> > > > > + notification function
> > > > > + @param EventGroup GUID for EventGroup if NULL act the
> > > same
> > > > > as
> > > > > + gBS->CreateEvent().
> > > > > + @param Event Pointer to the newly created event if the
> > > call
> > > > > + succeeds; undefined otherwise
> > > > > +
> > > > > + @retval EFI_SUCCESS The event structure was created
> > > > > + @retval EFI_INVALID_PARAMETER One of the parameters has an
> > > invalid
> > > > > value
> > > > > + @retval EFI_OUT_OF_RESOURCES The event could not be
> allocated
> > > > > +
> > > > > +**/
> > > > > +EFI_STATUS
> > > > > +EFIAPI
> > > > > +CommonCreateEventEx (
> > > > > + IN UINT32 Type,
> > > > > + IN EFI_TPL NotifyTpl,
> > > > > + IN EFI_EVENT_NOTIFY NotifyFunction, OPTIONAL
> > > > > + IN CONST VOID *NotifyContext, OPTIONAL
> > > > > + IN CONST EFI_GUID *EventGroup, OPTIONAL
> > > > > + OUT EFI_EVENT *Event
> > > > > + )
> > > > > +{
> > > > > + return CoreCreateEventInternal (Type, NotifyTpl,
> > > > > +NotifyFunction,
> > > > > NotifyContext, EventGroup, Event);
> > > > > +}
> > > > > +
> > > > > +
> > > > > /**
> > > > > Closes an event and frees the event structure.
> > > > >
> > > > > diff --git a/MdeModulePkg/Core/Dxe/Event/Event.h
> > > > > b/MdeModulePkg/Core/Dxe/Event/Event.h
> > > > > index 8141c5003e..050df26ec9 100644
> > > > > --- a/MdeModulePkg/Core/Dxe/Event/Event.h
> > > > > +++ b/MdeModulePkg/Core/Dxe/Event/Event.h
> > > > > @@ -12,7 +12,7 @@ SPDX-License-Identifier: BSD-2-Clause-Patent
> > > > >
> > > > >
> > > > > #define VALID_TPL(a) ((a) <= TPL_HIGH_LEVEL)
> > > > > -extern UINTN gEventPending;
> > > > > +extern volatile UINTN gEventPending;
> > > > >
> > > > > ///
> > > > > /// Set if Event is part of an event group diff --git
> > > > > a/MdeModulePkg/Include/Protocol/Cpu2.h
> > > > > b/MdeModulePkg/Include/Protocol/Cpu2.h
> > > > > index 14464a38c8..81c5c8e424 100644
> > > > > --- a/MdeModulePkg/Include/Protocol/Cpu2.h
> > > > > +++ b/MdeModulePkg/Include/Protocol/Cpu2.h
> > > > > @@ -37,7 +37,7 @@ EFI_STATUS
> > > > > /// Foundation.
> > > > > ///
> > > > > struct _EDKII_CPU2_PROTOCOL {
> > > > > - EDKII_CPU_ENABLE_AND_WAIT_FOR_INTERRUPT
> > > > > EnableAndForWaitInterrupt;
> > > > > + EDKII_CPU_ENABLE_AND_WAIT_FOR_INTERRUPT
> > > > > EnableAndWaitForInterrupt;
> > > > > };
> > > > >
> > > >
> > > > I think this change should belong to patch 1.
> > >
> > > Yes. You are right. I would move the change to 1/4.
> > >
> > > Thanks,
> > > Zhichao
> > >
> > > >
> > > > Regards,
> > > > Jian
> > > >
> > > > > #endif
> > > > > --
> > > > > 2.21.0.windows.1
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V2 2/4] UefiCpuPkg/CpuDxe: Implement Cpu2 protocol
2019-07-09 8:39 ` [PATCH V2 2/4] UefiCpuPkg/CpuDxe: Implement Cpu2 protocol Gao, Zhichao
@ 2019-07-11 2:22 ` Dong, Eric
2019-07-11 2:36 ` Gao, Zhichao
0 siblings, 1 reply; 15+ messages in thread
From: Dong, Eric @ 2019-07-11 2:22 UTC (permalink / raw)
To: Gao, Zhichao, devel@edk2.groups.io
Cc: Sean Brogan, Ni, Ray, Laszlo Ersek, Gao, Liming, Michael Turner,
Bret Barkelew
Hi Zhizhao,
The new add files don't have copyright info, is it ok?
Thanks,
Eric
> -----Original Message-----
> From: Gao, Zhichao
> Sent: Tuesday, July 9, 2019 4:40 PM
> To: devel@edk2.groups.io
> Cc: Sean Brogan <sean.brogan@microsoft.com>; Dong, Eric
> <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Laszlo Ersek
> <lersek@redhat.com>; Gao, Liming <liming.gao@intel.com>; Michael Turner
> <Michael.Turner@microsoft.com>; Bret Barkelew
> <Bret.Barkelew@microsoft.com>
> Subject: [PATCH V2 2/4] UefiCpuPkg/CpuDxe: Implement Cpu2 protocol
>
> From: Sean Brogan <sean.brogan@microsoft.com>
>
> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1400
>
> Implement Cp2 protocol: it has one interface to enable the interrupt and put
> cpu to sleep and wait for an interrupt.
>
> Cc: Eric Dong <eric.dong@intel.com>
> Cc: Ray Ni <ray.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.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>
> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> ---
> UefiCpuPkg/CpuDxe/CpuDxe.c | 38 +++++++++++++++++++
> UefiCpuPkg/CpuDxe/CpuDxe.h | 25 ++++++++++++
> UefiCpuPkg/CpuDxe/CpuDxe.inf | 3 ++
> .../CpuDxe/Ia32/EnableInterruptsAndSleep.c | 24 ++++++++++++
> .../CpuDxe/X64/EnableInterruptsAndSleep.nasm | 31 +++++++++++++++
> 5 files changed, 121 insertions(+)
> create mode 100644 UefiCpuPkg/CpuDxe/Ia32/EnableInterruptsAndSleep.c
> create mode 100644
> UefiCpuPkg/CpuDxe/X64/EnableInterruptsAndSleep.nasm
>
> diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.c b/UefiCpuPkg/CpuDxe/CpuDxe.c
> index 7d7270e10b..2eeffcf426 100644
> --- a/UefiCpuPkg/CpuDxe/CpuDxe.c
> +++ b/UefiCpuPkg/CpuDxe/CpuDxe.c
> @@ -18,6 +18,7 @@
> //
> BOOLEAN InterruptState = FALSE;
> EFI_HANDLE mCpuHandle = NULL;
> +EFI_HANDLE mCpu2Handle = NULL;
> BOOLEAN mIsFlushingGCD;
> BOOLEAN mIsAllocatingPageTable = FALSE;
> UINT64 mValidMtrrAddressMask;
> @@ -96,6 +97,10 @@ EFI_CPU_ARCH_PROTOCOL gCpu = {
> 4 // DmaBufferAlignment
> };
>
> +EDKII_CPU2_PROTOCOL gCpu2 = {
> + CpuEnableAndWaitForInterrupt
> +};
> +
> //
> // CPU Arch Protocol Functions
> //
> @@ -499,6 +504,28 @@ CpuSetMemoryAttributes (
> return AssignMemoryPageAttributes (NULL, BaseAddress, Length,
> MemoryAttributes, NULL); }
>
> +//
> +// CPU2 Protocol Functions
> +//
> +/**
> + This function enables CPU interrupts and then waits for an interrupt to
> arrive.
> +
> + @param This The EFI_CPU2_PROTOCOL instance.
> +
> + @retval EFI_SUCCESS Interrupts are enabled on the processor.
> + @retval EFI_DEVICE_ERROR Interrupts could not be enabled on the
> processor.
> +
> +**/
> +EFI_STATUS
> +CpuEnableAndWaitForInterrupt (
> + IN EDKII_CPU2_PROTOCOL *This
> + )
> +{
> + EnableInterruptsAndSleep ();
> +
> + return EFI_SUCCESS;
> +}
> +
> /**
> Initializes the valid bits mask and valid address mask for MTRRs.
>
> @@ -1211,6 +1238,17 @@ InitializeCpu (
> );
> ASSERT_EFI_ERROR (Status);
>
> + //
> + // Install CPU2 Protocol
> + //
> + Status = gBS->InstallMultipleProtocolInterfaces (
> + &mCpu2Handle,
> + &gEdkiiCpu2ProtocolGuid,
> + &gCpu2,
> + NULL
> + );
> + ASSERT_EFI_ERROR (Status);
> +
> InitializeMpSupport ();
>
> return Status;
> diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.h b/UefiCpuPkg/CpuDxe/CpuDxe.h
> index b029be430b..c1398830ba 100644
> --- a/UefiCpuPkg/CpuDxe/CpuDxe.h
> +++ b/UefiCpuPkg/CpuDxe/CpuDxe.h
> @@ -12,6 +12,7 @@
> #include <PiDxe.h>
>
> #include <Protocol/Cpu.h>
> +#include <Protocol/Cpu2.h>
> #include <Protocol/MpService.h>
> #include <Register/Msr.h>
>
> @@ -305,6 +306,30 @@ PageFaultExceptionHandler (
> IN EFI_SYSTEM_CONTEXT SystemContext
> );
>
> +/**
> + This function enables CPU interrupts and then waits for an interrupt to
> arrive.
> +
> + @param This The EFI_CPU2_PROTOCOL instance.
> +
> + @retval EFI_SUCCESS Interrupts are enabled on the processor.
> + @retval EFI_DEVICE_ERROR Interrupts could not be enabled on the
> processor.
> +
> +**/
> +EFI_STATUS
> +CpuEnableAndWaitForInterrupt (
> + IN EDKII_CPU2_PROTOCOL *This
> + );
> +
> +/**
> + Enables CPU interrupts and then waits for an interrupt to arrive.
> +
> +**/
> +VOID
> +EFIAPI
> +EnableInterruptsAndSleep (
> + VOID
> + );
> +
> extern BOOLEAN mIsAllocatingPageTable;
> extern UINTN mNumberOfProcessors;
>
> diff --git a/UefiCpuPkg/CpuDxe/CpuDxe.inf
> b/UefiCpuPkg/CpuDxe/CpuDxe.inf index 57381dbc85..334ddb142f 100644
> --- a/UefiCpuPkg/CpuDxe/CpuDxe.inf
> +++ b/UefiCpuPkg/CpuDxe/CpuDxe.inf
> @@ -54,14 +54,17 @@
>
> [Sources.IA32]
> Ia32/CpuAsm.nasm
> + Ia32/EnableInterruptsAndSleep.c
>
> [Sources.X64]
> X64/CpuAsm.nasm
> + X64/EnableInterruptsAndSleep.nasm
>
> [Protocols]
> gEfiCpuArchProtocolGuid ## PRODUCES
> gEfiMpServiceProtocolGuid ## PRODUCES
> gEfiSmmBase2ProtocolGuid ## SOMETIMES_CONSUMES
> + gEdkiiCpu2ProtocolGuid ## PRODUCES
>
> [Guids]
> gIdleLoopEventGuid ## CONSUMES ## Event
> diff --git a/UefiCpuPkg/CpuDxe/Ia32/EnableInterruptsAndSleep.c
> b/UefiCpuPkg/CpuDxe/Ia32/EnableInterruptsAndSleep.c
> new file mode 100644
> index 0000000000..dda76139ab
> --- /dev/null
> +++ b/UefiCpuPkg/CpuDxe/Ia32/EnableInterruptsAndSleep.c
> @@ -0,0 +1,24 @@
> +/** @file
> + EnableInterruptsAndSleep function
> +
> + SPDX-License-Identifier: BSD-2-Clause-Patent
> +
> +**/
> +
> +
> +
> +/**
> + Enables CPU interrupts and then sleep.
> +
> +**/
> +VOID
> +EFIAPI
> +EnableInterruptsAndSleep (
> + VOID
> + )
> +{
> + _asm {
> + sti
> + hlt
> + }
> +}
> diff --git a/UefiCpuPkg/CpuDxe/X64/EnableInterruptsAndSleep.nasm
> b/UefiCpuPkg/CpuDxe/X64/EnableInterruptsAndSleep.nasm
> new file mode 100644
> index 0000000000..2d93ecb4bb
> --- /dev/null
> +++ b/UefiCpuPkg/CpuDxe/X64/EnableInterruptsAndSleep.nasm
> @@ -0,0 +1,31 @@
> +;----------------------------------------------------------------------
> +--------
> +;
> +; SPDX-License-Identifier: BSD-2-Clause-Patent ; ; Module Name:
> +;
> +; EnableInterruptsAndSleep.nasm
> +;
> +; Abstract:
> +;
> +; EnableInterruptsAndSleep function
> +;
> +; Notes:
> +;
> +;----------------------------------------------------------------------
> +--------
> +
> + DEFAULT REL
> + SECTION .text
> +
> +;----------------------------------------------------------------------
> +--------
> +; VOID
> +; EFIAPI
> +; EnableInterruptsAndSleep (
> +; VOID
> +; );
> +;----------------------------------------------------------------------
> +-------- global ASM_PFX(EnableInterruptsAndSleep)
> +ASM_PFX(EnableInterruptsAndSleep):
> + sti
> + hlt
> + ret
> --
> 2.21.0.windows.1
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V2 2/4] UefiCpuPkg/CpuDxe: Implement Cpu2 protocol
2019-07-11 2:22 ` Dong, Eric
@ 2019-07-11 2:36 ` Gao, Zhichao
2019-07-11 10:24 ` Laszlo Ersek
0 siblings, 1 reply; 15+ messages in thread
From: Gao, Zhichao @ 2019-07-11 2:36 UTC (permalink / raw)
To: Dong, Eric, devel@edk2.groups.io
Cc: Sean Brogan, Ni, Ray, Laszlo Ersek, Gao, Liming, Michael Turner,
Bret Barkelew
> -----Original Message-----
> From: Dong, Eric
> Sent: Thursday, July 11, 2019 10:22 AM
> To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
> Cc: Sean Brogan <sean.brogan@microsoft.com>; Ni, Ray <ray.ni@intel.com>;
> Laszlo Ersek <lersek@redhat.com>; Gao, Liming <liming.gao@intel.com>;
> Michael Turner <Michael.Turner@microsoft.com>; Bret Barkelew
> <Bret.Barkelew@microsoft.com>
> Subject: RE: [PATCH V2 2/4] UefiCpuPkg/CpuDxe: Implement Cpu2 protocol
>
> Hi Zhizhao,
>
> The new add files don't have copyright info, is it ok?
I forgot add file comment for Ia32/EnableInterruptsAndSleep.c. Also lose the BSD+Patent License. I would add them in the future.
For the copyright:
The code is contributed by Microsoft at https://github.com/Microsoft/mu_basecore/blob/release/201808/MdePkg/Library/BaseLib/X64/EnableInterrupts.nasm#L48
I move the code from MdePkg to UefiCpuPkg to a single file. It is inappropriate to add both intel (not contributor) and Microsoft (they didn't add in the above link).
So I keep the copyright blank. I don't know whether it is ok without copyright.
Hi Liming,
What's your opinion?
Thanks,
Zhichao
>
> Thanks,
> Eric
>
> > -----Original Message-----
> > From: Gao, Zhichao
> > Sent: Tuesday, July 9, 2019 4:40 PM
> > To: devel@edk2.groups.io
> > Cc: Sean Brogan <sean.brogan@microsoft.com>; Dong, Eric
> > <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Laszlo Ersek
> > <lersek@redhat.com>; Gao, Liming <liming.gao@intel.com>; Michael
> > Turner <Michael.Turner@microsoft.com>; Bret Barkelew
> > <Bret.Barkelew@microsoft.com>
> > Subject: [PATCH V2 2/4] UefiCpuPkg/CpuDxe: Implement Cpu2 protocol
> >
> > From: Sean Brogan <sean.brogan@microsoft.com>
> >
> > REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1400
> >
> > Implement Cp2 protocol: it has one interface to enable the interrupt
> > and put cpu to sleep and wait for an interrupt.
> >
> > Cc: Eric Dong <eric.dong@intel.com>
> > Cc: Ray Ni <ray.ni@intel.com>
> > Cc: Laszlo Ersek <lersek@redhat.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>
> > Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
> > ---
> > UefiCpuPkg/CpuDxe/CpuDxe.c | 38 +++++++++++++++++++
> > UefiCpuPkg/CpuDxe/CpuDxe.h | 25 ++++++++++++
> > UefiCpuPkg/CpuDxe/CpuDxe.inf | 3 ++
> > .../CpuDxe/Ia32/EnableInterruptsAndSleep.c | 24 ++++++++++++
> > .../CpuDxe/X64/EnableInterruptsAndSleep.nasm | 31 +++++++++++++++
> > 5 files changed, 121 insertions(+)
> > create mode 100644
> UefiCpuPkg/CpuDxe/Ia32/EnableInterruptsAndSleep.c
> > create mode 100644
> > UefiCpuPkg/CpuDxe/X64/EnableInterruptsAndSleep.nasm
> >
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V2 2/4] UefiCpuPkg/CpuDxe: Implement Cpu2 protocol
2019-07-11 2:36 ` Gao, Zhichao
@ 2019-07-11 10:24 ` Laszlo Ersek
2019-07-11 10:48 ` Leif Lindholm
0 siblings, 1 reply; 15+ messages in thread
From: Laszlo Ersek @ 2019-07-11 10:24 UTC (permalink / raw)
To: Gao, Zhichao, Dong, Eric, devel@edk2.groups.io
Cc: Sean Brogan, Ni, Ray, Gao, Liming, Michael Turner, Bret Barkelew,
Michael Kinney, Leif Lindholm, Andrew Fish
On 07/11/19 04:36, Gao, Zhichao wrote:
>
>
>> -----Original Message-----
>> From: Dong, Eric
>> Sent: Thursday, July 11, 2019 10:22 AM
>> To: Gao, Zhichao <zhichao.gao@intel.com>; devel@edk2.groups.io
>> Cc: Sean Brogan <sean.brogan@microsoft.com>; Ni, Ray <ray.ni@intel.com>;
>> Laszlo Ersek <lersek@redhat.com>; Gao, Liming <liming.gao@intel.com>;
>> Michael Turner <Michael.Turner@microsoft.com>; Bret Barkelew
>> <Bret.Barkelew@microsoft.com>
>> Subject: RE: [PATCH V2 2/4] UefiCpuPkg/CpuDxe: Implement Cpu2 protocol
>>
>> Hi Zhizhao,
>>
>> The new add files don't have copyright info, is it ok?
Good catch!
> I forgot add file comment for Ia32/EnableInterruptsAndSleep.c. Also lose the BSD+Patent License. I would add them in the future.
> For the copyright:
> The code is contributed by Microsoft at https://github.com/Microsoft/mu_basecore/blob/release/201808/MdePkg/Library/BaseLib/X64/EnableInterrupts.nasm#L48
> I move the code from MdePkg to UefiCpuPkg to a single file. It is inappropriate to add both intel (not contributor) and Microsoft (they didn't add in the above link).
> So I keep the copyright blank. I don't know whether it is ok without copyright.
Every single source file must have both copyright notice(s) and
licensing information.
- Licensing information without copyright holder(s) makes zero sense --
the licensing terms are offered / dictated *by* the copyright holders.
- A copyright notice in itself, without a suitable license (including
the "no license" case) does not provide us with the necessary rights to
carry the source file in the edk2 project.
Regarding "EnableInterrupts.nasm". This looks like a tricky situation.
Under the above link, the file is currently missing a Microsoft (C)
notice (the last commit to modify the file is apparently from Microsoft,
b621971). So that should be addressed in Project Mu first, in my opinion.
Then, Microsoft should please relicense the file under BSD+Patent, from
pure 2-BSD. If they disagree, then the 2-BSD is still acceptable for edk2.
Then, the file could be imported into edk2, carrying
- the MS (C) notice,
- the Intel (C) notice (extended to 2019),
- the license -- BSD+Patent, or else 2-BSD --, expressed with an SPDX ID
(not open-coded license text).
Note: I'm not a lawyer; the above is just my opinion. CC'ing the other
stewards.
Thanks,
Laszlo
>
> Hi Liming,
>
> What's your opinion?
>
> Thanks,
> Zhichao
>
>>
>> Thanks,
>> Eric
>>
>>> -----Original Message-----
>>> From: Gao, Zhichao
>>> Sent: Tuesday, July 9, 2019 4:40 PM
>>> To: devel@edk2.groups.io
>>> Cc: Sean Brogan <sean.brogan@microsoft.com>; Dong, Eric
>>> <eric.dong@intel.com>; Ni, Ray <ray.ni@intel.com>; Laszlo Ersek
>>> <lersek@redhat.com>; Gao, Liming <liming.gao@intel.com>; Michael
>>> Turner <Michael.Turner@microsoft.com>; Bret Barkelew
>>> <Bret.Barkelew@microsoft.com>
>>> Subject: [PATCH V2 2/4] UefiCpuPkg/CpuDxe: Implement Cpu2 protocol
>>>
>>> From: Sean Brogan <sean.brogan@microsoft.com>
>>>
>>> REF: https://bugzilla.tianocore.org/show_bug.cgi?id=1400
>>>
>>> Implement Cp2 protocol: it has one interface to enable the interrupt
>>> and put cpu to sleep and wait for an interrupt.
>>>
>>> Cc: Eric Dong <eric.dong@intel.com>
>>> Cc: Ray Ni <ray.ni@intel.com>
>>> Cc: Laszlo Ersek <lersek@redhat.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>
>>> Signed-off-by: Zhichao Gao <zhichao.gao@intel.com>
>>> ---
>>> UefiCpuPkg/CpuDxe/CpuDxe.c | 38 +++++++++++++++++++
>>> UefiCpuPkg/CpuDxe/CpuDxe.h | 25 ++++++++++++
>>> UefiCpuPkg/CpuDxe/CpuDxe.inf | 3 ++
>>> .../CpuDxe/Ia32/EnableInterruptsAndSleep.c | 24 ++++++++++++
>>> .../CpuDxe/X64/EnableInterruptsAndSleep.nasm | 31 +++++++++++++++
>>> 5 files changed, 121 insertions(+)
>>> create mode 100644
>> UefiCpuPkg/CpuDxe/Ia32/EnableInterruptsAndSleep.c
>>> create mode 100644
>>> UefiCpuPkg/CpuDxe/X64/EnableInterruptsAndSleep.nasm
>>>
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH V2 2/4] UefiCpuPkg/CpuDxe: Implement Cpu2 protocol
2019-07-11 10:24 ` Laszlo Ersek
@ 2019-07-11 10:48 ` Leif Lindholm
0 siblings, 0 replies; 15+ messages in thread
From: Leif Lindholm @ 2019-07-11 10:48 UTC (permalink / raw)
To: Laszlo Ersek
Cc: Gao, Zhichao, Dong, Eric, devel@edk2.groups.io, Sean Brogan,
Ni, Ray, Gao, Liming, Michael Turner, Bret Barkelew,
Michael Kinney, Andrew Fish
On Thu, Jul 11, 2019 at 12:24:46PM +0200, Laszlo Ersek wrote:
> Every single source file must have both copyright notice(s) and
> licensing information.
>
> - Licensing information without copyright holder(s) makes zero sense --
> the licensing terms are offered / dictated *by* the copyright holders.
>
> - A copyright notice in itself, without a suitable license (including
> the "no license" case) does not provide us with the necessary rights to
> carry the source file in the edk2 project.
Agree with all of the above.
> Regarding "EnableInterrupts.nasm". This looks like a tricky situation.
> Under the above link, the file is currently missing a Microsoft (C)
> notice (the last commit to modify the file is apparently from Microsoft,
> b621971). So that should be addressed in Project Mu first, in my opinion.
>
> Then, Microsoft should please relicense the file under BSD+Patent, from
> pure 2-BSD. If they disagree, then the 2-BSD is still acceptable for edk2.
>
> Then, the file could be imported into edk2, carrying
> - the MS (C) notice,
> - the Intel (C) notice (extended to 2019),
> - the license -- BSD+Patent, or else 2-BSD --, expressed with an SPDX ID
> (not open-coded license text).
While I agree it would be better if Microsoft relicensed it as
BSD+Patent, BSD+Patent is a strict superset of 2-clause BSD - so there
is no point in TianoCore to carry 2-Clause BSD code.
We can just add the explicit patent grant on contribution (i.e. change
the license header for a BSD+Patent SPDX tag). And we should, because
otherwise without the tianocore contribution agreement, we open
ourselves up to submarine contributions.
The downside to doing that would be if we wanted the code to be able
to flow in both directions, which is why it would be good if
Microsoft relicensed (and also because then they would be making an
explicit statement that they weren't trying to submarine TianoCore).
Best Regards,
Leif
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2019-07-11 10:48 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2019-07-09 8:39 [PATCH V2 0/4] Fix race condition and add event protocol Gao, Zhichao
2019-07-09 8:39 ` [PATCH V2 1/4] MdeModulePkg: Add gEdkiiCpu2ProtocolGuid and header file Gao, Zhichao
2019-07-09 8:39 ` [PATCH V2 2/4] UefiCpuPkg/CpuDxe: Implement Cpu2 protocol Gao, Zhichao
2019-07-11 2:22 ` Dong, Eric
2019-07-11 2:36 ` Gao, Zhichao
2019-07-11 10:24 ` Laszlo Ersek
2019-07-11 10:48 ` Leif Lindholm
2019-07-09 8:39 ` [PATCH V2 3/4] MdeModulePkg: Add gEdkiiCommonEventProtocolGuid for creating event Gao, Zhichao
2019-07-09 8:39 ` [PATCH V2 4/4] MdeModulePkg/DxeMain: Implement common event protocol Gao, Zhichao
2019-07-09 9:23 ` Wang, Jian J
2019-07-10 0:24 ` Gao, Zhichao
2019-07-10 8:46 ` Wang, Jian J
2019-07-11 0:20 ` Gao, Zhichao
2019-07-11 1:26 ` Wang, Jian J
2019-07-10 12:20 ` [edk2-devel] [PATCH V2 0/4] Fix race condition and add " Laszlo Ersek
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox