From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail02.groups.io (mail02.groups.io [66.175.222.108]) by spool.mail.gandi.net (Postfix) with ESMTPS id 9522094193C for ; Tue, 23 Jan 2024 17:10:23 +0000 (UTC) DKIM-Signature: a=rsa-sha256; bh=rAMQsyW7cM2yrQ8TMxmUnwp7sfHKEwFWEXRfs6SfX2E=; c=relaxed/simple; d=groups.io; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From:In-Reply-To:Precedence:List-Subscribe:List-Help:Sender:List-Id:Mailing-List:Delivered-To:Reply-To:List-Unsubscribe-Post:List-Unsubscribe:Content-Language:Content-Type:Content-Transfer-Encoding; s=20140610; t=1706029822; v=1; b=pxKZI4p+pAr/iQr6ldam1px5v2npI8fHfi+xKdCysdf/n8c/pK1L9lEJ8vhkesQ1EvYhEsRn pFrHBuWw0EEnnjkRomK0VAMCRbtvrJZGWKDpjM9ymncN9G9iO6UWnyCN6nh0GX9M0EK2r2Ttxuc FPBlzozOIg/Ne9VdSrPQft5o= X-Received: by 127.0.0.2 with SMTP id FE29YY7687511xsGwjAhP10f; Tue, 23 Jan 2024 09:10:22 -0800 X-Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web11.18700.1706029821544994076 for ; Tue, 23 Jan 2024 09:10:21 -0800 X-Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-612-EgpKa_RhOviJOXSDCQ2TcQ-1; Tue, 23 Jan 2024 12:10:16 -0500 X-MC-Unique: EgpKa_RhOviJOXSDCQ2TcQ-1 X-Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 283BD108C062; Tue, 23 Jan 2024 17:10:16 +0000 (UTC) X-Received: from [10.39.194.212] (unknown [10.39.194.212]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 358942166B32; Tue, 23 Jan 2024 17:10:15 +0000 (UTC) Message-ID: <8150b1eb-a1cb-e5b2-d48c-13ad596f17a0@redhat.com> Date: Tue, 23 Jan 2024 18:10:14 +0100 MIME-Version: 1.0 Subject: Re: [edk2-devel] [PATCH v3 5/5] MdeModulePkg: Extend NestedInterruptTplLib to support Arm CPUs To: devel@edk2.groups.io, mcb30@ipxe.org Cc: Ray Ni , Gerd Hoffmann , Ard Biesheuvel References: <17ACFF3FDD20CD9A.13754@groups.io> <20240123153104.2451759-1-mcb30@ipxe.org> <0102018d36f28a0d-300f8626-f368-49ab-b5e3-3c4edf84ce7e-000000@eu-west-1.amazonses.com> From: "Laszlo Ersek" In-Reply-To: <0102018d36f28a0d-300f8626-f368-49ab-b5e3-3c4edf84ce7e-000000@eu-west-1.amazonses.com> X-Scanned-By: MIMEDefang 3.4.1 on 10.11.54.6 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Precedence: Bulk List-Subscribe: List-Help: Sender: devel@edk2.groups.io List-Id: Mailing-List: list devel@edk2.groups.io; contact devel+owner@edk2.groups.io Reply-To: devel@edk2.groups.io,lersek@redhat.com List-Unsubscribe-Post: List-Unsubscribe=One-Click List-Unsubscribe: X-Gm-Message-State: OrtYTNxxKyO5vFSfzFhfl1icx7686176AA= Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-GND-Status: LEGIT Authentication-Results: spool.mail.gandi.net; dkim=pass header.d=groups.io header.s=20140610 header.b=pxKZI4p+; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=redhat.com (policy=none); spf=pass (spool.mail.gandi.net: domain of bounce@groups.io designates 66.175.222.108 as permitted sender) smtp.mailfrom=bounce@groups.io 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. >=20 > 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). >=20 > Tested by patching ArmPkg's TimerDxe to use NestedInterruptTplLib and > verifying that ArmVirtQemu passes the NestedInterruptTplLib self-tests > for both ARM and AARCH64. >=20 > Cc: Ard Biesheuvel > Signed-off-by: Michael Brown > --- > MdeModulePkg/MdeModulePkg.dsc | 2 +- > .../NestedInterruptTplLib.inf | 3 +++ > .../Library/NestedInterruptTplLib/Iret.c | 18 ++++++++++++++++++ > 3 files changed, 22 insertions(+), 1 deletion(-) >=20 > diff --git a/MdeModulePkg/MdeModulePkg.dsc b/MdeModulePkg/MdeModulePkg.ds= c > index 5b9ddfd26e75..4565b8e1b6e7 100644 > --- a/MdeModulePkg/MdeModulePkg.dsc > +++ b/MdeModulePkg/MdeModulePkg.dsc > @@ -462,6 +462,7 @@ [Components.IA32, Components.X64, Components.AARCH64] > =20 > [Components.IA32, Components.X64, Components.ARM, Components.AARCH64] > MdeModulePkg/Library/BrotliCustomDecompressLib/BrotliCustomDecompressL= ib.inf > + MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf > MdeModulePkg/Library/LzmaCustomDecompressLib/LzmaCustomDecompressLib.i= nf > 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.i= nf > - MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.inf > =20 > [Components.X64] > MdeModulePkg/Universal/CapsulePei/CapsuleX64.inf > diff --git a/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTp= lLib.inf b/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib= .inf > index e67d899b9446..1501f067d77f 100644 > --- a/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.in= f > +++ b/MdeModulePkg/Library/NestedInterruptTplLib/NestedInterruptTplLib.in= f > @@ -27,6 +27,9 @@ [Packages] > MdePkg/MdePkg.dec > MdeModulePkg/MdeModulePkg.dec > =20 > +[Packages.ARM, Packages.AARCH64] > + ArmPkg/ArmPkg.dec > + > [LibraryClasses] > BaseLib > DebugLib > diff --git a/MdeModulePkg/Library/NestedInterruptTplLib/Iret.c b/MdeModul= ePkg/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 > #include > =20 > +#if defined (MDE_CPU_AARCH64) || defined (MDE_CPU_ARM) > +#include > +#endif > + > #include "Iret.h" > =20 > /** > @@ -54,6 +58,20 @@ DisableInterruptsOnIret ( > Eflags.Bits.IF =3D 0; > SystemContext.SystemContextIa32->Eflags =3D Eflags.UintN; > =20 > + #elif defined (MDE_CPU_AARCH64) > + > + // > + // Set IRQ-disabled and FIQ-disabled flags. > + // > + SystemContext.SystemContextAArch64->SPSR |=3D (SPSR_I | SPSR_F); > + > + #elif defined (MDE_CPU_ARM) > + > + // > + // Set IRQ-disabled and FIQ-disabled flags. > + // > + SystemContext.SystemContextArm->CPSR |=3D (CPSR_IRQ | CPSR_FIQ); > + > #else > =20 > #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 , scavenge #ifdef MDE_CPU_ARM #include #elif defined (MDE_CPU_AARCH64) #include #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 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 (=3D implementation), and the macros we need don't actually come from ArmLib.h. Acked-by: Laszlo Ersek Thanks Laszlo -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D- 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/19134562= 12/xyzzy [rebecca@openfw.io] -=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-=3D-