Two comments:
On 1/20/24 18:08, Michael Brown wrote:
> 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.
>
> Cc: Ray Ni <ray...@intel.com>
> Cc: Laszlo Ersek <ler...@redhat.com>
> Cc: Gerd Hoffmann <kra...@redhat.com>
> Cc: Michael D Kinney <michael.d.kin...@intel.com>
> Signed-off-by: Michael Brown <mc...@ipxe.org>
> ---
> MdeModulePkg/MdeModulePkg.dec | 4 ++++
> OvmfPkg/OvmfPkg.dec | 4 ----
> 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 | 1 +
> .../Include/Library/NestedInterruptTplLib.h | 0
> .../Library/NestedInterruptTplLib/Iret.h | 0
> .../Library/NestedInterruptTplLib/Iret.c | 0
> {OvmfPkg => MdeModulePkg}/Library/NestedInterruptTplLib/Tpl.c | 0
> 17 files changed, 15 insertions(+), 14 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/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..8a992764fb98 100644
> --- a/OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
> +++ b/OvmfPkg/LocalApicTimerDxe/LocalApicTimerDxe.inf
> @@ -20,6 +20,7 @@ [Defines]
>
> [Packages]
> MdePkg/MdePkg.dec
> + MdeModulePkg/MdeModulePkg.dec
> UefiCpuPkg/UefiCpuPkg.dec
> OvmfPkg/OvmfPkg.dec
>
(1) Does LocalApicTimerDxe have any other dependencies on OvmfPkg than
the library class header?
In other words, can't we remove "OvmfPkg/OvmfPkg.dec" from this
[Packages] section, too?
I can't see anything else, so I'd suggest to remove
"OvmfPkg/OvmfPkg.dec", but I could be missing something.
> 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) Core packages tend to differ from "leaf" packages in some ways; eg.:
- they don't build actual firmware platforms (so they don't have FDF
files),
- their DSC files list all possible components just for ensuring build
coverage -- not only driver and application binaries (executables), but
library instances too. (Well, for executables, it may not be *only*
"build coverage", as some of those *.efi binaries may well be executable
from a UEFI shell; but the basic idea is build coverage.)
Therefore, I suggest adding "NestedInterruptTplLib.inf" to
"MdeModulePkg.dsc". AIUI, right now the library instance is x86-only
(with a proposal on-list for the AARCH64 extension). Therefore, the
section in "MdeModulePkg.dsc" to add the INF file to would be:
[Components.IA32, Components.X64]
Within that section, library instances and drivers are apparently not
listed in lexicographical order, so maybe just tack the new line to the
bottom.
For build-testing, run the command
build \
-a IA32 -a X64 \
-b NOOPT \
-p MdeModulePkg/MdeModulePkg.dsc \
-t GCC5 \
-m MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf
(Feel free to use DEBUG or RELEASE rather than NOOPT; NOOPT should be
the fastest build. Also, change GCC5 to something else if necessary.)
With (1) and (2) considered / updated:
Reviewed-by: Laszlo Ersek <ler...@redhat.com>
Thanks,
Laszlo
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#114140): https://edk2.groups.io/g/devel/message/114140
Mute This Topic: https://groups.io/mt/103852900/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
