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>,
	Ard Biesheuvel <ardb+tianocore@kernel.org>
Subject: Re: [edk2-devel] [PATCH v3 5/5] MdeModulePkg: Extend NestedInterruptTplLib to support Arm CPUs
Date: Tue, 23 Jan 2024 18:10:14 +0100	[thread overview]
Message-ID: <8150b1eb-a1cb-e5b2-d48c-13ad596f17a0@redhat.com> (raw)
In-Reply-To: <0102018d36f28a0d-300f8626-f368-49ab-b5e3-3c4edf84ce7e-000000@eu-west-1.amazonses.com>

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]
-=-=-=-=-=-=-=-=-=-=-=-



  parent reply	other threads:[~2024-01-23 17:10 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <17ACFF3FDD20CD9A.13754@groups.io>
2024-01-23 15:31 ` [edk2-devel] [PATCH v3 0/5] MdeModulePkg: Move NestedInterruptTplLib to MdeModulePkg Michael Brown
2024-01-25 12:17   ` Ni, Ray
     [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   ` [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
2024-01-24 12:52         ` Laszlo Ersek
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 12:58         ` Laszlo Ersek
2024-01-24 10:24     ` Ni, Ray
2024-01-24 10:26       ` Ni, Ray
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 [this message]
2024-01-23 17:21       ` Michael Brown

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=8150b1eb-a1cb-e5b2-d48c-13ad596f17a0@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