* [edk2-devel] [PATCH v3 1/5] MdeModulePkg: Move NestedInterruptTplLib to MdeModulePkg
[not found] ` <20240123153104.2451759-1-mcb30@ipxe.org>
@ 2024-01-23 15:31 ` Michael Brown
2024-01-23 15:31 ` [edk2-devel] [PATCH v3 2/5] MdeModulePkg: Add missing Iret.h to NestedInterruptTplLib sources list Michael Brown
` (3 subsequent siblings)
4 siblings, 0 replies; 19+ messages in thread
From: Michael Brown @ 2024-01-23 15:31 UTC (permalink / raw)
To: devel; +Cc: Ray Ni, Gerd Hoffmann, Laszlo Ersek, Michael Brown,
Michael D Kinney
NestedInterruptTplLib provides a way for timer interrupt handlers
(which must support nested interrupts) to prevent unbounded stack
consumption.
The underlying issue was first observed in OvmfPkg, since interrupt
storms can arise more easily in virtual machines due to CPU
starvation. However, careful investigation shows that the unbounded
stack consumption can also occur in physical machines.
Move NestedInterruptTplLib from OvmfPkg to MdeModulePkg so that it can
more easily be consumed by drivers outside of OvmfPkg.
Add NestedInterruptTplLib to the MdeModulePkg component build list for
IA32 and X64 only, since those are the only two architectures for
which DisableInterruptsOnIret() is currently supported.
Cc: Ray Ni <ray.ni@intel.com>
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: Michael D Kinney <michael.d.kinney@intel.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Michael Brown <mcb30@ipxe.org>
---
MdeModulePkg/MdeModulePkg.dec | 4 ++++
OvmfPkg/OvmfPkg.dec | 4 ----
MdeModulePkg/MdeModulePkg.dsc | 1 +
OvmfPkg/AmdSev/AmdSevX64.dsc | 2 +-
OvmfPkg/CloudHv/CloudHvX64.dsc | 2 +-
OvmfPkg/IntelTdx/IntelTdxX64.dsc | 2 +-
OvmfPkg/Microvm/MicrovmX64.dsc | 2 +-
OvmfPkg/OvmfPkgIa32.dsc | 2 +-
OvmfPkg/OvmfPkgIa32X64.dsc | 2 +-
OvmfPkg/OvmfPkgX64.dsc | 2 +-
OvmfPkg/OvmfXen.dsc | 2 +-
UefiPayloadPkg/UefiPayloadPkg.dsc | 2 +-
.../Library/NestedInterruptTplLib/NestedInterruptTplLib.inf | 2 +-
OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf | 2 +-
.../Include/Library/NestedInterruptTplLib.h | 0
.../Library/NestedInterruptTplLib/Iret.h | 0
.../Library/NestedInterruptTplLib/Iret.c | 0
{OvmfPkg => MdeModulePkg}/Library/NestedInterruptTplLib/Tpl.c | 0
18 files changed, 16 insertions(+), 15 deletions(-)
rename {OvmfPkg => MdeModulePkg}/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf (91%)
rename {OvmfPkg => MdeModulePkg}/Include/Library/NestedInterruptTplLib.h (100%)
rename {OvmfPkg => MdeModulePkg}/Library/NestedInterruptTplLib/Iret.h (100%)
rename {OvmfPkg => MdeModulePkg}/Library/NestedInterruptTplLib/Iret.c (100%)
rename {OvmfPkg => MdeModulePkg}/Library/NestedInterruptTplLib/Tpl.c (100%)
diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index a2cd83345f5b..d6fb729af5a7 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -169,6 +169,10 @@ [LibraryClasses]
#
ImagePropertiesRecordLib|Include/Library/ImagePropertiesRecordLib.h
+ ## @libraryclass Handle TPL changes within nested interrupt handlers
+ #
+ NestedInterruptTplLib|Include/Library/NestedInterruptTplLib.h
+
[Guids]
## MdeModule package token space guid
# Include/Guid/MdeModulePkgTokenSpace.h
diff --git a/OvmfPkg/OvmfPkg.dec b/OvmfPkg/OvmfPkg.dec
index b44fa039f76c..05d43d5a6861 100644
--- a/OvmfPkg/OvmfPkg.dec
+++ b/OvmfPkg/OvmfPkg.dec
@@ -41,10 +41,6 @@ [LibraryClasses]
#
MemEncryptTdxLib|Include/Library/MemEncryptTdxLib.h
- ## @libraryclass Handle TPL changes within nested interrupt handlers
- #
- NestedInterruptTplLib|Include/Library/NestedInterruptTplLib.h
-
## @libraryclass Save and restore variables using a file
#
NvVarsFileLib|Include/Library/NvVarsFileLib.h
diff --git a/MdeModulePkg/MdeModulePkg.dsc b/MdeModulePkg/MdeModulePkg.dsc
index 6b3052ff4614..5b9ddfd26e75 100644
--- a/MdeModulePkg/MdeModulePkg.dsc
+++ b/MdeModulePkg/MdeModulePkg.dsc
@@ -526,6 +526,7 @@ [Components.IA32, Components.X64]
MdeModulePkg/Library/TraceHubDebugSysTLib/BaseTraceHubDebugSysTLib.inf
MdeModulePkg/Library/TraceHubDebugSysTLib/PeiTraceHubDebugSysTLib.inf
MdeModulePkg/Library/TraceHubDebugSysTLib/DxeSmmTraceHubDebugSysTLib.inf
+ MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
[Components.X64]
MdeModulePkg/Universal/CapsulePei/CapsuleX64.inf
diff --git a/OvmfPkg/AmdSev/AmdSevX64.dsc b/OvmfPkg/AmdSev/AmdSevX64.dsc
index a31a89344a60..80456f878a22 100644
--- a/OvmfPkg/AmdSev/AmdSevX64.dsc
+++ b/OvmfPkg/AmdSev/AmdSevX64.dsc
@@ -354,7 +354,7 @@ [LibraryClasses.common.DXE_DRIVER]
!endif
PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
MpInitLib|UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
- NestedInterruptTplLib|OvmfPkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
+ NestedInterruptTplLib|MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
QemuLoadImageLib|OvmfPkg/Library/GenericQemuLoadImageLib/GenericQemuLoadImageLib.inf
diff --git a/OvmfPkg/CloudHv/CloudHvX64.dsc b/OvmfPkg/CloudHv/CloudHvX64.dsc
index b522fa10594d..9c6c68ae2c35 100644
--- a/OvmfPkg/CloudHv/CloudHvX64.dsc
+++ b/OvmfPkg/CloudHv/CloudHvX64.dsc
@@ -394,7 +394,7 @@ [LibraryClasses.common.DXE_DRIVER]
!endif
PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
MpInitLib|UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
- NestedInterruptTplLib|OvmfPkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
+ NestedInterruptTplLib|MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
QemuLoadImageLib|OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf
diff --git a/OvmfPkg/IntelTdx/IntelTdxX64.dsc b/OvmfPkg/IntelTdx/IntelTdxX64.dsc
index 82e3e41cfc57..5270c59e1279 100644
--- a/OvmfPkg/IntelTdx/IntelTdxX64.dsc
+++ b/OvmfPkg/IntelTdx/IntelTdxX64.dsc
@@ -310,7 +310,7 @@ [LibraryClasses.common.DXE_DRIVER]
LockBoxLib|OvmfPkg/Library/LockBoxLib/LockBoxDxeLib.inf
PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
MpInitLib|UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
- NestedInterruptTplLib|OvmfPkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
+ NestedInterruptTplLib|MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
QemuLoadImageLib|OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf
diff --git a/OvmfPkg/Microvm/MicrovmX64.dsc b/OvmfPkg/Microvm/MicrovmX64.dsc
index 063324cd0572..8ec00eaf2015 100644
--- a/OvmfPkg/Microvm/MicrovmX64.dsc
+++ b/OvmfPkg/Microvm/MicrovmX64.dsc
@@ -393,7 +393,7 @@ [LibraryClasses.common.DXE_DRIVER]
PciPcdProducerLib|OvmfPkg/Fdt/FdtPciPcdProducerLib/FdtPciPcdProducerLib.inf
PciExpressLib|OvmfPkg/Library/BaseCachingPciExpressLib/BaseCachingPciExpressLib.inf
MpInitLib|UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
- NestedInterruptTplLib|OvmfPkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
+ NestedInterruptTplLib|MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
QemuLoadImageLib|OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf
diff --git a/OvmfPkg/OvmfPkgIa32.dsc b/OvmfPkg/OvmfPkgIa32.dsc
index 28379961a78e..154afbbaf45d 100644
--- a/OvmfPkg/OvmfPkgIa32.dsc
+++ b/OvmfPkg/OvmfPkgIa32.dsc
@@ -401,7 +401,7 @@ [LibraryClasses.common.DXE_DRIVER]
!endif
PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
MpInitLib|UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
- NestedInterruptTplLib|OvmfPkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
+ NestedInterruptTplLib|MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
QemuLoadImageLib|OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf
diff --git a/OvmfPkg/OvmfPkgIa32X64.dsc b/OvmfPkg/OvmfPkgIa32X64.dsc
index 5e9eee628aea..0f6173607759 100644
--- a/OvmfPkg/OvmfPkgIa32X64.dsc
+++ b/OvmfPkg/OvmfPkgIa32X64.dsc
@@ -407,7 +407,7 @@ [LibraryClasses.common.DXE_DRIVER]
!endif
PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
MpInitLib|UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
- NestedInterruptTplLib|OvmfPkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
+ NestedInterruptTplLib|MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
QemuLoadImageLib|OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf
diff --git a/OvmfPkg/OvmfPkgX64.dsc b/OvmfPkg/OvmfPkgX64.dsc
index bf4c7906c460..93e7e9a18cd0 100644
--- a/OvmfPkg/OvmfPkgX64.dsc
+++ b/OvmfPkg/OvmfPkgX64.dsc
@@ -427,7 +427,7 @@ [LibraryClasses.common.DXE_DRIVER]
!endif
PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
MpInitLib|UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
- NestedInterruptTplLib|OvmfPkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
+ NestedInterruptTplLib|MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
QemuLoadImageLib|OvmfPkg/Library/X86QemuLoadImageLib/X86QemuLoadImageLib.inf
diff --git a/OvmfPkg/OvmfXen.dsc b/OvmfPkg/OvmfXen.dsc
index 976b795d41c9..c2472aca38aa 100644
--- a/OvmfPkg/OvmfXen.dsc
+++ b/OvmfPkg/OvmfXen.dsc
@@ -340,7 +340,7 @@ [LibraryClasses.common.DXE_DRIVER]
!endif
PciLib|OvmfPkg/Library/DxePciLibI440FxQ35/DxePciLibI440FxQ35.inf
MpInitLib|UefiCpuPkg/Library/MpInitLib/DxeMpInitLib.inf
- NestedInterruptTplLib|OvmfPkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
+ NestedInterruptTplLib|MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
QemuFwCfgS3Lib|OvmfPkg/Library/QemuFwCfgS3Lib/DxeQemuFwCfgS3LibFwCfg.inf
[LibraryClasses.common.UEFI_APPLICATION]
diff --git a/UefiPayloadPkg/UefiPayloadPkg.dsc b/UefiPayloadPkg/UefiPayloadPkg.dsc
index 4f195c1e5212..51f18b5c0c2f 100644
--- a/UefiPayloadPkg/UefiPayloadPkg.dsc
+++ b/UefiPayloadPkg/UefiPayloadPkg.dsc
@@ -687,7 +687,7 @@ [Components.X64]
!elseif $(TIMER_SUPPORT) == "LAPIC"
OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf {
<LibraryClasses>
- NestedInterruptTplLib|OvmfPkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
+ NestedInterruptTplLib|MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
}
!else
!error "Invalid TIMER_SUPPORT"
diff --git a/OvmfPkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf b/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
similarity index 91%
rename from OvmfPkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
rename to MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
index 5eafb4197842..1e03e1364e0f 100644
--- a/OvmfPkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
+++ b/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
@@ -24,7 +24,7 @@ [Sources]
[Packages]
MdePkg/MdePkg.dec
- OvmfPkg/OvmfPkg.dec
+ MdeModulePkg/MdeModulePkg.dec
[LibraryClasses]
BaseLib
diff --git a/OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf b/OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
index b85965c75ea3..6dc5ec9bf4aa 100644
--- a/OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
+++ b/OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
@@ -20,8 +20,8 @@ [Defines]
[Packages]
MdePkg/MdePkg.dec
+ MdeModulePkg/MdeModulePkg.dec
UefiCpuPkg/UefiCpuPkg.dec
- OvmfPkg/OvmfPkg.dec
[LibraryClasses]
UefiBootServicesTableLib
diff --git a/OvmfPkg/Include/Library/NestedInterruptTplLib.h b/MdeModulePkg/Include/Library/NestedInterruptTplLib.h
similarity index 100%
rename from OvmfPkg/Include/Library/NestedInterruptTplLib.h
rename to MdeModulePkg/Include/Library/NestedInterruptTplLib.h
diff --git a/OvmfPkg/Library/NestedInterruptTplLib/Iret.h b/MdeModulePkg/Library/NestedInterruptTplLib/Iret.h
similarity index 100%
rename from OvmfPkg/Library/NestedInterruptTplLib/Iret.h
rename to MdeModulePkg/Library/NestedInterruptTplLib/Iret.h
diff --git a/OvmfPkg/Library/NestedInterruptTplLib/Iret.c b/MdeModulePkg/Library/NestedInterruptTplLib/Iret.c
similarity index 100%
rename from OvmfPkg/Library/NestedInterruptTplLib/Iret.c
rename to MdeModulePkg/Library/NestedInterruptTplLib/Iret.c
diff --git a/OvmfPkg/Library/NestedInterruptTplLib/Tpl.c b/MdeModulePkg/Library/NestedInterruptTplLib/Tpl.c
similarity index 100%
rename from OvmfPkg/Library/NestedInterruptTplLib/Tpl.c
rename to MdeModulePkg/Library/NestedInterruptTplLib/Tpl.c
--
2.43.0
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114207): https://edk2.groups.io/g/devel/message/114207
Mute This Topic: https://groups.io/mt/103911603/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 19+ messages in thread* [edk2-devel] [PATCH v3 2/5] MdeModulePkg: Add missing Iret.h to NestedInterruptTplLib sources list
[not found] ` <20240123153104.2451759-1-mcb30@ipxe.org>
2024-01-23 15:31 ` [edk2-devel] [PATCH v3 1/5] " Michael Brown
@ 2024-01-23 15:31 ` Michael Brown
2024-01-23 15:31 ` [edk2-devel] [PATCH v3 3/5] MdeModulePkg: Do nothing on NestedInterruptRestoreTPL(TPL_HIGH_LEVEL) Michael Brown
` (2 subsequent siblings)
4 siblings, 0 replies; 19+ messages in thread
From: Michael Brown @ 2024-01-23 15:31 UTC (permalink / raw)
To: devel; +Cc: Ray Ni, Gerd Hoffmann, Laszlo Ersek, Michael Brown
Allow build system to detect changes to Iret.h.
Suggested-by: Ray Ni <ray.ni@intel.com>
Reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Michael Brown <mcb30@ipxe.org>
---
.../Library/NestedInterruptTplLib/NestedInterruptTplLib.inf | 1 +
1 file changed, 1 insertion(+)
diff --git a/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf b/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
index 1e03e1364e0f..f130d6dcd213 100644
--- a/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
+++ b/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
@@ -20,6 +20,7 @@ [Defines]
[Sources]
Tpl.c
+ Iret.h
Iret.c
[Packages]
--
2.43.0
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114208): https://edk2.groups.io/g/devel/message/114208
Mute This Topic: https://groups.io/mt/103911605/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 19+ messages in thread* [edk2-devel] [PATCH v3 3/5] MdeModulePkg: Do nothing on NestedInterruptRestoreTPL(TPL_HIGH_LEVEL)
[not found] ` <20240123153104.2451759-1-mcb30@ipxe.org>
2024-01-23 15:31 ` [edk2-devel] [PATCH v3 1/5] " Michael Brown
2024-01-23 15:31 ` [edk2-devel] [PATCH v3 2/5] MdeModulePkg: Add missing Iret.h to NestedInterruptTplLib sources list Michael Brown
@ 2024-01-23 15:31 ` Michael Brown
2024-01-23 16:32 ` Laszlo Ersek
2024-01-23 15:31 ` [edk2-devel] [PATCH v3 4/5] MdeModulePkg: Add self-tests for NestedInterruptTplLib Michael Brown
2024-01-23 15:31 ` [edk2-devel] [PATCH v3 5/5] MdeModulePkg: Extend NestedInterruptTplLib to support Arm CPUs Michael Brown
4 siblings, 1 reply; 19+ messages in thread
From: Michael Brown @ 2024-01-23 15:31 UTC (permalink / raw)
To: devel; +Cc: Ray Ni, Gerd Hoffmann, Laszlo Ersek, Michael Brown
At TPL_HIGH_LEVEL, CPU interrupts are disabled (as per the UEFI
specification) and so we should never encounter a situation in which
an interrupt occurs at TPL_HIGH_LEVEL.
Restoring TPL to TPL_HIGH_LEVEL is always a no-op. Return immediately
from NestedInterruptRestoreTPL(TPL_HIGH_LEVEL), so that we do not need
to consider the effect of this possible invariant violation on the
remainder of the logic.
Signed-off-by: Michael Brown <mcb30@ipxe.org>
---
MdeModulePkg/Library/NestedInterruptTplLib/Tpl.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/MdeModulePkg/Library/NestedInterruptTplLib/Tpl.c b/MdeModulePkg/Library/NestedInterruptTplLib/Tpl.c
index d56c12a44529..99af553ab189 100644
--- a/MdeModulePkg/Library/NestedInterruptTplLib/Tpl.c
+++ b/MdeModulePkg/Library/NestedInterruptTplLib/Tpl.c
@@ -99,6 +99,19 @@ NestedInterruptRestoreTPL (
EFI_TPL SavedInProgressRestoreTPL;
BOOLEAN DeferredRestoreTPL;
+ //
+ // At TPL_HIGH_LEVEL, CPU interrupts are disabled (as per the UEFI
+ // specification) and so we should never encounter a situation in
+ // which InterruptedTPL==TPL_HIGH_LEVEL.
+ //
+ // Restoring TPL to TPL_HIGH_LEVEL is always a no-op. Return
+ // immediately so that we do not need to consider the effect of this
+ // possible invariant violation in the logic below.
+ //
+ if (InterruptedTPL >= TPL_HIGH_LEVEL) {
+ return;
+ }
+
//
// If the TPL at which this interrupt occurred is equal to that of
// the in-progress RestoreTPL() for an outer instance of the same
--
2.43.0
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114209): https://edk2.groups.io/g/devel/message/114209
Mute This Topic: https://groups.io/mt/103911606/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [edk2-devel] [PATCH v3 3/5] MdeModulePkg: Do nothing on NestedInterruptRestoreTPL(TPL_HIGH_LEVEL)
2024-01-23 15:31 ` [edk2-devel] [PATCH v3 3/5] MdeModulePkg: Do nothing on NestedInterruptRestoreTPL(TPL_HIGH_LEVEL) Michael Brown
@ 2024-01-23 16:32 ` Laszlo Ersek
2024-01-23 16:59 ` Michael Brown
0 siblings, 1 reply; 19+ messages in thread
From: Laszlo Ersek @ 2024-01-23 16:32 UTC (permalink / raw)
To: devel, mcb30; +Cc: Ray Ni, Gerd Hoffmann
On 1/23/24 16:31, Michael Brown wrote:
> At TPL_HIGH_LEVEL, CPU interrupts are disabled (as per the UEFI
> specification) and so we should never encounter a situation in which
> an interrupt occurs at TPL_HIGH_LEVEL.
>
> Restoring TPL to TPL_HIGH_LEVEL is always a no-op. Return immediately
> from NestedInterruptRestoreTPL(TPL_HIGH_LEVEL), so that we do not need
> to consider the effect of this possible invariant violation on the
> remainder of the logic.
>
> Signed-off-by: Michael Brown <mcb30@ipxe.org>
> ---
> MdeModulePkg/Library/NestedInterruptTplLib/Tpl.c | 13 +++++++++++++
> 1 file changed, 13 insertions(+)
>
> diff --git a/MdeModulePkg/Library/NestedInterruptTplLib/Tpl.c b/MdeModulePkg/Library/NestedInterruptTplLib/Tpl.c
> index d56c12a44529..99af553ab189 100644
> --- a/MdeModulePkg/Library/NestedInterruptTplLib/Tpl.c
> +++ b/MdeModulePkg/Library/NestedInterruptTplLib/Tpl.c
> @@ -99,6 +99,19 @@ NestedInterruptRestoreTPL (
> EFI_TPL SavedInProgressRestoreTPL;
> BOOLEAN DeferredRestoreTPL;
>
> + //
> + // At TPL_HIGH_LEVEL, CPU interrupts are disabled (as per the UEFI
> + // specification) and so we should never encounter a situation in
> + // which InterruptedTPL==TPL_HIGH_LEVEL.
> + //
> + // Restoring TPL to TPL_HIGH_LEVEL is always a no-op. Return
> + // immediately so that we do not need to consider the effect of this
> + // possible invariant violation in the logic below.
> + //
> + if (InterruptedTPL >= TPL_HIGH_LEVEL) {
> + return;
> + }
> +
> //
> // If the TPL at which this interrupt occurred is equal to that of
> // the in-progress RestoreTPL() for an outer instance of the same
Feels like the handling logic might as well be "panic" here (except edk2
does not have a central panic API that's suitable for all platforms). I
probably missed the previous discussion that led to this patch. Either
way, it seems reasonable.
Acked-by: Laszlo Ersek <lersek@redhat.com>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114215): https://edk2.groups.io/g/devel/message/114215
Mute This Topic: https://groups.io/mt/103911606/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [edk2-devel] [PATCH v3 3/5] MdeModulePkg: Do nothing on NestedInterruptRestoreTPL(TPL_HIGH_LEVEL)
2024-01-23 16:32 ` Laszlo Ersek
@ 2024-01-23 16:59 ` Michael Brown
2024-01-24 12:52 ` Laszlo Ersek
0 siblings, 1 reply; 19+ messages in thread
From: Michael Brown @ 2024-01-23 16:59 UTC (permalink / raw)
To: devel, lersek; +Cc: Ray Ni, Gerd Hoffmann
On 23/01/2024 16:32, Laszlo Ersek wrote:
> On 1/23/24 16:31, Michael Brown wrote:
>> At TPL_HIGH_LEVEL, CPU interrupts are disabled (as per the UEFI
>> specification) and so we should never encounter a situation in which
>> an interrupt occurs at TPL_HIGH_LEVEL.
>>
>> Restoring TPL to TPL_HIGH_LEVEL is always a no-op. Return immediately
>> from NestedInterruptRestoreTPL(TPL_HIGH_LEVEL), so that we do not need
>> to consider the effect of this possible invariant violation on the
>> remainder of the logic.
>
> Feels like the handling logic might as well be "panic" here (except edk2
> does not have a central panic API that's suitable for all platforms). I
> probably missed the previous discussion that led to this patch. Either
> way, it seems reasonable.
>
> Acked-by: Laszlo Ersek <lersek@redhat.com>
Thank you. We can't panic because there are some bootloaders (*cough*
Microsoft *cough*) that illegally call RaiseTPL(TPL_HIGH_LEVEL) and then
even more illegally enable interrupts via STI. Gerd tracked this down
before, which lead to commit
https://github.com/tianocore/edk2/commit/bee67e0c1
I found another way to trigger a RestoreTPL(TPL_HIGH_LEVEL) while I was
testing the self-tests by deliberately breaking
DisableInterruptsOnIret() to fail to disable interrupts. This also
induces a situation in which we end up at TPL_HIGH_LEVEL with interrupts
enabled.
This ended up triggering an assertion (due to the invariant violation)
in NestedInterruptRestoreTPL() before reaching the point in the
self-test that would have reported a more meaningful error message.
Adding the bypass is justifiable on its own merits (as per the reasoning
in the commit), and it avoids needing to add clutter to the complex
logic in NestedInterruptRestoreTPL() just to ensure that the self-test
fails on the desired ASSERT().
I decided against trying to explain all that in the commit message, but
I can have a go if you think it needs to be captured. :)
Thanks,
Michael
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114220): https://edk2.groups.io/g/devel/message/114220
Mute This Topic: https://groups.io/mt/103911606/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [edk2-devel] [PATCH v3 3/5] MdeModulePkg: Do nothing on NestedInterruptRestoreTPL(TPL_HIGH_LEVEL)
2024-01-23 16:59 ` Michael Brown
@ 2024-01-24 12:52 ` Laszlo Ersek
0 siblings, 0 replies; 19+ messages in thread
From: Laszlo Ersek @ 2024-01-24 12:52 UTC (permalink / raw)
To: Michael Brown, devel; +Cc: Ray Ni, Gerd Hoffmann
On 1/23/24 17:59, Michael Brown wrote:
> On 23/01/2024 16:32, Laszlo Ersek wrote:
>> On 1/23/24 16:31, Michael Brown wrote:
>>> At TPL_HIGH_LEVEL, CPU interrupts are disabled (as per the UEFI
>>> specification) and so we should never encounter a situation in which
>>> an interrupt occurs at TPL_HIGH_LEVEL.
>>>
>>> Restoring TPL to TPL_HIGH_LEVEL is always a no-op. Return immediately
>>> from NestedInterruptRestoreTPL(TPL_HIGH_LEVEL), so that we do not need
>>> to consider the effect of this possible invariant violation on the
>>> remainder of the logic.
>>
>> Feels like the handling logic might as well be "panic" here (except edk2
>> does not have a central panic API that's suitable for all platforms). I
>> probably missed the previous discussion that led to this patch. Either
>> way, it seems reasonable.
>>
>> Acked-by: Laszlo Ersek <lersek@redhat.com>
>
> Thank you. We can't panic because there are some bootloaders (*cough*
> Microsoft *cough*) that illegally call RaiseTPL(TPL_HIGH_LEVEL) and then
> even more illegally enable interrupts via STI. Gerd tracked this down
> before, which lead to commit
>
> https://github.com/tianocore/edk2/commit/bee67e0c1
>
> I found another way to trigger a RestoreTPL(TPL_HIGH_LEVEL) while I was
> testing the self-tests by deliberately breaking
> DisableInterruptsOnIret() to fail to disable interrupts. This also
> induces a situation in which we end up at TPL_HIGH_LEVEL with interrupts
> enabled.
>
> This ended up triggering an assertion (due to the invariant violation)
> in NestedInterruptRestoreTPL() before reaching the point in the
> self-test that would have reported a more meaningful error message.
>
> Adding the bypass is justifiable on its own merits (as per the reasoning
> in the commit), and it avoids needing to add clutter to the complex
> logic in NestedInterruptRestoreTPL() just to ensure that the self-test
> fails on the desired ASSERT().
>
> I decided against trying to explain all that in the commit message, but
> I can have a go if you think it needs to be captured. :)
Ugh, no, thanks :) This should suffice! :)
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114286): https://edk2.groups.io/g/devel/message/114286
Mute This Topic: https://groups.io/mt/103911606/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 19+ messages in thread
* [edk2-devel] [PATCH v3 4/5] MdeModulePkg: Add self-tests for NestedInterruptTplLib
[not found] ` <20240123153104.2451759-1-mcb30@ipxe.org>
` (2 preceding siblings ...)
2024-01-23 15:31 ` [edk2-devel] [PATCH v3 3/5] MdeModulePkg: Do nothing on NestedInterruptRestoreTPL(TPL_HIGH_LEVEL) Michael Brown
@ 2024-01-23 15:31 ` Michael Brown
2024-01-23 16:55 ` Laszlo Ersek
2024-01-24 10:24 ` Ni, Ray
2024-01-23 15:31 ` [edk2-devel] [PATCH v3 5/5] MdeModulePkg: Extend NestedInterruptTplLib to support Arm CPUs Michael Brown
4 siblings, 2 replies; 19+ messages in thread
From: Michael Brown @ 2024-01-23 15:31 UTC (permalink / raw)
To: devel; +Cc: Ray Ni, Gerd Hoffmann, Laszlo Ersek, Michael Brown
Add the ability to perform self-tests on the first few invocations of
NestedInterruptRestoreTPL(), to verify that:
- the timer interrupt handler correctly rearms the timer interrupt
before calling NestedInterruptRestoreTPL(), and
- the architecture-specific DisableInterruptsOnIret() implementation
correctly causes interrupts to be disabled after the IRET or
equivalent instruction.
Any test failure will be treated as fatal and will halt the system
with an appropriate diagnostic message.
Each test invocation adds approximately one timer tick of delay to the
overall system startup time.
Only one test is performed by default (to avoid unnecessary system
startup delay). The number of tests performed can be controlled via
PcdNestedInterruptNumberOfSelfTests at build time.
Signed-off-by: Michael Brown <mcb30@ipxe.org>
---
MdeModulePkg/MdeModulePkg.dec | 4 +
.../NestedInterruptTplLib.inf | 3 +
.../Include/Library/NestedInterruptTplLib.h | 4 +
.../Library/NestedInterruptTplLib/Tpl.c | 129 ++++++++++++++++++
4 files changed, 140 insertions(+)
diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
index d6fb729af5a7..efd32c197b18 100644
--- a/MdeModulePkg/MdeModulePkg.dec
+++ b/MdeModulePkg/MdeModulePkg.dec
@@ -1142,6 +1142,10 @@ [PcdsFixedAtBuild]
# @Prompt Enable large address image loading.
gEfiMdeModulePkgTokenSpaceGuid.PcdImageLargeAddressLoad|TRUE|BOOLEAN|0x30001059
+ ## Number of NestedInterruptTplLib self-tests to perform at startup.
+ # @Prompt Number of NestedInterruptTplLib self-tests.
+ gEfiMdeModulePkgTokenSpaceGuid.PcdNestedInterruptNumberOfSelfTests|1|UINT32|0x30001060
+
[PcdsFixedAtBuild, PcdsPatchableInModule]
## Dynamic type PCD can be registered callback function for Pcd setting action.
# PcdMaxPeiPcdCallBackNumberPerPcdEntry indicates the maximum number of callback function
diff --git a/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf b/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
index f130d6dcd213..e67d899b9446 100644
--- a/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
+++ b/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
@@ -34,3 +34,6 @@ [LibraryClasses]
[Depex.common.DXE_DRIVER]
TRUE
+
+[Pcd]
+ gEfiMdeModulePkgTokenSpaceGuid.PcdNestedInterruptNumberOfSelfTests
diff --git a/MdeModulePkg/Include/Library/NestedInterruptTplLib.h b/MdeModulePkg/Include/Library/NestedInterruptTplLib.h
index 0ead6e4b346a..7dd934577e99 100644
--- a/MdeModulePkg/Include/Library/NestedInterruptTplLib.h
+++ b/MdeModulePkg/Include/Library/NestedInterruptTplLib.h
@@ -32,6 +32,10 @@ typedef struct {
/// interrupt handler.
///
BOOLEAN DeferredRestoreTPL;
+ ///
+ /// Number of self-tests performed.
+ ///
+ UINTN SelfTestCount;
} NESTED_INTERRUPT_STATE;
/**
diff --git a/MdeModulePkg/Library/NestedInterruptTplLib/Tpl.c b/MdeModulePkg/Library/NestedInterruptTplLib/Tpl.c
index 99af553ab189..dfe22331204f 100644
--- a/MdeModulePkg/Library/NestedInterruptTplLib/Tpl.c
+++ b/MdeModulePkg/Library/NestedInterruptTplLib/Tpl.c
@@ -17,6 +17,18 @@
#include "Iret.h"
+//
+// Number of self-tests to perform.
+//
+#define NUMBER_OF_SELF_TESTS \
+ (FixedPcdGet32 (PcdNestedInterruptNumberOfSelfTests))
+
+STATIC
+VOID
+NestedInterruptSelfTest (
+ IN NESTED_INTERRUPT_STATE *IsrState
+ );
+
/**
Raise the task priority level to TPL_HIGH_LEVEL.
@@ -211,6 +223,16 @@ NestedInterruptRestoreTPL (
//
DisableInterrupts ();
+ ///
+ /// Perform a limited number of self-tests on the first few
+ /// invocations.
+ ///
+ if ((IsrState->DeferredRestoreTPL == FALSE) &&
+ (IsrState->SelfTestCount < NUMBER_OF_SELF_TESTS)) {
+ IsrState->SelfTestCount++;
+ NestedInterruptSelfTest (IsrState);
+ }
+
//
// DEFERRAL RETURN POINT
//
@@ -248,3 +270,110 @@ NestedInterruptRestoreTPL (
}
}
}
+
+/**
+ Perform internal self-test.
+
+ Induce a delay to force a nested timer interrupt to take place, and
+ verify that the nested interrupt behaves as required.
+
+ @param IsrState A pointer to the state shared between all
+ invocations of the nested interrupt handler.
+**/
+VOID
+NestedInterruptSelfTest (
+ IN NESTED_INTERRUPT_STATE *IsrState
+ )
+{
+ UINTN SelfTestCount;
+ UINTN TimeOut;
+
+ //
+ // Record number of this self-test for debug messages.
+ //
+ SelfTestCount = IsrState->SelfTestCount;
+
+ //
+ // Re-enable interrupts and stall for up to one second to induce at
+ // least one more timer interrupt.
+ //
+ // This mimics the effect of an interrupt having occurred precisely
+ // at the end of our call to RestoreTPL(), with interrupts having
+ // been re-enabled by RestoreTPL() and with the interrupt happening
+ // to occur after the TPL has already been lowered back down to
+ // InterruptedTPL. (This is the scenario that can lead to stack
+ // exhaustion, as described above.)
+ //
+ ASSERT (GetInterruptState () == FALSE);
+ ASSERT (IsrState->DeferredRestoreTPL == FALSE);
+ EnableInterrupts();
+ for (TimeOut = 0; TimeOut < 1000; TimeOut++) {
+ //
+ // Stall for 1ms
+ //
+ gBS->Stall (1000);
+
+ //
+ // If we observe that interrupts have been spontaneously disabled,
+ // then this must be because the induced interrupt handler's call
+ // to NestedInterruptRestoreTPL() correctly chose to defer the
+ // RestoreTPL() call to the outer handler (i.e. to us).
+ //
+ if (GetInterruptState() == FALSE) {
+ ASSERT (IsrState->DeferredRestoreTPL == TRUE);
+ DEBUG ((
+ DEBUG_INFO,
+ "Nested interrupt self-test %u/%u passed\n",
+ SelfTestCount,
+ NUMBER_OF_SELF_TESTS
+ ));
+ return;
+ }
+ }
+
+ //
+ // The test has failed and we will halt the system. Disable
+ // interrupts now so that any test-induced interrupt storm does not
+ // prevent the fatal error messages from being displayed correctly.
+ //
+ DisableInterrupts();
+
+ //
+ // If we observe that DeferredRestoreTPL is TRUE then this indicates
+ // that an interrupt did occur and NestedInterruptRestoreTPL() chose
+ // to defer the RestoreTPL() call to the outer handler, but that
+ // DisableInterruptsOnIret() failed to cause interrupts to be
+ // disabled after the IRET or equivalent instruction.
+ //
+ // This error represents a bug in the architecture-specific
+ // implementation of DisableInterruptsOnIret().
+ //
+ if (IsrState->DeferredRestoreTPL == TRUE) {
+ DEBUG ((
+ DEBUG_ERROR,
+ "Nested interrupt self-test %u/%u failed: interrupts still enabled\n",
+ SelfTestCount,
+ NUMBER_OF_SELF_TESTS
+ ));
+ ASSERT (FALSE);
+ }
+
+ //
+ // If no timer interrupt occurred then this indicates that the timer
+ // interrupt handler failed to rearm the timer before calling
+ // NestedInterruptRestoreTPL(). This would prevent nested
+ // interrupts from occurring at all, which would break
+ // e.g. callbacks at TPL_CALLBACK that themselves wait on further
+ // timer events.
+ //
+ // This error represents a bug in the platform-specific timer
+ // interrupt handler.
+ //
+ DEBUG ((
+ DEBUG_ERROR,
+ "Nested interrupt self-test %u/%u failed: no nested interrupt\n",
+ SelfTestCount,
+ NUMBER_OF_SELF_TESTS
+ ));
+ ASSERT (FALSE);
+}
--
2.43.0
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114210): https://edk2.groups.io/g/devel/message/114210
Mute This Topic: https://groups.io/mt/103911608/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [edk2-devel] [PATCH v3 4/5] MdeModulePkg: Add self-tests for NestedInterruptTplLib
2024-01-23 15:31 ` [edk2-devel] [PATCH v3 4/5] MdeModulePkg: Add self-tests for NestedInterruptTplLib Michael Brown
@ 2024-01-23 16:55 ` Laszlo Ersek
2024-01-23 17:41 ` Michael Brown
2024-01-24 10:24 ` Ni, Ray
1 sibling, 1 reply; 19+ messages in thread
From: Laszlo Ersek @ 2024-01-23 16:55 UTC (permalink / raw)
To: devel, mcb30; +Cc: Ray Ni, Gerd Hoffmann
only superficial comments:
On 1/23/24 16:31, Michael Brown wrote:
> Add the ability to perform self-tests on the first few invocations of
> NestedInterruptRestoreTPL(), to verify that:
>
> - the timer interrupt handler correctly rearms the timer interrupt
> before calling NestedInterruptRestoreTPL(), and
>
> - the architecture-specific DisableInterruptsOnIret() implementation
> correctly causes interrupts to be disabled after the IRET or
> equivalent instruction.
>
> Any test failure will be treated as fatal and will halt the system
> with an appropriate diagnostic message.
>
> Each test invocation adds approximately one timer tick of delay to the
> overall system startup time.
>
> Only one test is performed by default (to avoid unnecessary system
> startup delay). The number of tests performed can be controlled via
> PcdNestedInterruptNumberOfSelfTests at build time.
>
> Signed-off-by: Michael Brown <mcb30@ipxe.org>
> ---
> MdeModulePkg/MdeModulePkg.dec | 4 +
> .../NestedInterruptTplLib.inf | 3 +
> .../Include/Library/NestedInterruptTplLib.h | 4 +
> .../Library/NestedInterruptTplLib/Tpl.c | 129 ++++++++++++++++++
> 4 files changed, 140 insertions(+)
(This is not even a comment, just a hint :) consider passing
"--stat=1000 --stat-graph-width=20" to git, when formatting the patches.
Those options deal well with the extremely long filenames / pathnames in
edk2.)
>
> diff --git a/MdeModulePkg/MdeModulePkg.dec b/MdeModulePkg/MdeModulePkg.dec
> index d6fb729af5a7..efd32c197b18 100644
> --- a/MdeModulePkg/MdeModulePkg.dec
> +++ b/MdeModulePkg/MdeModulePkg.dec
> @@ -1142,6 +1142,10 @@ [PcdsFixedAtBuild]
> # @Prompt Enable large address image loading.
> gEfiMdeModulePkgTokenSpaceGuid.PcdImageLargeAddressLoad|TRUE|BOOLEAN|0x30001059
>
> + ## Number of NestedInterruptTplLib self-tests to perform at startup.
> + # @Prompt Number of NestedInterruptTplLib self-tests.
> + gEfiMdeModulePkgTokenSpaceGuid.PcdNestedInterruptNumberOfSelfTests|1|UINT32|0x30001060
> +
> [PcdsFixedAtBuild, PcdsPatchableInModule]
> ## Dynamic type PCD can be registered callback function for Pcd setting action.
> # PcdMaxPeiPcdCallBackNumberPerPcdEntry indicates the maximum number of callback function
> diff --git a/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf b/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
> index f130d6dcd213..e67d899b9446 100644
> --- a/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
> +++ b/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
> @@ -34,3 +34,6 @@ [LibraryClasses]
>
> [Depex.common.DXE_DRIVER]
> TRUE
> +
> +[Pcd]
> + gEfiMdeModulePkgTokenSpaceGuid.PcdNestedInterruptNumberOfSelfTests
> diff --git a/MdeModulePkg/Include/Library/NestedInterruptTplLib.h b/MdeModulePkg/Include/Library/NestedInterruptTplLib.h
> index 0ead6e4b346a..7dd934577e99 100644
> --- a/MdeModulePkg/Include/Library/NestedInterruptTplLib.h
> +++ b/MdeModulePkg/Include/Library/NestedInterruptTplLib.h
> @@ -32,6 +32,10 @@ typedef struct {
> /// interrupt handler.
> ///
> BOOLEAN DeferredRestoreTPL;
> + ///
> + /// Number of self-tests performed.
> + ///
> + UINTN SelfTestCount;
> } NESTED_INTERRUPT_STATE;
>
> /**
I suggest that the new field be UINT32. The (exclusive) limit is a
32-bit PCD. Making the counter (potentially) wider than the limit is not
useful, but it's also a bit of a complication for the debug messages
(see below).
> diff --git a/MdeModulePkg/Library/NestedInterruptTplLib/Tpl.c b/MdeModulePkg/Library/NestedInterruptTplLib/Tpl.c
> index 99af553ab189..dfe22331204f 100644
> --- a/MdeModulePkg/Library/NestedInterruptTplLib/Tpl.c
> +++ b/MdeModulePkg/Library/NestedInterruptTplLib/Tpl.c
> @@ -17,6 +17,18 @@
>
> #include "Iret.h"
>
> +//
> +// Number of self-tests to perform.
> +//
> +#define NUMBER_OF_SELF_TESTS \
> + (FixedPcdGet32 (PcdNestedInterruptNumberOfSelfTests))
> +
> +STATIC
> +VOID
> +NestedInterruptSelfTest (
> + IN NESTED_INTERRUPT_STATE *IsrState
> + );
> +
> /**
> Raise the task priority level to TPL_HIGH_LEVEL.
>
> @@ -211,6 +223,16 @@ NestedInterruptRestoreTPL (
> //
> DisableInterrupts ();
>
> + ///
> + /// Perform a limited number of self-tests on the first few
> + /// invocations.
> + ///
> + if ((IsrState->DeferredRestoreTPL == FALSE) &&
This comment applies to several locations in the patch:
BOOLEANs should not be checked using explicit "== TRUE" and "== FALSE"
operators / comparisons; they should only be evaluated in their logical
contexts:
(Foo)
(!Bar)
etc
> + (IsrState->SelfTestCount < NUMBER_OF_SELF_TESTS)) {
> + IsrState->SelfTestCount++;
> + NestedInterruptSelfTest (IsrState);
> + }
> +
> //
> // DEFERRAL RETURN POINT
> //
> @@ -248,3 +270,110 @@ NestedInterruptRestoreTPL (
> }
> }
> }
> +
> +/**
> + Perform internal self-test.
> +
> + Induce a delay to force a nested timer interrupt to take place, and
> + verify that the nested interrupt behaves as required.
> +
> + @param IsrState A pointer to the state shared between all
> + invocations of the nested interrupt handler.
> +**/
> +VOID
> +NestedInterruptSelfTest (
> + IN NESTED_INTERRUPT_STATE *IsrState
> + )
> +{
> + UINTN SelfTestCount;
> + UINTN TimeOut;
Did this pass the uncrustify check? I think uncrustify would insist on
inserting two spaces here.
(
For running uncrustify locally:
- clone
<https://projectmu@dev.azure.com/projectmu/Uncrustify/_git/Uncrustify>
- check it out at tag 73.0.8 (the tag that edk2 CI uses on github is in
".pytool/Plugin/UncrustifyCheck/uncrustify_ext_dep.yaml")
- build it (IIRC it uses cmake)
- with nothing dirty in the working tree (i.e., everything committed, or
at least stashed to the index), run
uncrustify \
-c .pytool/Plugin/UncrustifyCheck/uncrustify.cfg \
--replace \
--no-backup \
--if-changed \
-F file-list.txt
)
> +
> + //
> + // Record number of this self-test for debug messages.
> + //
> + SelfTestCount = IsrState->SelfTestCount;
> +
> + //
> + // Re-enable interrupts and stall for up to one second to induce at
> + // least one more timer interrupt.
> + //
> + // This mimics the effect of an interrupt having occurred precisely
> + // at the end of our call to RestoreTPL(), with interrupts having
> + // been re-enabled by RestoreTPL() and with the interrupt happening
> + // to occur after the TPL has already been lowered back down to
> + // InterruptedTPL. (This is the scenario that can lead to stack
> + // exhaustion, as described above.)
> + //
> + ASSERT (GetInterruptState () == FALSE);
> + ASSERT (IsrState->DeferredRestoreTPL == FALSE);
> + EnableInterrupts();
I think uncrustify would want to insert a space here too
> + for (TimeOut = 0; TimeOut < 1000; TimeOut++) {
> + //
> + // Stall for 1ms
> + //
> + gBS->Stall (1000);
> +
> + //
> + // If we observe that interrupts have been spontaneously disabled,
> + // then this must be because the induced interrupt handler's call
> + // to NestedInterruptRestoreTPL() correctly chose to defer the
> + // RestoreTPL() call to the outer handler (i.e. to us).
> + //
> + if (GetInterruptState() == FALSE) {
> + ASSERT (IsrState->DeferredRestoreTPL == TRUE);
> + DEBUG ((
> + DEBUG_INFO,
> + "Nested interrupt self-test %u/%u passed\n",
> + SelfTestCount,
> + NUMBER_OF_SELF_TESTS
> + ));
So SelfTestCount may be a UINT64. The central PrintLib instance does not
support a conversion specifier for UINTN, only for UINT32 and UINT64
explicitly. Therefore the best way to print a UINTN is to cast the value
to fixed UINT64, and then print that with %Lu. However, in this case, we
can work in the opposite direction: change the type of SelfTestCount to
UINT32 (and then %u will be fine). Which is what I propose at the top.
%u is already fine for NUMBER_OF_SELF_TESTS (which is a FixedPcdGet32).
> + return;
> + }
> + }
> +
> + //
> + // The test has failed and we will halt the system. Disable
> + // interrupts now so that any test-induced interrupt storm does not
> + // prevent the fatal error messages from being displayed correctly.
> + //
> + DisableInterrupts();
> +
> + //
> + // If we observe that DeferredRestoreTPL is TRUE then this indicates
> + // that an interrupt did occur and NestedInterruptRestoreTPL() chose
> + // to defer the RestoreTPL() call to the outer handler, but that
> + // DisableInterruptsOnIret() failed to cause interrupts to be
> + // disabled after the IRET or equivalent instruction.
> + //
> + // This error represents a bug in the architecture-specific
> + // implementation of DisableInterruptsOnIret().
> + //
> + if (IsrState->DeferredRestoreTPL == TRUE) {
> + DEBUG ((
> + DEBUG_ERROR,
> + "Nested interrupt self-test %u/%u failed: interrupts still enabled\n",
> + SelfTestCount,
> + NUMBER_OF_SELF_TESTS
> + ));
> + ASSERT (FALSE);
> + }
> +
> + //
> + // If no timer interrupt occurred then this indicates that the timer
> + // interrupt handler failed to rearm the timer before calling
> + // NestedInterruptRestoreTPL(). This would prevent nested
> + // interrupts from occurring at all, which would break
> + // e.g. callbacks at TPL_CALLBACK that themselves wait on further
> + // timer events.
> + //
> + // This error represents a bug in the platform-specific timer
> + // interrupt handler.
> + //
> + DEBUG ((
> + DEBUG_ERROR,
> + "Nested interrupt self-test %u/%u failed: no nested interrupt\n",
> + SelfTestCount,
> + NUMBER_OF_SELF_TESTS
> + ));
> + ASSERT (FALSE);
> +}
I'd prefer something stronger than just ASSERT (FALSE) here, but -- per
previous discussion -- we don't have a generally accepted "panic" API
yet, and CpuDeadLoop() is not suitable for all platforms, so this should do.
With my trivial comments addressed:
Acked-by: Laszlo Ersek <lersek@redhat.com>
Comment on the general idea: I much like that the self-test is active on
every boot (without high costs).
Side idea: technically we could merge the first two patches in
separation (pending MdeModulePkg maintainer approval), and then consider
the last three patches as new improvements (possibly needing longer
review). This kind of splitting has both advantages and disadvantages;
the advantage is that the code movement / upstreaming to MdeModulePkg is
not blocked by (somewhat) unrelated discussion. The disadvantages are
that more admin work is needed (more posting, and more PRs), and that
patches in the series that one might consider to belong together will
fly apart in the git history. So I just figured I'd raise the option.
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114219): https://edk2.groups.io/g/devel/message/114219
Mute This Topic: https://groups.io/mt/103911608/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [edk2-devel] [PATCH v3 4/5] MdeModulePkg: Add self-tests for NestedInterruptTplLib
2024-01-23 16:55 ` Laszlo Ersek
@ 2024-01-23 17:41 ` Michael Brown
2024-01-24 12:58 ` Laszlo Ersek
0 siblings, 1 reply; 19+ messages in thread
From: Michael Brown @ 2024-01-23 17:41 UTC (permalink / raw)
To: devel, lersek; +Cc: Ray Ni, Gerd Hoffmann
On 23/01/2024 16:55, Laszlo Ersek wrote:
>> + ///
>> + /// Number of self-tests performed.
>> + ///
>> + UINTN SelfTestCount;
>> } NESTED_INTERRUPT_STATE;
>>
>> /**
>
> I suggest that the new field be UINT32. The (exclusive) limit is a
> 32-bit PCD. Making the counter (potentially) wider than the limit is not
> useful, but it's also a bit of a complication for the debug messages
> (see below).
Great suggestion, thanks!
>> + ///
>> + /// Perform a limited number of self-tests on the first few
>> + /// invocations.
>> + ///
>> + if ((IsrState->DeferredRestoreTPL == FALSE) &&
>
> This comment applies to several locations in the patch:
>
> BOOLEANs should not be checked using explicit "== TRUE" and "== FALSE"
> operators / comparisons; they should only be evaluated in their logical
> contexts:
We've had this conversation before :)
https://edk2.groups.io/g/devel/message/104369
I personally find that the "!" operators get visually lost in the EDK2
coding style with its very long variable and function names. That said,
I'm happy to omit all of the explicit comparisons, but I should then add
a precursor patch that changes the existing code style first. Thoughts?
>> +VOID
>> +NestedInterruptSelfTest (
>> + IN NESTED_INTERRUPT_STATE *IsrState
>> + )
>> +{
>> + UINTN SelfTestCount;
>> + UINTN TimeOut;
>
> Did this pass the uncrustify check? I think uncrustify would insist on
> inserting two spaces here.
>
> For running uncrustify locally:
You are right, and thank you for reminding me how to run the EDK2
version of uncrustify. I've fixed up everything it identified.
>> + // This error represents a bug in the platform-specific timer
>> + // interrupt handler.
>> + //
>> + DEBUG ((
>> + DEBUG_ERROR,
>> + "Nested interrupt self-test %u/%u failed: no nested interrupt\n",
>> + SelfTestCount,
>> + NUMBER_OF_SELF_TESTS
>> + ));
>> + ASSERT (FALSE);
>> +}
>
> I'd prefer something stronger than just ASSERT (FALSE) here, but -- per
> previous discussion -- we don't have a generally accepted "panic" API
> yet, and CpuDeadLoop() is not suitable for all platforms, so this should do.
>
> With my trivial comments addressed:
>
> Acked-by: Laszlo Ersek <lersek@redhat.com>
>
> Comment on the general idea: I much like that the self-test is active on
> every boot (without high costs).
Thank you!
> Side idea: technically we could merge the first two patches in
> separation (pending MdeModulePkg maintainer approval), and then consider
> the last three patches as new improvements (possibly needing longer
> review). This kind of splitting has both advantages and disadvantages;
> the advantage is that the code movement / upstreaming to MdeModulePkg is
> not blocked by (somewhat) unrelated discussion. The disadvantages are
> that more admin work is needed (more posting, and more PRs), and that
> patches in the series that one might consider to belong together will
> fly apart in the git history. So I just figured I'd raise the option.
I'm happy to work either way, and shall await instruction.
In the absence of any instruction to the contrary, I'll send out a v4
tomorrow with everyone's suggestions and tags included, but without the
above-mentioned precursor patch to remove the boolean comparisons.
Thanks,
Michael
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114227): https://edk2.groups.io/g/devel/message/114227
Mute This Topic: https://groups.io/mt/103911608/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [edk2-devel] [PATCH v3 4/5] MdeModulePkg: Add self-tests for NestedInterruptTplLib
2024-01-23 17:41 ` Michael Brown
@ 2024-01-24 12:58 ` Laszlo Ersek
0 siblings, 0 replies; 19+ messages in thread
From: Laszlo Ersek @ 2024-01-24 12:58 UTC (permalink / raw)
To: Michael Brown, devel; +Cc: Ray Ni, Gerd Hoffmann
On 1/23/24 18:41, Michael Brown wrote:
> On 23/01/2024 16:55, Laszlo Ersek wrote:
>>> + ///
>>> + /// Number of self-tests performed.
>>> + ///
>>> + UINTN SelfTestCount;
>>> } NESTED_INTERRUPT_STATE;
>>> /**
>>
>> I suggest that the new field be UINT32. The (exclusive) limit is a
>> 32-bit PCD. Making the counter (potentially) wider than the limit is not
>> useful, but it's also a bit of a complication for the debug messages
>> (see below).
>
> Great suggestion, thanks!
>
>>> + ///
>>> + /// Perform a limited number of self-tests on the first few
>>> + /// invocations.
>>> + ///
>>> + if ((IsrState->DeferredRestoreTPL == FALSE) &&
>>
>> This comment applies to several locations in the patch:
>>
>> BOOLEANs should not be checked using explicit "== TRUE" and "== FALSE"
>> operators / comparisons; they should only be evaluated in their logical
>> contexts:
>
> We've had this conversation before :)
>
> https://edk2.groups.io/g/devel/message/104369
Oops, sorry :)
>
> I personally find that the "!" operators get visually lost in the EDK2
> coding style with its very long variable and function names. That said,
> I'm happy to omit all of the explicit comparisons, but I should then add
> a precursor patch that changes the existing code style first. Thoughts?
I'm unsure.
Some people prefer (! Expression) over (!Expression), i.e., the extra
space, but I'm unsure if uncrustify tolerates that, and whether it
matches the edk2 coding style, regardless of uncrustify. How about:
!(Expression)
such as
if (!(IsrState->DeferredRestoreTPL) &&
? Does that make it more readable? (I believe this form would not trip
up uncrustify, and that it would satisfy the coding style too.)
With none of those working, the explicit ==TRUE / ==FALSE would not
"break" the coding style any stronger either, so in that case, feel free
(from my side) to just stick with the syntax in the patch (and in the
pre-patch code).
>
>>> +VOID
>>> +NestedInterruptSelfTest (
>>> + IN NESTED_INTERRUPT_STATE *IsrState
>>> + )
>>> +{
>>> + UINTN SelfTestCount;
>>> + UINTN TimeOut;
>>
>> Did this pass the uncrustify check? I think uncrustify would insist on
>> inserting two spaces here.
>>
>> For running uncrustify locally:
>
> You are right, and thank you for reminding me how to run the EDK2
> version of uncrustify. I've fixed up everything it identified.
>
>>> + // This error represents a bug in the platform-specific timer
>>> + // interrupt handler.
>>> + //
>>> + DEBUG ((
>>> + DEBUG_ERROR,
>>> + "Nested interrupt self-test %u/%u failed: no nested interrupt\n",
>>> + SelfTestCount,
>>> + NUMBER_OF_SELF_TESTS
>>> + ));
>>> + ASSERT (FALSE);
>>> +}
>>
>> I'd prefer something stronger than just ASSERT (FALSE) here, but -- per
>> previous discussion -- we don't have a generally accepted "panic" API
>> yet, and CpuDeadLoop() is not suitable for all platforms, so this
>> should do.
>>
>> With my trivial comments addressed:
>>
>> Acked-by: Laszlo Ersek <lersek@redhat.com>
>>
>> Comment on the general idea: I much like that the self-test is active on
>> every boot (without high costs).
>
> Thank you!
>
>> Side idea: technically we could merge the first two patches in
>> separation (pending MdeModulePkg maintainer approval), and then consider
>> the last three patches as new improvements (possibly needing longer
>> review). This kind of splitting has both advantages and disadvantages;
>> the advantage is that the code movement / upstreaming to MdeModulePkg is
>> not blocked by (somewhat) unrelated discussion. The disadvantages are
>> that more admin work is needed (more posting, and more PRs), and that
>> patches in the series that one might consider to belong together will
>> fly apart in the git history. So I just figured I'd raise the option.
>
> I'm happy to work either way, and shall await instruction.
>
> In the absence of any instruction to the contrary, I'll send out a v4
> tomorrow with everyone's suggestions and tags included, but without the
> above-mentioned precursor patch to remove the boolean comparisons.
A v4 like that sounds good (as far as I'm aware, the first two patches
have no maintainer review yet, anyway).
Thanks!
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114289): https://edk2.groups.io/g/devel/message/114289
Mute This Topic: https://groups.io/mt/103911608/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [edk2-devel] [PATCH v3 4/5] MdeModulePkg: Add self-tests for NestedInterruptTplLib
2024-01-23 15:31 ` [edk2-devel] [PATCH v3 4/5] MdeModulePkg: Add self-tests for NestedInterruptTplLib Michael Brown
2024-01-23 16:55 ` Laszlo Ersek
@ 2024-01-24 10:24 ` Ni, Ray
2024-01-24 10:26 ` Ni, Ray
1 sibling, 1 reply; 19+ messages in thread
From: Ni, Ray @ 2024-01-24 10:24 UTC (permalink / raw)
To: Michael Brown, devel@edk2.groups.io; +Cc: Gerd Hoffmann, Laszlo Ersek
> +//
> +// Number of self-tests to perform.
> +//
> +#define NUMBER_OF_SELF_TESTS \
> + (FixedPcdGet32 (PcdNestedInterruptNumberOfSelfTests))
1. can we avoid defining a PCD? I do not see a need that each platform needs
to use a different count. How about just define it as 1?
> +
> +STATIC
> +VOID
> +NestedInterruptSelfTest (
> + IN NESTED_INTERRUPT_STATE *IsrState
> + );
> +
> /**
> Raise the task priority level to TPL_HIGH_LEVEL.
>
> @@ -211,6 +223,16 @@ NestedInterruptRestoreTPL (
> //
> DisableInterrupts ();
>
> + ///
> + /// Perform a limited number of self-tests on the first few
> + /// invocations.
2. the comment could mention that the self-test only is performed when
no nested interrupt happens in gBS->RestoreTpl() call in above.
> + ///
> + if ((IsrState->DeferredRestoreTPL == FALSE) &&
> + (IsrState->SelfTestCount < NUMBER_OF_SELF_TESTS)) {
3. might have coding style issue.
> + //
> + // The test has failed and we will halt the system. Disable
> + // interrupts now so that any test-induced interrupt storm does not
> + // prevent the fatal error messages from being displayed correctly.
> + //
4. The comment might be wrong. It does not indicate the test has failed.
It's possible that the timer period is very long that causes no timer interrupt happens till now.
In that case, IsrState->DeferredRestoreTPL is FALSE. That's not an issue IMO.
Or, how about we stall for ever to wait for the timer interrupt instead of waiting just 1 second.
We could wait for the IsrState->DeferredRestoreTPL is TRUE.
> + DisableInterrupts();
> +
> + //
> + // If we observe that DeferredRestoreTPL is TRUE then this indicates
> + // that an interrupt did occur and NestedInterruptRestoreTPL() chose
> + // to defer the RestoreTPL() call to the outer handler, but that
> + // DisableInterruptsOnIret() failed to cause interrupts to be
> + // disabled after the IRET or equivalent instruction.
5. The comment "failed to cause interrupts to be disabled after IRET" can be inside the if-condition.
> + //
> + // This error represents a bug in the architecture-specific
> + // implementation of DisableInterruptsOnIret().
> + //
> + if (IsrState->DeferredRestoreTPL == TRUE) {
> + DEBUG ((
> + DEBUG_ERROR,
> + "Nested interrupt self-test %u/%u failed: interrupts still enabled\n",
> + SelfTestCount,
> + NUMBER_OF_SELF_TESTS
> + ));
> + ASSERT (FALSE);
> + }
> +
> + //
> + // If no timer interrupt occurred then this indicates that the timer
> + // interrupt handler failed to rearm the timer before calling
> + // NestedInterruptRestoreTPL(). This would prevent nested
> + // interrupts from occurring at all, which would break
> + // e.g. callbacks at TPL_CALLBACK that themselves wait on further
> + // timer events.
> + //
> + // This error represents a bug in the platform-specific timer
> + // interrupt handler.
> + //
> + DEBUG ((
> + DEBUG_ERROR,
> + "Nested interrupt self-test %u/%u failed: no nested interrupt\n",
> + SelfTestCount,
> + NUMBER_OF_SELF_TESTS
> + ));
> + ASSERT (FALSE);
6. If we change the above code to wait forever until the DeferredRestoreTPL is TRUE,
the above debug message and ASSERT() can be removed. Agree?
> +}
> --
> 2.43.0
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114272): https://edk2.groups.io/g/devel/message/114272
Mute This Topic: https://groups.io/mt/103911608/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [edk2-devel] [PATCH v3 4/5] MdeModulePkg: Add self-tests for NestedInterruptTplLib
2024-01-24 10:24 ` Ni, Ray
@ 2024-01-24 10:26 ` Ni, Ray
0 siblings, 0 replies; 19+ messages in thread
From: Ni, Ray @ 2024-01-24 10:26 UTC (permalink / raw)
To: Michael Brown, devel@edk2.groups.io; +Cc: Gerd Hoffmann, Laszlo Ersek
Thank you for adding the self-test code!
Thanks,
Ray
> -----Original Message-----
> From: Ni, Ray
> Sent: Wednesday, January 24, 2024 6:25 PM
> To: Michael Brown <mcb30@ipxe.org>; devel@edk2.groups.io
> Cc: Gerd Hoffmann <kraxel@redhat.com>; Laszlo Ersek <lersek@redhat.com>
> Subject: RE: [PATCH v3 4/5] MdeModulePkg: Add self-tests for
> NestedInterruptTplLib
>
> > +//
> > +// Number of self-tests to perform.
> > +//
> > +#define NUMBER_OF_SELF_TESTS \
> > + (FixedPcdGet32 (PcdNestedInterruptNumberOfSelfTests))
>
> 1. can we avoid defining a PCD? I do not see a need that each platform needs
> to use a different count. How about just define it as 1?
>
> > +
> > +STATIC
> > +VOID
> > +NestedInterruptSelfTest (
> > + IN NESTED_INTERRUPT_STATE *IsrState
> > + );
> > +
> > /**
> > Raise the task priority level to TPL_HIGH_LEVEL.
> >
> > @@ -211,6 +223,16 @@ NestedInterruptRestoreTPL (
> > //
> > DisableInterrupts ();
> >
> > + ///
> > + /// Perform a limited number of self-tests on the first few
> > + /// invocations.
>
> 2. the comment could mention that the self-test only is performed when
> no nested interrupt happens in gBS->RestoreTpl() call in above.
>
> > + ///
> > + if ((IsrState->DeferredRestoreTPL == FALSE) &&
> > + (IsrState->SelfTestCount < NUMBER_OF_SELF_TESTS)) {
>
> 3. might have coding style issue.
>
> > + //
> > + // The test has failed and we will halt the system. Disable
> > + // interrupts now so that any test-induced interrupt storm does not
> > + // prevent the fatal error messages from being displayed correctly.
> > + //
>
> 4. The comment might be wrong. It does not indicate the test has failed.
> It's possible that the timer period is very long that causes no timer interrupt
> happens till now.
> In that case, IsrState->DeferredRestoreTPL is FALSE. That's not an issue IMO.
>
> Or, how about we stall for ever to wait for the timer interrupt instead of
> waiting just 1 second.
> We could wait for the IsrState->DeferredRestoreTPL is TRUE.
>
> > + DisableInterrupts();
> > +
> > + //
> > + // If we observe that DeferredRestoreTPL is TRUE then this indicates
> > + // that an interrupt did occur and NestedInterruptRestoreTPL() chose
> > + // to defer the RestoreTPL() call to the outer handler, but that
> > + // DisableInterruptsOnIret() failed to cause interrupts to be
> > + // disabled after the IRET or equivalent instruction.
>
> 5. The comment "failed to cause interrupts to be disabled after IRET" can be
> inside the if-condition.
>
> > + //
> > + // This error represents a bug in the architecture-specific
> > + // implementation of DisableInterruptsOnIret().
> > + //
> > + if (IsrState->DeferredRestoreTPL == TRUE) {
> > + DEBUG ((
> > + DEBUG_ERROR,
> > + "Nested interrupt self-test %u/%u failed: interrupts still
> enabled\n",
> > + SelfTestCount,
> > + NUMBER_OF_SELF_TESTS
> > + ));
> > + ASSERT (FALSE);
> > + }
> > +
>
>
> > + //
> > + // If no timer interrupt occurred then this indicates that the timer
> > + // interrupt handler failed to rearm the timer before calling
> > + // NestedInterruptRestoreTPL(). This would prevent nested
> > + // interrupts from occurring at all, which would break
> > + // e.g. callbacks at TPL_CALLBACK that themselves wait on further
> > + // timer events.
> > + //
> > + // This error represents a bug in the platform-specific timer
> > + // interrupt handler.
> > + //
> > + DEBUG ((
> > + DEBUG_ERROR,
> > + "Nested interrupt self-test %u/%u failed: no nested interrupt\n",
> > + SelfTestCount,
> > + NUMBER_OF_SELF_TESTS
> > + ));
> > + ASSERT (FALSE);
>
> 6. If we change the above code to wait forever until the DeferredRestoreTPL
> is TRUE,
> the above debug message and ASSERT() can be removed. Agree?
>
> > +}
> > --
> > 2.43.0
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114273): https://edk2.groups.io/g/devel/message/114273
Mute This Topic: https://groups.io/mt/103911608/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 19+ messages in thread
* [edk2-devel] [PATCH v3 5/5] MdeModulePkg: Extend NestedInterruptTplLib to support Arm CPUs
[not found] ` <20240123153104.2451759-1-mcb30@ipxe.org>
` (3 preceding siblings ...)
2024-01-23 15:31 ` [edk2-devel] [PATCH v3 4/5] MdeModulePkg: Add self-tests for NestedInterruptTplLib Michael Brown
@ 2024-01-23 15:31 ` Michael Brown
2024-01-23 15:51 ` Ard Biesheuvel
2024-01-23 17:10 ` Laszlo Ersek
4 siblings, 2 replies; 19+ messages in thread
From: Michael Brown @ 2024-01-23 15:31 UTC (permalink / raw)
To: devel; +Cc: Ray Ni, Gerd Hoffmann, Laszlo Ersek, Michael Brown,
Ard Biesheuvel
The only architecture-specific portion of NestedInterruptTplLib is in
Iret.c, which must manipulate the interrupt stack frame such that the
return-from-interrupt instruction will not re-enable interrupts. The
remaining logic in Tpl.c is architecture-agnostic.
Add implementations of DisableInterruptsOnIret() for MDE_CPU_ARM and
MDE_CPU_AARCH64. In both cases, the saved IRQs-disabled and
FIQs-disabled flags are set in the stored processor status register
(matching the behaviour of DisableInterrupts(), which also sets both
flags).
Tested by patching ArmPkg's TimerDxe to use NestedInterruptTplLib and
verifying that ArmVirtQemu passes the NestedInterruptTplLib self-tests
for both ARM and AARCH64.
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Signed-off-by: Michael Brown <mcb30@ipxe.org>
---
MdeModulePkg/MdeModulePkg.dsc | 2 +-
.../NestedInterruptTplLib.inf | 3 +++
.../Library/NestedInterruptTplLib/Iret.c | 18 ++++++++++++++++++
3 files changed, 22 insertions(+), 1 deletion(-)
diff --git a/MdeModulePkg/MdeModulePkg.dsc b/MdeModulePkg/MdeModulePkg.dsc
index 5b9ddfd26e75..4565b8e1b6e7 100644
--- a/MdeModulePkg/MdeModulePkg.dsc
+++ b/MdeModulePkg/MdeModulePkg.dsc
@@ -462,6 +462,7 @@ [Components.IA32, Components.X64, Components.AARCH64]
[Components.IA32, Components.X64, Components.ARM, Components.AARCH64]
MdeModulePkg/Library/BrotliCustomDecompressLib/BrotliCustomDecompressLib.inf
+ MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
MdeModulePkg/Library/LzmaCustomDecompressLib/LzmaCustomDecompressLib.inf
MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
MdeModulePkg/Core/Dxe/DxeMain.inf {
@@ -526,7 +527,6 @@ [Components.IA32, Components.X64]
MdeModulePkg/Library/TraceHubDebugSysTLib/BaseTraceHubDebugSysTLib.inf
MdeModulePkg/Library/TraceHubDebugSysTLib/PeiTraceHubDebugSysTLib.inf
MdeModulePkg/Library/TraceHubDebugSysTLib/DxeSmmTraceHubDebugSysTLib.inf
- MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
[Components.X64]
MdeModulePkg/Universal/CapsulePei/CapsuleX64.inf
diff --git a/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf b/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
index e67d899b9446..1501f067d77f 100644
--- a/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
+++ b/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
@@ -27,6 +27,9 @@ [Packages]
MdePkg/MdePkg.dec
MdeModulePkg/MdeModulePkg.dec
+[Packages.ARM, Packages.AARCH64]
+ ArmPkg/ArmPkg.dec
+
[LibraryClasses]
BaseLib
DebugLib
diff --git a/MdeModulePkg/Library/NestedInterruptTplLib/Iret.c b/MdeModulePkg/Library/NestedInterruptTplLib/Iret.c
index f6b2c51b6cc1..87cb74566730 100644
--- a/MdeModulePkg/Library/NestedInterruptTplLib/Iret.c
+++ b/MdeModulePkg/Library/NestedInterruptTplLib/Iret.c
@@ -9,6 +9,10 @@
#include <Library/BaseLib.h>
#include <Library/DebugLib.h>
+#if defined (MDE_CPU_AARCH64) || defined (MDE_CPU_ARM)
+#include <Library/ArmLib.h>
+#endif
+
#include "Iret.h"
/**
@@ -54,6 +58,20 @@ DisableInterruptsOnIret (
Eflags.Bits.IF = 0;
SystemContext.SystemContextIa32->Eflags = Eflags.UintN;
+ #elif defined (MDE_CPU_AARCH64)
+
+ //
+ // Set IRQ-disabled and FIQ-disabled flags.
+ //
+ SystemContext.SystemContextAArch64->SPSR |= (SPSR_I | SPSR_F);
+
+ #elif defined (MDE_CPU_ARM)
+
+ //
+ // Set IRQ-disabled and FIQ-disabled flags.
+ //
+ SystemContext.SystemContextArm->CPSR |= (CPSR_IRQ | CPSR_FIQ);
+
#else
#error "Unsupported CPU"
--
2.43.0
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114211): https://edk2.groups.io/g/devel/message/114211
Mute This Topic: https://groups.io/mt/103911611/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [edk2-devel] [PATCH v3 5/5] MdeModulePkg: Extend NestedInterruptTplLib to support Arm CPUs
2024-01-23 15:31 ` [edk2-devel] [PATCH v3 5/5] MdeModulePkg: Extend NestedInterruptTplLib to support Arm CPUs Michael Brown
@ 2024-01-23 15:51 ` Ard Biesheuvel
2024-01-23 16:48 ` Michael Brown
2024-01-23 17:10 ` Laszlo Ersek
1 sibling, 1 reply; 19+ messages in thread
From: Ard Biesheuvel @ 2024-01-23 15:51 UTC (permalink / raw)
To: Michael Brown; +Cc: devel, Ray Ni, Gerd Hoffmann, Laszlo Ersek
On Tue, 23 Jan 2024 at 16:31, Michael Brown <mcb30@ipxe.org> wrote:
>
> The only architecture-specific portion of NestedInterruptTplLib is in
> Iret.c, which must manipulate the interrupt stack frame such that the
> return-from-interrupt instruction will not re-enable interrupts. The
> remaining logic in Tpl.c is architecture-agnostic.
>
> Add implementations of DisableInterruptsOnIret() for MDE_CPU_ARM and
> MDE_CPU_AARCH64. In both cases, the saved IRQs-disabled and
> FIQs-disabled flags are set in the stored processor status register
> (matching the behaviour of DisableInterrupts(), which also sets both
> flags).
>
> Tested by patching ArmPkg's TimerDxe to use NestedInterruptTplLib and
> verifying that ArmVirtQemu passes the NestedInterruptTplLib self-tests
> for both ARM and AARCH64.
>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Signed-off-by: Michael Brown <mcb30@ipxe.org>
Thanks for this.
You can drop the FIQ bits, though: anything that can run Tianocore
will have the FIQs routed to the secure world, and all of the higher
level en/disable interrupt code only reasons about the IRQ line.
With that,
Acked-by: Ard Biesheuvel <ardb@kernel.org>
> ---
> MdeModulePkg/MdeModulePkg.dsc | 2 +-
> .../NestedInterruptTplLib.inf | 3 +++
> .../Library/NestedInterruptTplLib/Iret.c | 18 ++++++++++++++++++
> 3 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/MdeModulePkg/MdeModulePkg.dsc b/MdeModulePkg/MdeModulePkg.dsc
> index 5b9ddfd26e75..4565b8e1b6e7 100644
> --- a/MdeModulePkg/MdeModulePkg.dsc
> +++ b/MdeModulePkg/MdeModulePkg.dsc
> @@ -462,6 +462,7 @@ [Components.IA32, Components.X64, Components.AARCH64]
>
> [Components.IA32, Components.X64, Components.ARM, Components.AARCH64]
> MdeModulePkg/Library/BrotliCustomDecompressLib/BrotliCustomDecompressLib.inf
> + MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
> MdeModulePkg/Library/LzmaCustomDecompressLib/LzmaCustomDecompressLib.inf
> MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
> MdeModulePkg/Core/Dxe/DxeMain.inf {
> @@ -526,7 +527,6 @@ [Components.IA32, Components.X64]
> MdeModulePkg/Library/TraceHubDebugSysTLib/BaseTraceHubDebugSysTLib.inf
> MdeModulePkg/Library/TraceHubDebugSysTLib/PeiTraceHubDebugSysTLib.inf
> MdeModulePkg/Library/TraceHubDebugSysTLib/DxeSmmTraceHubDebugSysTLib.inf
> - MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
>
> [Components.X64]
> MdeModulePkg/Universal/CapsulePei/CapsuleX64.inf
> diff --git a/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf b/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
> index e67d899b9446..1501f067d77f 100644
> --- a/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
> +++ b/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
> @@ -27,6 +27,9 @@ [Packages]
> MdePkg/MdePkg.dec
> MdeModulePkg/MdeModulePkg.dec
>
> +[Packages.ARM, Packages.AARCH64]
> + ArmPkg/ArmPkg.dec
> +
> [LibraryClasses]
> BaseLib
> DebugLib
> diff --git a/MdeModulePkg/Library/NestedInterruptTplLib/Iret.c b/MdeModulePkg/Library/NestedInterruptTplLib/Iret.c
> index f6b2c51b6cc1..87cb74566730 100644
> --- a/MdeModulePkg/Library/NestedInterruptTplLib/Iret.c
> +++ b/MdeModulePkg/Library/NestedInterruptTplLib/Iret.c
> @@ -9,6 +9,10 @@
> #include <Library/BaseLib.h>
> #include <Library/DebugLib.h>
>
> +#if defined (MDE_CPU_AARCH64) || defined (MDE_CPU_ARM)
> +#include <Library/ArmLib.h>
> +#endif
> +
> #include "Iret.h"
>
> /**
> @@ -54,6 +58,20 @@ DisableInterruptsOnIret (
> Eflags.Bits.IF = 0;
> SystemContext.SystemContextIa32->Eflags = Eflags.UintN;
>
> + #elif defined (MDE_CPU_AARCH64)
> +
> + //
> + // Set IRQ-disabled and FIQ-disabled flags.
> + //
> + SystemContext.SystemContextAArch64->SPSR |= (SPSR_I | SPSR_F);
> +
> + #elif defined (MDE_CPU_ARM)
> +
> + //
> + // Set IRQ-disabled and FIQ-disabled flags.
> + //
> + SystemContext.SystemContextArm->CPSR |= (CPSR_IRQ | CPSR_FIQ);
> +
> #else
>
> #error "Unsupported CPU"
> --
> 2.43.0
>
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114212): https://edk2.groups.io/g/devel/message/114212
Mute This Topic: https://groups.io/mt/103911611/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [edk2-devel] [PATCH v3 5/5] MdeModulePkg: Extend NestedInterruptTplLib to support Arm CPUs
2024-01-23 15:51 ` Ard Biesheuvel
@ 2024-01-23 16:48 ` Michael Brown
0 siblings, 0 replies; 19+ messages in thread
From: Michael Brown @ 2024-01-23 16:48 UTC (permalink / raw)
To: devel, ardb; +Cc: Ray Ni, Gerd Hoffmann, Laszlo Ersek
On 23/01/2024 15:51, Ard Biesheuvel wrote:
> On Tue, 23 Jan 2024 at 16:31, Michael Brown <mcb30@ipxe.org> wrote:
>> Add implementations of DisableInterruptsOnIret() for MDE_CPU_ARM and
>> MDE_CPU_AARCH64. In both cases, the saved IRQs-disabled and
>> FIQs-disabled flags are set in the stored processor status register
>> (matching the behaviour of DisableInterrupts(), which also sets both
>> flags).
>
> Thanks for this.
>
> You can drop the FIQ bits, though: anything that can run Tianocore
> will have the FIQs routed to the secure world, and all of the higher
> level en/disable interrupt code only reasons about the IRQ line.
Thank you.
My commit message was incorrect, sorry: DisableInterrupts() in MdePkg
does indeed disable only IRQs. I was accidentally looking at
ArmDisableInterrupts() in ArmPkg, which disables both IRQs and FIQs.
My (incomplete) understanding of Arm behaviour is that when an interrupt
is raised, both IRQs and FIQs will be disabled by hardware. When the
interrupt handler ends up enabling interrupts during RestoreTPL(), only
the IRQs will be re-enabled since EnableInterrupts() touches only the
IRQ bit. This state can persist for an extended period of time (e.g. 30
seconds) if a callback at TPL_CALLBACK ends up waiting for further timer
events.
Q1: Is it a problem to have this situation, with FIQs disabled for an
extended period of time?
Q2: What happens with FIQs when there is no secure world (e.g. using
ArmVirtQemu with "-M virt")?
Q3: Would you like me to add in the extra patch that modifies TimerDxe
to use NestedInterruptTplLib?
Many thanks,
Michael
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114217): https://edk2.groups.io/g/devel/message/114217
Mute This Topic: https://groups.io/mt/103911611/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [edk2-devel] [PATCH v3 5/5] MdeModulePkg: Extend NestedInterruptTplLib to support Arm CPUs
2024-01-23 15:31 ` [edk2-devel] [PATCH v3 5/5] MdeModulePkg: Extend NestedInterruptTplLib to support Arm CPUs Michael Brown
2024-01-23 15:51 ` Ard Biesheuvel
@ 2024-01-23 17:10 ` Laszlo Ersek
2024-01-23 17:21 ` Michael Brown
1 sibling, 1 reply; 19+ messages in thread
From: Laszlo Ersek @ 2024-01-23 17:10 UTC (permalink / raw)
To: devel, mcb30; +Cc: Ray Ni, Gerd Hoffmann, Ard Biesheuvel
On 1/23/24 16:31, Michael Brown wrote:
> The only architecture-specific portion of NestedInterruptTplLib is in
> Iret.c, which must manipulate the interrupt stack frame such that the
> return-from-interrupt instruction will not re-enable interrupts. The
> remaining logic in Tpl.c is architecture-agnostic.
>
> Add implementations of DisableInterruptsOnIret() for MDE_CPU_ARM and
> MDE_CPU_AARCH64. In both cases, the saved IRQs-disabled and
> FIQs-disabled flags are set in the stored processor status register
> (matching the behaviour of DisableInterrupts(), which also sets both
> flags).
>
> Tested by patching ArmPkg's TimerDxe to use NestedInterruptTplLib and
> verifying that ArmVirtQemu passes the NestedInterruptTplLib self-tests
> for both ARM and AARCH64.
>
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Signed-off-by: Michael Brown <mcb30@ipxe.org>
> ---
> MdeModulePkg/MdeModulePkg.dsc | 2 +-
> .../NestedInterruptTplLib.inf | 3 +++
> .../Library/NestedInterruptTplLib/Iret.c | 18 ++++++++++++++++++
> 3 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/MdeModulePkg/MdeModulePkg.dsc b/MdeModulePkg/MdeModulePkg.dsc
> index 5b9ddfd26e75..4565b8e1b6e7 100644
> --- a/MdeModulePkg/MdeModulePkg.dsc
> +++ b/MdeModulePkg/MdeModulePkg.dsc
> @@ -462,6 +462,7 @@ [Components.IA32, Components.X64, Components.AARCH64]
>
> [Components.IA32, Components.X64, Components.ARM, Components.AARCH64]
> MdeModulePkg/Library/BrotliCustomDecompressLib/BrotliCustomDecompressLib.inf
> + MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
> MdeModulePkg/Library/LzmaCustomDecompressLib/LzmaCustomDecompressLib.inf
> MdeModulePkg/Library/VarCheckUefiLib/VarCheckUefiLib.inf
> MdeModulePkg/Core/Dxe/DxeMain.inf {
> @@ -526,7 +527,6 @@ [Components.IA32, Components.X64]
> MdeModulePkg/Library/TraceHubDebugSysTLib/BaseTraceHubDebugSysTLib.inf
> MdeModulePkg/Library/TraceHubDebugSysTLib/PeiTraceHubDebugSysTLib.inf
> MdeModulePkg/Library/TraceHubDebugSysTLib/DxeSmmTraceHubDebugSysTLib.inf
> - MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
>
> [Components.X64]
> MdeModulePkg/Universal/CapsulePei/CapsuleX64.inf
> diff --git a/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf b/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
> index e67d899b9446..1501f067d77f 100644
> --- a/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
> +++ b/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
> @@ -27,6 +27,9 @@ [Packages]
> MdePkg/MdePkg.dec
> MdeModulePkg/MdeModulePkg.dec
>
> +[Packages.ARM, Packages.AARCH64]
> + ArmPkg/ArmPkg.dec
> +
> [LibraryClasses]
> BaseLib
> DebugLib
> diff --git a/MdeModulePkg/Library/NestedInterruptTplLib/Iret.c b/MdeModulePkg/Library/NestedInterruptTplLib/Iret.c
> index f6b2c51b6cc1..87cb74566730 100644
> --- a/MdeModulePkg/Library/NestedInterruptTplLib/Iret.c
> +++ b/MdeModulePkg/Library/NestedInterruptTplLib/Iret.c
> @@ -9,6 +9,10 @@
> #include <Library/BaseLib.h>
> #include <Library/DebugLib.h>
>
> +#if defined (MDE_CPU_AARCH64) || defined (MDE_CPU_ARM)
> +#include <Library/ArmLib.h>
> +#endif
> +
> #include "Iret.h"
>
> /**
> @@ -54,6 +58,20 @@ DisableInterruptsOnIret (
> Eflags.Bits.IF = 0;
> SystemContext.SystemContextIa32->Eflags = Eflags.UintN;
>
> + #elif defined (MDE_CPU_AARCH64)
> +
> + //
> + // Set IRQ-disabled and FIQ-disabled flags.
> + //
> + SystemContext.SystemContextAArch64->SPSR |= (SPSR_I | SPSR_F);
> +
> + #elif defined (MDE_CPU_ARM)
> +
> + //
> + // Set IRQ-disabled and FIQ-disabled flags.
> + //
> + SystemContext.SystemContextArm->CPSR |= (CPSR_IRQ | CPSR_FIQ);
> +
> #else
>
> #error "Unsupported CPU"
I can't comment on the register massaging.
Other than that, the patch looks great to me; I'd only propose one
(superficial) improvement:
rather than include <Library/ArmLib.h>, scavenge
#ifdef MDE_CPU_ARM
#include <Chipset/ArmV7.h>
#elif defined (MDE_CPU_AARCH64)
#include <Chipset/AArch64.h>
#endif
from it.
Reasons:
(a) Those are the headers that directly provide the macros we need, no
need to include the rest of ArmLib.h. (Listing ArmPkg/ArmPkg.dec in the
Packages section of the INF file will make these more direct #include
directives work, too.)
(b) Including <Library/ArmLib.h> kind of de-synchronizes the #include
directives in the C source from the [LibraryClasses] section in the INF
file. Generally there should be a 1-to-1 match -- we should include the
declarations of variables and functions for exactly those libraries that
we link against. There are two exceptions (that I can think of at once):
when we only want macros from a lib class header, or when we include a
lib class header because we are implementing an instance for that lib
class (i.e., we're providing, not consuming, the definitions for the
header-declared variables and functions). In this case, neither seems to
apply, this is not an ArmLib instance (= implementation), and the macros
we need don't actually come from ArmLib.h.
Acked-by: Laszlo Ersek <lersek@redhat.com>
Thanks
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114221): https://edk2.groups.io/g/devel/message/114221
Mute This Topic: https://groups.io/mt/103911611/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [edk2-devel] [PATCH v3 5/5] MdeModulePkg: Extend NestedInterruptTplLib to support Arm CPUs
2024-01-23 17:10 ` Laszlo Ersek
@ 2024-01-23 17:21 ` Michael Brown
0 siblings, 0 replies; 19+ messages in thread
From: Michael Brown @ 2024-01-23 17:21 UTC (permalink / raw)
To: devel, lersek; +Cc: Ray Ni, Gerd Hoffmann, Ard Biesheuvel
On 23/01/2024 17:10, Laszlo Ersek wrote:
> Other than that, the patch looks great to me; I'd only propose one
> (superficial) improvement:
>
> rather than include <Library/ArmLib.h>, scavenge
>
> #ifdef MDE_CPU_ARM
> #include <Chipset/ArmV7.h>
> #elif defined (MDE_CPU_AARCH64)
> #include <Chipset/AArch64.h>
> #endif
>
> from it.
>
> Reasons:
>
> (a) Those are the headers that directly provide the macros we need, no
> need to include the rest of ArmLib.h. (Listing ArmPkg/ArmPkg.dec in the
> Packages section of the INF file will make these more direct #include
> directives work, too.)
>
> (b) Including <Library/ArmLib.h> kind of de-synchronizes the #include
> directives in the C source from the [LibraryClasses] section in the INF
> file. Generally there should be a 1-to-1 match -- we should include the
> declarations of variables and functions for exactly those libraries that
> we link against. There are two exceptions (that I can think of at once):
> when we only want macros from a lib class header, or when we include a
> lib class header because we are implementing an instance for that lib
> class (i.e., we're providing, not consuming, the definitions for the
> header-declared variables and functions). In this case, neither seems to
> apply, this is not an ArmLib instance (= implementation), and the macros
> we need don't actually come from ArmLib.h.
>
> Acked-by: Laszlo Ersek <lersek@redhat.com>
Will do, thanks!
Michael
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114225): https://edk2.groups.io/g/devel/message/114225
Mute This Topic: https://groups.io/mt/103911611/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
^ permalink raw reply [flat|nested] 19+ messages in thread