Re: [edk2-devel] [PATCH v3 5/5] MdeModulePkg: Extend NestedInterruptTplLib to support Arm CPUs

["Laszlo Ersek" <lersek@redhat.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]
-=-=-=-=-=-=-=-=-=-=-=-