public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
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]
-=-=-=-=-=-=-=-=-=-=-=-



  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