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+tianoc...@kernel.org>
> Signed-off-by: Michael Brown <mc...@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 <ler...@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/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe:
https://edk2.groups.io/g/devel/leave/9847357/21656/1706620634/xyzzy
[arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
