From: "Laszlo Ersek" <lersek@redhat.com>
To: devel@edk2.groups.io, mcb30@ipxe.org
Cc: Ray Ni <ray.ni@intel.com>, Gerd Hoffmann <kraxel@redhat.com>,
Michael D Kinney <michael.d.kinney@intel.com>
Subject: Re: [edk2-devel] [PATCH v2 1/2] MdeModulePkg: Move NestedInterruptTplLib to MdeModulePkg
Date: Mon, 22 Jan 2024 11:36:43 +0100 [thread overview]
Message-ID: <3c38a732-1a40-cace-91e6-49a7009e59c1@redhat.com> (raw)
In-Reply-To: <0102018d27d868d8-f4f6629f-cfed-4c14-ba31-f42bc9130efc-000000@eu-west-1.amazonses.com>
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.ni@intel.com>
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: Michael D Kinney <michael.d.kinney@intel.com>
> Signed-off-by: Michael Brown <mcb30@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 <lersek@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/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/leave/12367111/7686176/1913456212/xyzzy [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-
next prev parent reply other threads:[~2024-01-22 10:36 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <17AC1CE6B1CDE741.7395@groups.io>
2024-01-20 17:08 ` [edk2-devel] [PATCH v2 0/2] Move NestedInterruptTplLib to MdeModulePkg Michael Brown
[not found] ` <20240120170814.1777073-1-mcb30@ipxe.org>
2024-01-20 17:08 ` [edk2-devel] [PATCH v2 1/2] MdeModulePkg: " Michael Brown
2024-01-22 10:36 ` Laszlo Ersek [this message]
2024-01-23 14:10 ` Michael Brown
2024-01-20 17:08 ` [edk2-devel] [PATCH v2 2/2] MdeModulePkg: Add missing Iret.h to NestedInterruptTplLib sources list Michael Brown
2024-01-22 10:39 ` Laszlo Ersek
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-list from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3c38a732-1a40-cace-91e6-49a7009e59c1@redhat.com \
--to=devel@edk2.groups.io \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox